[Xen-changelog] [xen master] x86/pagewalk: Fix pagewalk's handling of instruction fetches

[patchbot@xxxxxxx]

commit a0b40c3e08bb81192063f97089cb8c3849b8cfa0
Author:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
AuthorDate: Tue May 23 16:32:30 2017 +0000
Commit:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CommitDate: Thu Jun 1 14:13:57 2017 +0100

    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@xxxxxxxxxx>
    Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
---
 xen/arch/x86/mm/guest_walk.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
index 5c6a85b..6055fec 100644
--- 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
--
generated by git-patchbot for /home/xen/git/xen.git#master

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