On 18.09.2019 12:47, Jan Beulich wrote:
> On 17.09.2019 17:09, Tamas K Lengyel wrote:
>> On Tue, Sep 17, 2019 at 8:24 AM Razvan Cojocaru
>> <rcojoc...@bbu.bitdefender.biz> wrote:
>>>
>>> On 9/17/19 5:11 PM, Alexandru Stefan ISAILA wrote:
>>>>>>>> +bool hvm_monitor_check_p2m(unsigned long gla, gfn_t gfn, uint32_t
>>>>>>>> pfec,
>>>>>>>> + uint16_t kind)
>>>>>>>> +{
>>>>>>>> + xenmem_access_t access;
>>>>>>>> + vm_event_request_t req = {};
>>>>>>>> + paddr_t gpa = (gfn_to_gaddr(gfn) | (gla & ~PAGE_MASK));
>>>>>>>> +
>>>>>>>> + ASSERT(current->arch.vm_event->send_event);
>>>>>>>> +
>>>>>>>> + current->arch.vm_event->send_event = false;
>>>>>>>> +
>>>>>>>> + if ( p2m_get_mem_access(current->domain, gfn, &access,
>>>>>>>> + altp2m_vcpu_idx(current)) != 0 )
>>>>>>>> + return false;
>>>>>>> ... next to the call here (but the maintainers of the file would
>>>>>>> have to judge in the end). That said, I continue to not understand
>>>>>>> why a not found entry means unrestricted access. Isn't it
>>>>>>> ->default_access which controls what such a "virtual" entry would
>>>>>>> permit?
>>>>>> I'm sorry for this misleading comment. The code states that if entry was
>>>>>> not found the access will be default_access and return 0. So in this
>>>>>> case the default_access will be checked.
>>>>>>
>>>>>> /* If request to get default access. */
>>>>>> if ( gfn_eq(gfn, INVALID_GFN) )
>>>>>> {
>>>>>> *access = memaccess[p2m->default_access];
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> If this clears thing up I can remove the "NOTE" part if the comment.
>>>>> I'm afraid it doesn't clear things up: I'm still lost as to why
>>>>> "entry not found" implies "full access". And I'm further lost as
>>>>> to what the code fragment above (dealing with INVALID_GFN, but
>>>>> not really the "entry not found" case, which would be INVALID_MFN
>>>>> coming back from a translation) is supposed to tell me.
>>>>>
>>>> It is safe enough to consider a invalid mfn from hostp2 to be a
>>>> violation. There is still a small problem with having the altp2m view
>>>> not having the page propagated from hostp2m. In this case we have to use
>>>> altp2m_get_effective_entry().
>>>
>>> In the absence of clear guidance from the Intel SDM on what the hardware
>>> default is for a page not present in the p2m, we should probably follow
>>> Jan's advice and check violations against default_access for such pages.
>>
>> The SDM is very clear that pages that are not present in the EPT are a
>> violation:
>>
>> 28.2.2
>> An EPT paging-structure entry is present if any of bits 2:0 is 1;
>> otherwise, the entry is not present. The processor
>> ignores bits 62:3 and uses the entry neither to reference another EPT
>> paging-structure entry nor to produce a
>> physical address. A reference using a guest-physical address whose
>> translation encounters an EPT paging-struc-
>> ture that is not present causes an EPT violation (see Section 28.2.3.2).
>>
>> 28.2.3.2
>> EPT Violations
>> An EPT violation may occur during an access using a guest-physical
>> address whose translation does not cause an
>> EPT misconfiguration. An EPT violation occurs in any of the following
>> situations:
>> • Translation of the guest-physical address encounters an EPT
>> paging-structure entry that is not present (see
>> Section 28.2.2).
>
> I'm not sure if / how this helps (other than to answer Razvan's
> immediate question): It was for a reason that I talked about
> "virtual" entries, e.g. ones that would be there if they had
> been propagated already. Albeit propagated ones probably aren't
> a good case here, since those don't have default_access
> permissions anyway.
>
> But anyway - what my original remark here was about was the
> (missing) distinction of the different failure modes of
> p2m_get_mem_access(). For example I'd expect a GFN mapping
> to physical memory access to which is emulated to go the
> -ESRCH return path, due to INVALID_MFN coming back. Yet such
> GFNs still ought to have access controls (at least in theory).
>
I agree with this and I think they should be treated as XENMEM_access_n.
If everyone is OK with this I will add a -ESRCH path that uses
XENMEM_access_n as access.
Alex
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
