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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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? In any event
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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