Re: [Xen-devel] [PATCH v2] x86/mm: Add mem access rights to NPT

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.09.2019 13:23, Alexandru Stefan ISAILA wrote:
> On 29.08.2019 18:04, Jan Beulich wrote:
>> On 22.08.2019 16:02, Alexandru Stefan ISAILA wrote:
>>> This patch adds access control for NPT mode.
>>>
>>> The access rights are stored in the NPT p2m table 56:53.
>>
>> Why starting from bit 53? I can't seem to find any use of bit 52.
> 
> There is a comment in page.h that warns that bit 12(52) is taken.
> "/*
>   * Bit 12 of a 24-bit flag mask. This corresponds to bit 52 of a pte.
>   * This is needed to distinguish between user and kernel PTEs since 
> _PAGE_USER
>   * is asserted for both.
>   */
> #define _PAGE_GUEST_KERNEL (1U<<12)
> "

But that's a PV-only thing. With sufficient care it should be
possible to have overlapping uses. And given that the available
bit are a pretty limited resource, I'd very much appreciate if
you at least tried to make this work.

>>> @@ -104,8 +112,32 @@ static unsigned long p2m_type_to_flags(const struct 
>>> p2m_domain *p2m,
>>>               flags |= _PAGE_PWT;
>>>               ASSERT(!level);
>>>           }
>>> -        return flags | P2M_BASE_FLAGS | _PAGE_PCD;
>>> +        flags |= P2M_BASE_FLAGS | _PAGE_PCD;
>>> +        break;
>>>       }
>>> +
>>> +    switch ( access )
>>> +    {
>>> +    case p2m_access_r:
>>> +        flags |= _PAGE_NX_BIT;
>>> +        flags &= ~_PAGE_RW;
>>> +        break;
>>> +    case p2m_access_rw:
>>> +        flags |= _PAGE_NX_BIT;
>>> +        break;
>>> +    case p2m_access_rx:
>>> +    case p2m_access_rx2rw:
>>> +        flags &= ~(_PAGE_NX_BIT | _PAGE_RW);
>>> +        break;
>>> +    case p2m_access_x:
>>> +        flags &= ~_PAGE_RW;
>>> +        break;
>>
>> I can't seem to be able to follow you here. In fact I don't see
>> how you would be able to express execute-only with NPT. If this
>> is really needed for some reason, then a justifying comment
>> should be added.
> 
> Execute-only should be expressed as not PAGE_RW and PAGE_NX_BIT not set.

But that still doesn't yield exec-only. Where is this "should be
expressed" stated? I.e. on what basis is it tolerable to also allow
read access despite a request to the contrary?

>>> @@ -474,6 +520,32 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
>>>       return rc;
>>>   }
>>>   
>>> +static int p2m_pt_check_access(p2m_access_t p2ma)
>>> +{
>>> +    switch ( p2ma )
>>> +    {
>>> +    case p2m_access_n:
>>> +    case p2m_access_w:
>>> +    case p2m_access_wx:
>>> +    case p2m_access_n2rwx:
>>> +        return -EINVAL;
>>
>> I'm not convinced EINVAL is appropriate here - the argument isn't
>> invalid, it's just that there's no way to represent it.
> 
> Would EPERM be a better return here?

Quite a bit better, yes. But still not optimal, but I confess that
I also can't find an optimal one. EDOM would look to be suitable too,
if one was to ignore the "math" aspect of it.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel