| Message ID | 1495818213-345-3-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 26.05.17 at 19:03, <andrew.cooper3@citrix.com> 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. Jan
On 29/05/2017 09:58, Jan Beulich wrote: >>>> On 26.05.17 at 19:03, <andrew.cooper3@citrix.com> 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. ~Andrew
>>> On 29.05.17 at 11:03, <andrew.cooper3@citrix.com> wrote: > On 29/05/2017 09:58, Jan Beulich wrote: >>>>> On 26.05.17 at 19:03, <andrew.cooper3@citrix.com> 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@suse.com> Jan
diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c index 5c6a85b..972364f 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... */
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. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Tim Deegan <tim@xen.org> CC: George Dunlap <george.dunlap@eu.citrix.com> --- xen/arch/x86/mm/guest_walk.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-)