Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4

Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4

[Paolo Bonzini]

Il 28/03/2014 18:36, Feng Wu ha scritto:
> +	smap = kvm_read_cr4_bits(vcpu, X86_CR4_SMAP);

You are overwriting this variable below, but that is not okay because
the value of CR4 must be considered separately in each iteration.  This
also hides a uninitialized-variable bug for "smap" correctly in the EPT
case.

To avoid that, rename this variable to cr4_smap; it's probably better 
to rename smep to cr4_smep too.

>  	for (byte = 0; byte < ARRAY_SIZE(mmu->permissions); ++byte) {
>  		pfec = byte << 1;
>  		map = 0;
>  		wf = pfec & PFERR_WRITE_MASK;
>  		uf = pfec & PFERR_USER_MASK;
>  		ff = pfec & PFERR_FETCH_MASK;
> +		smapf = pfec & PFERR_RSVD_MASK;

The reader will expect PFERR_RSVD_MASK to be zero here.  So please
add a comment: /* PFERR_RSVD_MASK is set in pfec if ... */".

>  		for (bit = 0; bit < 8; ++bit) {
>  			x = bit & ACC_EXEC_MASK;
>  			w = bit & ACC_WRITE_MASK;
> @@ -3627,11 +3629,27 @@ static void update_permission_bitmask(struct kvm_vcpu *vcpu,
>  				w |= !is_write_protection(vcpu) && !uf;
>  				/* Disallow supervisor fetches of user code if cr4.smep */
>  				x &= !(smep && u && !uf);
> +
> +				/*
> +				 * SMAP:kernel-mode data accesses from user-mode
> +				 * mappings should fault. A fault is considered
> +				 * as a SMAP violation if all of the following
> +				 * conditions are ture:
> +				 *   - X86_CR4_SMAP is set in CR4
> +				 *   - An user page is accessed
> +				 *   - Page fault in kernel mode
> +				 *   - !(CPL<3 && X86_EFLAGS_AC is set)
> +				 *
> +				 *   Here, we cover the first three conditions,
> +				 *   we need to check CPL and X86_EFLAGS_AC in
> +				 *   permission_fault() dynamiccally

"dynamically".  These three lines however are not entirely correct.  We do
cover the last condition here, it is in smapf.  So perhaps something like

 * Here, we cover the first three conditions.
 * The CPL and X86_EFLAGS_AC is in smapf, which
 * permission_fault() computes dynamically.

> +				 */
> +				smap = smap && smapf && u && !uf;

SMAP does not affect instruction fetches.  Do you need "&& !ff" here?  Perhaps
it's clearer to add it even if it is not strictly necessary.

Please write just "smap = cr4_smap && u && !uf && !ff" here, and add back smapf below
in the assignment to "fault".  This makes the code more homogeneous.

>  			} else
>  				/* Not really needed: no U/S accesses on ept  */
>  				u = 1;
> -			fault = (ff && !x) || (uf && !u) || (wf && !w);
> +			fault = (ff && !x) || (uf && !u) || (wf && !w) || smap;

...

> +
> +	/*
> +	 * If CPL < 3, SMAP protections are disabled if EFLAGS.AC = 1.
> +	 *
> +	 * If CPL = 3, SMAP applies to all supervisor-mode data accesses
> +	 * (these are implicit supervisor accesses) regardless of the value
> +	 * of EFLAGS.AC.
> +	 *
> +	 * So we need to check CPL and EFLAGS.AC to detect whether there is
> +	 * a SMAP violation.
> +	 */
> +
> +	smapf = ((mmu->permissions[(pfec|PFERR_RSVD_MASK) >> 1] >> pte_access) &
> +		 1) && !((cpl < 3) && ((rflags & X86_EFLAGS_AC) == 1));
> +
> +	return ((mmu->permissions[pfec >> 1] >> pte_access) & 1) || smapf;

You do not need two separate accesses.  Just add PFERR_RSVD_MASK to pfec if
the conditions for SMAP are satisfied.  There are two possibilities:

1) setting PFERR_RSVD_MASK if SMAP is being enforced, that is if CPL = 3
|| AC = 0.  This is what you are doing now.

2) setting PFERR_RSVD_MASK if SMAP is being overridden, that is if CPL < 3
&& AC = 1.  You then have to invert the bit in update_permission_bitmask.

Please consider both choices, and pick the one that gives better code.

Also, this must be written in a branchless way.  Branchless tricks are common
throughout the MMU code because the hit rate of most branches is pretty much
50%-50%.  This is also true in this case, at least if SMAP is in use (if it
is not in use, we'll have AC=0 most of the time).

I don't want to spoil the fun, but I don't want to waste your time either
so I rot13'ed my solution and placed it after the signature. ;)

As to nested virtualization, I reread the code and it should already work,
because it sets PFERR_USER_MASK.  This means uf=1, and a SMAP fault will
never trigger with uf=1.

Thanks for following this!  Please include "v3" in the patch subject on
your next post!

Paolo

------------------------------------- 8< --------------------------------------
Nqq qrsvavgvbaf sbe CSREE_*_OVG (0 sbe cerfrag, 1 sbe jevgr, rgp.) naq
hfr gur sbyybjvat:

        vag vaqrk, fznc;

        /*
         * Vs PCY < 3, FZNC cebgrpgvbaf ner qvfnoyrq vs RSYNTF.NP = 1.
         *
         * Vs PCY = 3, FZNC nccyvrf gb nyy fhcreivfbe-zbqr qngn npprffrf
         * (gurfr ner vzcyvpvg fhcreivfbe npprffrf) ertneqyrff bs gur inyhr
         * bs RSYNTF.NP.
         *
         * Guvf pbzchgrf (pcy < 3) && (esyntf & K86_RSYNTF_NP), yrnivat
         * gur erfhyg va K86_RSYNTF_NP.  Jr gura vafreg vg va cynpr
         * bs gur CSREE_EFIQ_ZNFX ovg; guvf ovg jvyy nyjnlf or mreb va csrp,
         * ohg vg jvyy or bar va vaqrk vs FZNC purpxf ner orvat bireevqqra.
         * Vg vf vzcbegnag gb xrrc guvf oenapuyrff.
         */
        fznc = (pcy - 3) & (esyntf & K86_RSYNTF_NP);
        vaqrk =
           (csrp >> 1) +
           (fznc >> (K86_RSYNTF_NP_OVG - CSREE_EFIQ_OVG + 1));

        erghea (zzh->crezvffvbaf[vaqrk] >> cgr_npprff) & 1;

Gur qverpgvba bs CSREE_EFIQ_ZNFX vf gur bccbfvgr pbzcnerq gb lbhe pbqr.

RE: [PATCH 2/4] KVM: Add SMAP support when setting CR4

[Wu, Feng]

> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Friday, March 28, 2014 8:03 PM
> To: Wu, Feng; gleb@redhat.com; hpa@zytor.com; kvm@vger.kernel.org
> Subject: Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4
> 
> Il 28/03/2014 18:36, Feng Wu ha scritto:
> > +	smap = kvm_read_cr4_bits(vcpu, X86_CR4_SMAP);
> 
> You are overwriting this variable below, but that is not okay because
> the value of CR4 must be considered separately in each iteration.  This
> also hides a uninitialized-variable bug for "smap" correctly in the EPT
> case.
> 
> To avoid that, rename this variable to cr4_smap; it's probably better
> to rename smep to cr4_smep too.
> 
> >  	for (byte = 0; byte < ARRAY_SIZE(mmu->permissions); ++byte) {
> >  		pfec = byte << 1;
> >  		map = 0;
> >  		wf = pfec & PFERR_WRITE_MASK;
> >  		uf = pfec & PFERR_USER_MASK;
> >  		ff = pfec & PFERR_FETCH_MASK;
> > +		smapf = pfec & PFERR_RSVD_MASK;
> 
> The reader will expect PFERR_RSVD_MASK to be zero here.  So please
> add a comment: /* PFERR_RSVD_MASK is set in pfec if ... */".
> 
> >  		for (bit = 0; bit < 8; ++bit) {
> >  			x = bit & ACC_EXEC_MASK;
> >  			w = bit & ACC_WRITE_MASK;
> > @@ -3627,11 +3629,27 @@ static void update_permission_bitmask(struct
> kvm_vcpu *vcpu,
> >  				w |= !is_write_protection(vcpu) && !uf;
> >  				/* Disallow supervisor fetches of user code if cr4.smep */
> >  				x &= !(smep && u && !uf);
> > +
> > +				/*
> > +				 * SMAP:kernel-mode data accesses from user-mode
> > +				 * mappings should fault. A fault is considered
> > +				 * as a SMAP violation if all of the following
> > +				 * conditions are ture:
> > +				 *   - X86_CR4_SMAP is set in CR4
> > +				 *   - An user page is accessed
> > +				 *   - Page fault in kernel mode
> > +				 *   - !(CPL<3 && X86_EFLAGS_AC is set)
> > +				 *
> > +				 *   Here, we cover the first three conditions,
> > +				 *   we need to check CPL and X86_EFLAGS_AC in
> > +				 *   permission_fault() dynamiccally
> 
> "dynamically".  These three lines however are not entirely correct.  We do
> cover the last condition here, it is in smapf.  So perhaps something like
> 
>  * Here, we cover the first three conditions.
>  * The CPL and X86_EFLAGS_AC is in smapf, which
>  * permission_fault() computes dynamically.
> 
> > +				 */
> > +				smap = smap && smapf && u && !uf;
> 
> SMAP does not affect instruction fetches.  Do you need "&& !ff" here?
> Perhaps
> it's clearer to add it even if it is not strictly necessary.
> 
> Please write just "smap = cr4_smap && u && !uf && !ff" here, and add back
> smapf below
> in the assignment to "fault".  This makes the code more homogeneous.
> 
> >  			} else
> >  				/* Not really needed: no U/S accesses on ept  */
> >  				u = 1;
> > -			fault = (ff && !x) || (uf && !u) || (wf && !w);
> > +			fault = (ff && !x) || (uf && !u) || (wf && !w) || smap;
> 
> ...
> 
> > +
> > +	/*
> > +	 * If CPL < 3, SMAP protections are disabled if EFLAGS.AC = 1.
> > +	 *
> > +	 * If CPL = 3, SMAP applies to all supervisor-mode data accesses
> > +	 * (these are implicit supervisor accesses) regardless of the value
> > +	 * of EFLAGS.AC.
> > +	 *
> > +	 * So we need to check CPL and EFLAGS.AC to detect whether there is
> > +	 * a SMAP violation.
> > +	 */
> > +
> > +	smapf = ((mmu->permissions[(pfec|PFERR_RSVD_MASK) >> 1] >>
> pte_access) &
> > +		 1) && !((cpl < 3) && ((rflags & X86_EFLAGS_AC) == 1));
> > +
> > +	return ((mmu->permissions[pfec >> 1] >> pte_access) & 1) || smapf;
> 
> You do not need two separate accesses.  Just add PFERR_RSVD_MASK to pfec
> if
> the conditions for SMAP are satisfied.  There are two possibilities:
> 
> 1) setting PFERR_RSVD_MASK if SMAP is being enforced, that is if CPL = 3
> || AC = 0.  This is what you are doing now.
> 
> 2) setting PFERR_RSVD_MASK if SMAP is being overridden, that is if CPL < 3
> && AC = 1.  You then have to invert the bit in update_permission_bitmask.
> 
> Please consider both choices, and pick the one that gives better code.
> 
> Also, this must be written in a branchless way.  Branchless tricks are common
> throughout the MMU code because the hit rate of most branches is pretty
> much
> 50%-50%.  This is also true in this case, at least if SMAP is in use (if it
> is not in use, we'll have AC=0 most of the time).
> 
> I don't want to spoil the fun, but I don't want to waste your time either
> so I rot13'ed my solution and placed it after the signature. ;)
> 
> As to nested virtualization, I reread the code and it should already work,
> because it sets PFERR_USER_MASK.  This means uf=1, and a SMAP fault will
> never trigger with uf=1.
> 
> Thanks for following this!  Please include "v3" in the patch subject on
> your next post!

Thanks very much for your detailed comments, I will make a v3 patch soon!
> 
> Paolo
> 
> ------------------------------------- 8< --------------------------------------
> Nqq qrsvavgvbaf sbe CSREE_*_OVG (0 sbe cerfrag, 1 sbe jevgr, rgp.) naq
> hfr gur sbyybjvat:
> 
>         vag vaqrk, fznc;
> 
>         /*
>          * Vs PCY < 3, FZNC cebgrpgvbaf ner qvfnoyrq vs RSYNTF.NP = 1.
>          *
>          * Vs PCY = 3, FZNC nccyvrf gb nyy fhcreivfbe-zbqr qngn npprffrf
>          * (gurfr ner vzcyvpvg fhcreivfbe npprffrf) ertneqyrff bs gur inyhr
>          * bs RSYNTF.NP.
>          *
>          * Guvf pbzchgrf (pcy < 3) && (esyntf & K86_RSYNTF_NP), yrnivat
>          * gur erfhyg va K86_RSYNTF_NP.  Jr gura vafreg vg va cynpr
>          * bs gur CSREE_EFIQ_ZNFX ovg; guvf ovg jvyy nyjnlf or mreb va csrp,
>          * ohg vg jvyy or bar va vaqrk vs FZNC purpxf ner orvat bireevqqra.
>          * Vg vf vzcbegnag gb xrrc guvf oenapuyrff.
>          */
>         fznc = (pcy - 3) & (esyntf & K86_RSYNTF_NP);
>         vaqrk =
>            (csrp >> 1) +
>            (fznc >> (K86_RSYNTF_NP_OVG - CSREE_EFIQ_OVG + 1));
> 
>         erghea (zzh->crezvffvbaf[vaqrk] >> cgr_npprff) & 1;
> 
> Gur qverpgvba bs CSREE_EFIQ_ZNFX vf gur bccbfvgr pbzcnerq gb lbhe pbqr.

Thanks,
Feng

[PATCH 0/4] KVM: enable Intel SMAP for KVM

[Feng Wu]

Supervisor Mode Access Prevention (SMAP) is a new security feature 
disclosed by Intel, please refer to the following document: 

http://software.intel.com/sites/default/files/319433-014.pdf
 
Every access to a linear address is either a supervisor-mode access
or a user-mode access. All accesses performed while the current
privilege level (CPL) is less than 3 are supervisor-mode accesses.
If CPL = 3, accesses are generally user-mode accesses. However, some
operations implicitly access system data structures, and the resulting
accesses to those data structures are supervisor-mode accesses regardless
of CPL. Examples of such implicit supervisor accesses include the following:
accesses to the global descriptor table (GDT) or local descriptor table
(LDT) to load a segment descriptor; accesses to the interrupt descriptor
table (IDT) when delivering an interrupt or exception; and accesses to the
task-state segment (TSS) as part of a task switch or change of CPL.

If CR4.SMAP = 1, supervisor-mode data accesses are not allowed to linear
addresses that are accessible in user mode. If CPL < 3, SMAP protections
are disabled if EFLAGS.AC = 1. If CPL = 3, SMAP applies to all supervisor-mode
data accesses (these are implicit supervisor accesses) regardless of the
value of EFLAGS.AC.

This patchset pass-through SMAP feature to guests, and let guests
benefit from it.

Version 1:
  * Remove SMAP bit from CR4_RESERVED_BITS.
  * Add SMAP support when setting CR4
  * Disable SMAP for guests in EPT realmode and EPT unpaging mode
  * Expose SMAP feature to guest

Version 1:
  * Change the logic of updatinng mmu permission bitmap for SMAP violation
  * Expose SMAP feature to guest in the last patch of this series.

Feng Wu (4):
  KVM: Remove SMAP bit from CR4_RESERVED_BITS.
  KVM: Add SMAP support when setting CR4
  KVM: Disable SMAP for guests in EPT realmode and EPT unpaging mode
  KVM: expose SMAP feature to guest

 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kvm/cpuid.c            |  2 +-
 arch/x86/kvm/cpuid.h            |  8 ++++++++
 arch/x86/kvm/mmu.c              | 24 +++++++++++++++++++++---
 arch/x86/kvm/mmu.h              | 26 +++++++++++++++++++++++---
 arch/x86/kvm/paging_tmpl.h      |  2 +-
 arch/x86/kvm/vmx.c              | 11 ++++++-----
 arch/x86/kvm/x86.c              |  9 ++++++++-
 8 files changed, 69 insertions(+), 15 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] KVM: Remove SMAP bit from CR4_RESERVED_BITS.

[Feng Wu]

This patch removes SMAP bit from CR4_RESERVED_BITS.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 arch/x86/include/asm/kvm_host.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index fdf83af..4eeb049 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -60,7 +60,7 @@
 			  | X86_CR4_PSE | X86_CR4_PAE | X86_CR4_MCE     \
 			  | X86_CR4_PGE | X86_CR4_PCE | X86_CR4_OSFXSR | X86_CR4_PCIDE \
 			  | X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE \
-			  | X86_CR4_OSXMMEXCPT | X86_CR4_VMXE))
+			  | X86_CR4_OSXMMEXCPT | X86_CR4_VMXE | X86_CR4_SMAP))
 
 #define CR8_RESERVED_BITS (~(unsigned long)X86_CR8_TPR)
 
-- 
1.8.3.1

[PATCH 2/4] KVM: Add SMAP support when setting CR4

[Feng Wu]

This patch adds SMAP handling logic when setting CR4 for guests

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 arch/x86/kvm/cpuid.h       |  8 ++++++++
 arch/x86/kvm/mmu.c         | 24 +++++++++++++++++++++---
 arch/x86/kvm/mmu.h         | 26 +++++++++++++++++++++++---
 arch/x86/kvm/paging_tmpl.h |  2 +-
 arch/x86/kvm/x86.c         |  9 ++++++++-
 5 files changed, 61 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index a2a1bb7..eeecbed 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -48,6 +48,14 @@ static inline bool guest_cpuid_has_smep(struct kvm_vcpu *vcpu)
 	return best && (best->ebx & bit(X86_FEATURE_SMEP));
 }
 
+static inline bool guest_cpuid_has_smap(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry(vcpu, 7, 0);
+	return best && (best->ebx & bit(X86_FEATURE_SMAP));
+}
+
 static inline bool guest_cpuid_has_fsgsbase(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 9b53135..83b7f8d 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3601,20 +3601,22 @@ static void reset_rsvds_bits_mask_ept(struct kvm_vcpu *vcpu,
 	}
 }
 
-static void update_permission_bitmask(struct kvm_vcpu *vcpu,
+void update_permission_bitmask(struct kvm_vcpu *vcpu,
 		struct kvm_mmu *mmu, bool ept)
 {
 	unsigned bit, byte, pfec;
 	u8 map;
-	bool fault, x, w, u, wf, uf, ff, smep;
+	bool fault, x, w, u, wf, uf, ff, smapf, smep, smap;
 
 	smep = kvm_read_cr4_bits(vcpu, X86_CR4_SMEP);
+	smap = kvm_read_cr4_bits(vcpu, X86_CR4_SMAP);
 	for (byte = 0; byte < ARRAY_SIZE(mmu->permissions); ++byte) {
 		pfec = byte << 1;
 		map = 0;
 		wf = pfec & PFERR_WRITE_MASK;
 		uf = pfec & PFERR_USER_MASK;
 		ff = pfec & PFERR_FETCH_MASK;
+		smapf = pfec & PFERR_RSVD_MASK;
 		for (bit = 0; bit < 8; ++bit) {
 			x = bit & ACC_EXEC_MASK;
 			w = bit & ACC_WRITE_MASK;
@@ -3627,11 +3629,27 @@ static void update_permission_bitmask(struct kvm_vcpu *vcpu,
 				w |= !is_write_protection(vcpu) && !uf;
 				/* Disallow supervisor fetches of user code if cr4.smep */
 				x &= !(smep && u && !uf);
+
+				/*
+				 * SMAP:kernel-mode data accesses from user-mode
+				 * mappings should fault. A fault is considered
+				 * as a SMAP violation if all of the following
+				 * conditions are ture:
+				 *   - X86_CR4_SMAP is set in CR4
+				 *   - An user page is accessed
+				 *   - Page fault in kernel mode
+				 *   - !(CPL<3 && X86_EFLAGS_AC is set)
+				 *
+				 *   Here, we cover the first three conditions,
+				 *   we need to check CPL and X86_EFLAGS_AC in
+				 *   permission_fault() dynamiccally
+				 */
+				smap = smap && smapf && u && !uf;
 			} else
 				/* Not really needed: no U/S accesses on ept  */
 				u = 1;
 
-			fault = (ff && !x) || (uf && !u) || (wf && !w);
+			fault = (ff && !x) || (uf && !u) || (wf && !w) || smap;
 			map |= fault << bit;
 		}
 		mmu->permissions[byte] = map;
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 2926152..9d7a0b3 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -73,6 +73,8 @@ int handle_mmio_page_fault_common(struct kvm_vcpu *vcpu, u64 addr, bool direct);
 void kvm_init_shadow_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *context);
 void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, struct kvm_mmu *context,
 		bool execonly);
+void update_permission_bitmask(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
+		bool ept);
 
 static inline unsigned int kvm_mmu_available_pages(struct kvm *kvm)
 {
@@ -110,10 +112,28 @@ static inline bool is_write_protection(struct kvm_vcpu *vcpu)
  * Will a fault with a given page-fault error code (pfec) cause a permission
  * fault with the given access (in ACC_* format)?
  */
-static inline bool permission_fault(struct kvm_mmu *mmu, unsigned pte_access,
-				    unsigned pfec)
+static inline bool permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
+				    unsigned pte_access, unsigned pfec)
 {
-	return (mmu->permissions[pfec >> 1] >> pte_access) & 1;
+	bool smapf;
+	int cpl = kvm_x86_ops->get_cpl(vcpu);
+	unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
+
+	/*
+	 * If CPL < 3, SMAP protections are disabled if EFLAGS.AC = 1.
+	 *
+	 * If CPL = 3, SMAP applies to all supervisor-mode data accesses
+	 * (these are implicit supervisor accesses) regardless of the value
+	 * of EFLAGS.AC.
+	 *
+	 * So we need to check CPL and EFLAGS.AC to detect whether there is
+	 * a SMAP violation.
+	 */
+
+	smapf = ((mmu->permissions[(pfec|PFERR_RSVD_MASK) >> 1] >> pte_access) &
+		 1) && !((cpl < 3) && ((rflags & X86_EFLAGS_AC) == 1));
+
+	return ((mmu->permissions[pfec >> 1] >> pte_access) & 1) || smapf;
 }
 
 void kvm_mmu_invalidate_zap_all_pages(struct kvm *kvm);
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index cba218a..4107765 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -353,7 +353,7 @@ retry_walk:
 		walker->ptes[walker->level - 1] = pte;
 	} while (!is_last_gpte(mmu, walker->level, pte));
 
-	if (unlikely(permission_fault(mmu, pte_access, access))) {
+	if (unlikely(permission_fault(vcpu, mmu, pte_access, access))) {
 		errcode |= PFERR_PRESENT_MASK;
 		goto error;
 	}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2b85784..5869c6d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -646,6 +646,9 @@ int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 	if (!guest_cpuid_has_smep(vcpu) && (cr4 & X86_CR4_SMEP))
 		return 1;
 
+	if (!guest_cpuid_has_smap(vcpu) && (cr4 & X86_CR4_SMAP))
+		return 1;
+
 	if (!guest_cpuid_has_fsgsbase(vcpu) && (cr4 & X86_CR4_FSGSBASE))
 		return 1;
 
@@ -674,6 +677,9 @@ int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 	    (!(cr4 & X86_CR4_PCIDE) && (old_cr4 & X86_CR4_PCIDE)))
 		kvm_mmu_reset_context(vcpu);
 
+	if ((cr4 ^ old_cr4) & X86_CR4_SMAP)
+		update_permission_bitmask(vcpu, vcpu->arch.walk_mmu, false);
+
 	if ((cr4 ^ old_cr4) & X86_CR4_OSXSAVE)
 		kvm_update_cpuid(vcpu);
 
@@ -4108,7 +4114,8 @@ static int vcpu_mmio_gva_to_gpa(struct kvm_vcpu *vcpu, unsigned long gva,
 		| (write ? PFERR_WRITE_MASK : 0);
 
 	if (vcpu_match_mmio_gva(vcpu, gva)
-	    && !permission_fault(vcpu->arch.walk_mmu, vcpu->arch.access, access)) {
+	    && !permission_fault(vcpu, vcpu->arch.walk_mmu,
+				 vcpu->arch.access, access)) {
 		*gpa = vcpu->arch.mmio_gfn << PAGE_SHIFT |
 					(gva & (PAGE_SIZE - 1));
 		trace_vcpu_match_mmio(gva, *gpa, write, false);
-- 
1.8.3.1

[PATCH 3/4] KVM: Disable SMAP for guests in EPT realmode and EPT unpaging mode

[Feng Wu]

SMAP is disabled if CPU is in non-paging mode in hardware.
However KVM always uses paging mode to emulate guest non-paging
mode with TDP. To emulate this behavior, SMAP needs to be
manually disabled when guest switches to non-paging mode.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 arch/x86/kvm/vmx.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 3927528..e58cb5f 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3452,13 +3452,14 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 			hw_cr4 &= ~X86_CR4_PAE;
 			hw_cr4 |= X86_CR4_PSE;
 			/*
-			 * SMEP is disabled if CPU is in non-paging mode in
-			 * hardware. However KVM always uses paging mode to
+			 * SMEP/SMAP is disabled if CPU is in non-paging mode
+			 * in hardware. However KVM always uses paging mode to
 			 * emulate guest non-paging mode with TDP.
-			 * To emulate this behavior, SMEP needs to be manually
-			 * disabled when guest switches to non-paging mode.
+			 * To emulate this behavior, SMEP/SMAP needs to be
+			 * manually disabled when guest switches to non-paging
+			 * mode.
 			 */
-			hw_cr4 &= ~X86_CR4_SMEP;
+			hw_cr4 &= ~(X86_CR4_SMEP | X86_CR4_SMAP);
 		} else if (!(cr4 & X86_CR4_PAE)) {
 			hw_cr4 &= ~X86_CR4_PAE;
 		}
-- 
1.8.3.1

[PATCH 4/4] KVM: expose SMAP feature to guest

[Feng Wu]

This patch exposes SMAP feature to guest

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 arch/x86/kvm/cpuid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index c697625..deb5f9b 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -303,7 +303,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
 	/* cpuid 7.0.ebx */
 	const u32 kvm_supported_word9_x86_features =
 		F(FSGSBASE) | F(BMI1) | F(HLE) | F(AVX2) | F(SMEP) |
-		F(BMI2) | F(ERMS) | f_invpcid | F(RTM);
+		F(BMI2) | F(ERMS) | f_invpcid | F(RTM) | F(SMAP);
 
 	/* all calls to cpuid_count() should be made on the same cpu */
 	get_cpu();
-- 
1.8.3.1

RE: [PATCH 2/4] KVM: Add SMAP support when setting CR4

[Wu, Feng]

> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Friday, March 28, 2014 8:03 PM
> To: Wu, Feng; gleb@redhat.com; hpa@zytor.com; kvm@vger.kernel.org
> Subject: Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4
> 
> Il 28/03/2014 18:36, Feng Wu ha scritto:
> > +	smap = kvm_read_cr4_bits(vcpu, X86_CR4_SMAP);
> 
> You are overwriting this variable below, but that is not okay because
> the value of CR4 must be considered separately in each iteration.  This
> also hides a uninitialized-variable bug for "smap" correctly in the EPT
> case.
> 
> To avoid that, rename this variable to cr4_smap; it's probably better
> to rename smep to cr4_smep too.
> 
> >  	for (byte = 0; byte < ARRAY_SIZE(mmu->permissions); ++byte) {
> >  		pfec = byte << 1;
> >  		map = 0;
> >  		wf = pfec & PFERR_WRITE_MASK;
> >  		uf = pfec & PFERR_USER_MASK;
> >  		ff = pfec & PFERR_FETCH_MASK;
> > +		smapf = pfec & PFERR_RSVD_MASK;
> 
> The reader will expect PFERR_RSVD_MASK to be zero here.  So please
> add a comment: /* PFERR_RSVD_MASK is set in pfec if ... */".
> 
> >  		for (bit = 0; bit < 8; ++bit) {
> >  			x = bit & ACC_EXEC_MASK;
> >  			w = bit & ACC_WRITE_MASK;
> > @@ -3627,11 +3629,27 @@ static void update_permission_bitmask(struct
> kvm_vcpu *vcpu,
> >  				w |= !is_write_protection(vcpu) && !uf;
> >  				/* Disallow supervisor fetches of user code if cr4.smep */
> >  				x &= !(smep && u && !uf);
> > +
> > +				/*
> > +				 * SMAP:kernel-mode data accesses from user-mode
> > +				 * mappings should fault. A fault is considered
> > +				 * as a SMAP violation if all of the following
> > +				 * conditions are ture:
> > +				 *   - X86_CR4_SMAP is set in CR4
> > +				 *   - An user page is accessed
> > +				 *   - Page fault in kernel mode
> > +				 *   - !(CPL<3 && X86_EFLAGS_AC is set)
> > +				 *
> > +				 *   Here, we cover the first three conditions,
> > +				 *   we need to check CPL and X86_EFLAGS_AC in
> > +				 *   permission_fault() dynamiccally
> 
> "dynamically".  These three lines however are not entirely correct.  We do
> cover the last condition here, it is in smapf.  So perhaps something like
> 
>  * Here, we cover the first three conditions.
>  * The CPL and X86_EFLAGS_AC is in smapf, which
>  * permission_fault() computes dynamically.
> 
> > +				 */
> > +				smap = smap && smapf && u && !uf;
> 
> SMAP does not affect instruction fetches.  Do you need "&& !ff" here?
> Perhaps
> it's clearer to add it even if it is not strictly necessary.
> 
> Please write just "smap = cr4_smap && u && !uf && !ff" here, and add back
> smapf below
> in the assignment to "fault".  This makes the code more homogeneous.
> 
> >  			} else
> >  				/* Not really needed: no U/S accesses on ept  */
> >  				u = 1;
> > -			fault = (ff && !x) || (uf && !u) || (wf && !w);
> > +			fault = (ff && !x) || (uf && !u) || (wf && !w) || smap;
> 
> ...
> 
> > +
> > +	/*
> > +	 * If CPL < 3, SMAP protections are disabled if EFLAGS.AC = 1.
> > +	 *
> > +	 * If CPL = 3, SMAP applies to all supervisor-mode data accesses
> > +	 * (these are implicit supervisor accesses) regardless of the value
> > +	 * of EFLAGS.AC.
> > +	 *
> > +	 * So we need to check CPL and EFLAGS.AC to detect whether there is
> > +	 * a SMAP violation.
> > +	 */
> > +
> > +	smapf = ((mmu->permissions[(pfec|PFERR_RSVD_MASK) >> 1] >>
> pte_access) &
> > +		 1) && !((cpl < 3) && ((rflags & X86_EFLAGS_AC) == 1));
> > +
> > +	return ((mmu->permissions[pfec >> 1] >> pte_access) & 1) || smapf;
> 
> You do not need two separate accesses.  Just add PFERR_RSVD_MASK to pfec
> if
> the conditions for SMAP are satisfied.  There are two possibilities:
> 
> 1) setting PFERR_RSVD_MASK if SMAP is being enforced, that is if CPL = 3
> || AC = 0.  This is what you are doing now.
> 
> 2) setting PFERR_RSVD_MASK if SMAP is being overridden, that is if CPL < 3
> && AC = 1.  You then have to invert the bit in update_permission_bitmask.
> 
> Please consider both choices, and pick the one that gives better code.
> 
> Also, this must be written in a branchless way.  Branchless tricks are common
> throughout the MMU code because the hit rate of most branches is pretty
> much
> 50%-50%.  This is also true in this case, at least if SMAP is in use (if it
> is not in use, we'll have AC=0 most of the time).
> 
> I don't want to spoil the fun, but I don't want to waste your time either
> so I rot13'ed my solution and placed it after the signature. ;)
> 
> As to nested virtualization, I reread the code and it should already work,
> because it sets PFERR_USER_MASK.  This means uf=1, and a SMAP fault will
> never trigger with uf=1.
> 
> Thanks for following this!  Please include "v3" in the patch subject on
> your next post!
> 
> Paolo
> 
> ------------------------------------- 8< --------------------------------------
> Nqq qrsvavgvbaf sbe CSREE_*_OVG (0 sbe cerfrag, 1 sbe jevgr, rgp.) naq
> hfr gur sbyybjvat:
> 
>         vag vaqrk, fznc;
> 
>         /*
>          * Vs PCY < 3, FZNC cebgrpgvbaf ner qvfnoyrq vs RSYNTF.NP = 1.
>          *
>          * Vs PCY = 3, FZNC nccyvrf gb nyy fhcreivfbe-zbqr qngn npprffrf
>          * (gurfr ner vzcyvpvg fhcreivfbe npprffrf) ertneqyrff bs gur inyhr
>          * bs RSYNTF.NP.
>          *
>          * Guvf pbzchgrf (pcy < 3) && (esyntf & K86_RSYNTF_NP), yrnivat
>          * gur erfhyg va K86_RSYNTF_NP.  Jr gura vafreg vg va cynpr
>          * bs gur CSREE_EFIQ_ZNFX ovg; guvf ovg jvyy nyjnlf or mreb va csrp,
>          * ohg vg jvyy or bar va vaqrk vs FZNC purpxf ner orvat bireevqqra.
>          * Vg vf vzcbegnag gb xrrc guvf oenapuyrff.
>          */
>         fznc = (pcy - 3) & (esyntf & K86_RSYNTF_NP);
>         vaqrk =
>            (csrp >> 1) +
>            (fznc >> (K86_RSYNTF_NP_OVG - CSREE_EFIQ_OVG + 1));
> 
>         erghea (zzh->crezvffvbaf[vaqrk] >> cgr_npprff) & 1;
> 
> Gur qverpgvba bs CSREE_EFIQ_ZNFX vf gur bccbfvgr pbzcnerq gb lbhe pbqr.

I find that the above code is unreadable, I tried to translate it and here is the result: (If I made mistakes while this transition, please correct me!)

--------------------------------------------------------------------------------------------------------------------
/*
 * If CPL < 3, SMAP protections are disabled if EFLAGS.AC = 1.
 *
 * If CPL = 3, SMAP applies to all supervisor-mode data accesses
 * (these are implicit supervisor accesses) regardless of the value
 * of EFLAGS.AC.
 *
 * This computes (cpl < 3) && (rflags & X86_EFLAGS_AC), leaving
 * the result in X86_EFLAGS_AC. We then insert it in place of
 * the PFERR_RSVD_MASK bit; this bit will always be zero in pfec,
 * but it will be one in index if SMAP checks are being overridden.
 * It is important to keep this branchless.
 */
smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);
index =
        (pfec >> 1) +
        (smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1));

return (mmu->permissions[index] >> pte_access) & 1;

The direction of PFERR_RSVD_MASK is the opposite compared to your code.
-------------------------------------------------------------------------------------------------------------------

I am a little confused about some points of the above code:
1. "smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);" 
"smap" equals 1 when it is overridden and it is 0 when being enforced. So "index"
will be (pfec >> 1) when SMAP is enforced, but in my understanding of this case, we
should use the index with PFERR_RSVD_MASK bit being 1 in mmu-> permissions[]
to check the fault.
2. " smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1)"
I am not quite understand this line. BTW, I cannot find the definition of "PFERR_RSVD_BIT",
Do you mean PFERR_RSVD_BIT equals 3?

Here is my understanding:
Basically, we can divide the array mmu->permissions[16] into two groups, one with
PFERR_RSVD_BIT being 1 while the other with this bit being 0 like below:

PFERR_RSVD_BIT:	is 0 (group 0)	is 1 (group 1)
				0000		0100
				0001		0100
				0010		0110
				0011		0111
				1000		1100
				1001		1101
				1010		1110
				1011		1111

I think the basic idea is using group 0 to check permission faults when !((cpl - 3) & (rflags & X86_EFLAGS_AC)), that is SMAP is overridden
while using group 1 to check faults when (cpl - 3) & (rflags & X86_EFLAGS_AC), that is SMAP is enforced.

Here is the code base on your proposal in my understanding:

-------------------------------------------------------------------------------------------------------------------
smap = !((cpl - 3) & (rflags & X86_EFLAGS_AC));
index =
        (pfec >> 1) + (smap << (PFERR_RSVD_BIT - 1)); /*assuming PFERR_RSVD_BIT == 3*/

return (mmu->permissions[index] >> pte_access) & 1;
-------------------------------------------------------------------------------------------------------------------

Could you please have a look at it? Appreciate your help! :)

Thanks,
Feng

Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4

[Paolo Bonzini]

Il 31/03/2014 08:16, Wu, Feng ha scritto:
> --------------------------------------------------------------------------------------------------------------------
> /*
>  * If CPL < 3, SMAP protections are disabled if EFLAGS.AC = 1.
>  *
>  * If CPL = 3, SMAP applies to all supervisor-mode data accesses
>  * (these are implicit supervisor accesses) regardless of the value
>  * of EFLAGS.AC.
>  *
>  * This computes (cpl < 3) && (rflags & X86_EFLAGS_AC), leaving
>  * the result in X86_EFLAGS_AC. We then insert it in place of
>  * the PFERR_RSVD_MASK bit; this bit will always be zero in pfec,
>  * but it will be one in index if SMAP checks are being overridden.
>  * It is important to keep this branchless.
>  */
> smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);
> index =
>         (pfec >> 1) +
>         (smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1));
>
> return (mmu->permissions[index] >> pte_access) & 1;
>
> The direction of PFERR_RSVD_MASK is the opposite compared to your code.
> -------------------------------------------------------------------------------------------------------------------

Correct.

> I am a little confused about some points of the above code:
> 1. "smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);"
> "smap" equals 1 when it is overridden and it is 0 when being enforced.

Actually, smap equals X86_EFLAGS_AC when it is overridden.  Perhaps this 
is the source of the confusion.  Note that I'm using &, not &&.

> So "index"
> will be (pfec >> 1) when SMAP is enforced, but in my understanding of this case, we
> should use the index with PFERR_RSVD_MASK bit being 1 in mmu-> permissions[]
> to check the fault.
> 2. " smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1)"
> I am not quite understand this line. BTW, I cannot find the definition of "PFERR_RSVD_BIT",
> Do you mean PFERR_RSVD_BIT equals 3?

Yes.  You can similarly add PFERR_PRESENT_BIT (equal to 0) etc.

> I think the basic idea is using group 0 to check permission faults when !((cpl - 3) & (rflags & X86_EFLAGS_AC)), that is SMAP is overridden
> while using group 1 to check faults when (cpl - 3) & (rflags & X86_EFLAGS_AC), that is SMAP is enforced.
>
> Here is the code base on your proposal in my understanding:
>
> -------------------------------------------------------------------------------------------------------------------
> smap = !((cpl - 3) & (rflags & X86_EFLAGS_AC));
> index =
>         (pfec >> 1) + (smap << (PFERR_RSVD_BIT - 1)); /*assuming PFERR_RSVD_BIT == 3*/
>
> return (mmu->permissions[index] >> pte_access) & 1;
> -------------------------------------------------------------------------------------------------------------------
>
> Could you please have a look at it? Appreciate your help! :)

It is faster if you avoid the "!" and shift right from the AC bit into 
position PFERR_RSVD_BIT - 1.  In update_permission_bitmask you can 
invert the direction of the bit when you extract it from pfec.

Paolo

RE: [PATCH 2/4] KVM: Add SMAP support when setting CR4

[Wu, Feng]

> -----Original Message-----
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Monday, March 31, 2014 3:29 PM
> To: Wu, Feng; gleb@redhat.com; hpa@zytor.com; kvm@vger.kernel.org
> Subject: Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4
> 
> Il 31/03/2014 08:16, Wu, Feng ha scritto:
> >
> ----------------------------------------------------------------------------------------------------------------
> ----
> > /*
> >  * If CPL < 3, SMAP protections are disabled if EFLAGS.AC = 1.
> >  *
> >  * If CPL = 3, SMAP applies to all supervisor-mode data accesses
> >  * (these are implicit supervisor accesses) regardless of the value
> >  * of EFLAGS.AC.
> >  *
> >  * This computes (cpl < 3) && (rflags & X86_EFLAGS_AC), leaving
> >  * the result in X86_EFLAGS_AC. We then insert it in place of
> >  * the PFERR_RSVD_MASK bit; this bit will always be zero in pfec,
> >  * but it will be one in index if SMAP checks are being overridden.
> >  * It is important to keep this branchless.
> >  */
> > smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);
> > index =
> >         (pfec >> 1) +
> >         (smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1));
> >
> > return (mmu->permissions[index] >> pte_access) & 1;
> >
> > The direction of PFERR_RSVD_MASK is the opposite compared to your code.
> >
> ----------------------------------------------------------------------------------------------------------------
> ---
> 
> Correct.
> 
> > I am a little confused about some points of the above code:
> > 1. "smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);"
> > "smap" equals 1 when it is overridden and it is 0 when being enforced.
> 
> Actually, smap equals X86_EFLAGS_AC when it is overridden.  Perhaps this
> is the source of the confusion.  Note that I'm using &, not &&.
> 

Thanks for your explanation, you are right I didn't notice *&* just now, I was thinking
it is a *&&*, sorry for making the confusion.

> > So "index"
> > will be (pfec >> 1) when SMAP is enforced, but in my understanding of this
> case, we
> > should use the index with PFERR_RSVD_MASK bit being 1 in mmu->
> permissions[]
> > to check the fault.
> > 2. " smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1)"
> > I am not quite understand this line. BTW, I cannot find the definition of
> "PFERR_RSVD_BIT",
> > Do you mean PFERR_RSVD_BIT equals 3?
> 
> Yes.  You can similarly add PFERR_PRESENT_BIT (equal to 0) etc.
> 
> > I think the basic idea is using group 0 to check permission faults when !((cpl -
> 3) & (rflags & X86_EFLAGS_AC)), that is SMAP is overridden
> > while using group 1 to check faults when (cpl - 3) & (rflags & X86_EFLAGS_AC),
> that is SMAP is enforced.
> >
> > Here is the code base on your proposal in my understanding:
> >
> >
> ----------------------------------------------------------------------------------------------------------------
> ---
> > smap = !((cpl - 3) & (rflags & X86_EFLAGS_AC));
> > index =
> >         (pfec >> 1) + (smap << (PFERR_RSVD_BIT - 1)); /*assuming
> PFERR_RSVD_BIT == 3*/
> >
> > return (mmu->permissions[index] >> pte_access) & 1;
> >
> ----------------------------------------------------------------------------------------------------------------
> ---
> >
> > Could you please have a look at it? Appreciate your help! :)
> 
> It is faster if you avoid the "!" and shift right from the AC bit into
> position PFERR_RSVD_BIT - 1.  In update_permission_bitmask you can
> invert the direction of the bit when you extract it from pfec.

So in that case, we should set "smapf" in update_permission_bitmask() this way, right?

smapf = !(pfec & PFERR_RSVD_MASK);

> 
> Paolo

Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4

[Paolo Bonzini]

Il 31/03/2014 10:06, Wu, Feng ha scritto:
>> >
>> > It is faster if you avoid the "!" and shift right from the AC bit into
>> > position PFERR_RSVD_BIT - 1.  In update_permission_bitmask you can
>> > invert the direction of the bit when you extract it from pfec.
> So in that case, we should set "smapf" in update_permission_bitmask() this way, right?
>
> smapf = !(pfec & PFERR_RSVD_MASK);
>

Yep.

Paolo