Re: [Xen-devel] [PATCH 2/2] x86/pagewalk: Fix pagewalk's handling of instruction fetches

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 29/05/17 10:15, Jan Beulich wrote:
>>>> On 29.05.17 at 11:03, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 29/05/2017 09:58, Jan Beulich wrote:
>>>>>> On 26.05.17 at 19:03, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> --- a/xen/arch/x86/mm/guest_walk.c
>>>> +++ b/xen/arch/x86/mm/guest_walk.c
>>>> @@ -114,22 +114,18 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain 
>>>> *p2m,
>>>>      ASSERT(!(walk & PFEC_implicit) ||
>>>>             !(walk & (PFEC_insn_fetch | PFEC_user_mode)));
>>>>  
>>>> -    /*
>>>> -     * PFEC_insn_fetch is only used as an input to pagetable walking if 
>>>> NX or
>>>> -     * SMEP are enabled.  Otherwise, instruction fetches are 
>>>> indistinguishable
>>>> -     * from data reads.
>>>> -     *
>>>> -     * This property can be demonstrated on real hardware by having NX and
>>>> -     * SMEP inactive, but SMAP active, and observing that EFLAGS.AC 
>>>> determines
>>>> -     * whether a pagefault occures for supervisor execution on user 
>>>> mappings.
>>>> -     */
>>>> -    if ( !(guest_nx_enabled(v) || guest_smep_enabled(v)) )
>>>> -        walk &= ~PFEC_insn_fetch;
>>>> -
>>>>      perfc_incr(guest_walk);
>>>>      memset(gw, 0, sizeof(*gw));
>>>>      gw->va = va;
>>>> -    gw->pfec = walk & (PFEC_insn_fetch | PFEC_user_mode | 
>>>> PFEC_write_access);
>>>> +    gw->pfec = walk & (PFEC_user_mode | PFEC_write_access);
>>>> +
>>>> +    /*
>>>> +     * PFEC_insn_fetch is only reported if NX or SMEP are enabled.  
>>>> Hardware
>>>> +     * still distingueses instruction fetches during determination of 
>>>> access
>>>> +     * rights.
>>>> +     */
>>>> +    if ( guest_nx_enabled(v) || guest_smep_enabled(v) )
>>>> +        gw->pfec |= (walk & PFEC_insn_fetch);
>>>>  
>>>>  #if GUEST_PAGING_LEVELS >= 3 /* PAE or 64... */
>>>>  #if GUEST_PAGING_LEVELS >= 4 /* 64-bit only... */
>>> Don't you another adjustment to
>>>
>>>     if ( (walk & PFEC_insn_fetch) && (ar & _PAGE_NX_BIT) )
>>>         /* Requested an instruction fetch and found NX? Fail. */
>>>         goto out;
>>>
>>> I can't see anything that would keep _PAGE_NX_BIT out of
>>> ar if NX is not enabled.
>> _PAGE_NX_BIT is reserved if NX is not enabled, and is accounted for in
>> guest_rsvd_bits() in guest_pt.h, and we never hit the access rights logic.
> Ah, right. But perhaps worth having a respective ASSERT()
> here, at once serving as documentation?

I could, but it would feel be out of place.  NX being incorrectly set is
a translation failure, and by definition, the translation needs to have
succeeded before permissions get considered.

Would this clarification be acceptable?

index 5c6a85b..6d6b454 100644
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -360,8 +360,9 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain
*p2m,
     gw->pfec |= PFEC_page_present;
 
     /*
-     * The pagetable walk has returned a successful translation.  Now check
-     * access rights to see whether the access should succeed.
+     * The pagetable walk has returned a successful translation (i.e. All
+     * PTEs are present and have no reserved bits set).  Now check access
+     * rights to see whether the access should succeed.
      */
     ar = (ar_and & AR_ACCUM_AND) | (ar_or & AR_ACCUM_OR);
 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel