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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.06.17 at 13:22, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 01/06/17 11:51, Jan Beulich wrote:
>> While this perhaps is a worthwhile addition, my original request
>> really was to make more visible around the place where it matters
>> that the NX bit is part of the reserved ones when NX is off. Hence
>> I'm not sure the comment change is worthwhile, and if you dislike
>> adding the suggested ASSERT() I won't the patch be left as is.
> 
> I presume you means something like you won't mind if the patch is left
> as-is?

Oop, yes.

> How about this?
> 
> diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
> index 972364f..6055fec 100644
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -356,11 +356,19 @@ 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);
>  
> +    /*
> +     * Sanity check.  If EFER.NX is disabled, _PAGE_NX_BIT is reserved and
> +     * should have caused a translation failure before we get here.
> +     */
> +    if ( ar & _PAGE_NX_BIT )
> +        ASSERT(guest_nx_enabled(v));
> +
>  #if GUEST_PAGING_LEVELS >= 4 /* 64-bit only... */
>      /*
>       * If all access checks are thus far ok, check Protection Key for 64bit

That's fine, thanks.

> One problem I have with an ASSERT beside the "if ( (walk &
> PFEC_insn_fetch) && (ar & _PAGE_NX_BIT) )" is that it is mid-way through
> the permissions checks, rather than at the start, which is likely to get
> missed if future access checks get introduced ahead of the protection
> key checks.

I can understand this.

Jan


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