x86/pagewalk: Fix pagewalk's handling of instruction fetches

Despite the claim in the comment (which was based partly on the code already
being like that, and mistaken reasoning because of Xen leaking NX into guest
context), reality differs.

Use of the SMAP feature without NX, or in a 2-level guest, demonstrate an
observable difference between reads and instruction fetches, despite
PFEC_insn_fetch not being reported in the #PF error code.  This demonstrates
that instruction fetches are distinguished from data reads even without
PFEC_insn_fetch being reported.

Alter the pagewalk logic to keep the pagewalk insn_fetch input intact, but
only conditionally report insn_fetch in the error code.  This logic is more
in line with the Intel SDM text:

 * I/D flag (bit 4).
   This flag is 1 if (1) the access causing the page-fault exception was an
   instruction fetch; and (2) either (a) CR4.SMEP = 1; or (b) both (i) CR4.PAE
   = 1 (either PAE paging or 4-level paging is in use); and (ii) IA32_EFER.NXE
   = 1. Otherwise, the flag is 0. This flag describes the access causing the
   page-fault exception, not the access rights specified by paging.

and the AMD SDM text:

 * I/D - Bit 4. If this bit is set to 1, it indicates that the access that
   caused the page fault was an instruction fetch. Otherwise, this bit is
   cleared to 0. This bit is only defined if no-execute feature is enabled
   (EFER.NXE=1 && CR4.PAE=1).

Curiously, the AMD manual doesn't mention SMEP despite some Fam16h processors
and all Fam17h processors supporting it.  Experimentally, it behaves as
described by Intel.

In addition, add some extra clarification and sanity checking around the use
of NX for the access checks, where it might be reserved.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
index 5c6a85b988e744f9243c520435c3a8f5f1ab86cb..6055fec1ad8a26d2a15023abe102a9c58bede3c8 100644 (file)
--- 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... */
@@ -360,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