[Patch v3] Enable CPU SMEP feature for KVM

[Patch v3] Enable CPU SMEP feature for KVM

[Yang, Wei Y]

This patchset enables a new CPU feature SMEP (Supervisor Mode Execution
Protection) in KVM. SMEP prevents kernel from executing code in application.
Updated Intel SDM describes this CPU feature. The document will be published soon.

This patchset is based on Fenghua's SMEP patch series, as referred by:
https://lkml.org/lkml/2011/5/17/523

Changes since v2: enable SMEP for spt mode.

 Signed-off-by: Yang Wei <wei.y.yang@intel.com>
 Signed-off-by: Shan Haitao <haitao.shan@intel.com>

---
 arch/x86/include/asm/kvm_host.h |    1 +
 arch/x86/kvm/paging_tmpl.h      |   15 +++++++++++++--
 arch/x86/kvm/vmx.c              |    9 +++++++++
 arch/x86/kvm/x86.c              |    7 +++++--
 4 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index d2ac8e2..154287b 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -306,6 +306,7 @@ struct kvm_vcpu_arch {
 	unsigned long cr3;
 	unsigned long cr4;
 	unsigned long cr4_guest_owned_bits;
+	unsigned long cr4_reserved_bits;
 	unsigned long cr8;
 	u32 hflags;
 	u64 efer;
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 6c4dc01..7e0b2f8 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -120,7 +120,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
 				    struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 				    gva_t addr, u32 access)
 {
-	pt_element_t pte;
+	pt_element_t pte, pte_smep;
 	pt_element_t __user *ptep_user;
 	gfn_t table_gfn;
 	unsigned index, pt_access, uninitialized_var(pte_access);
@@ -150,7 +150,11 @@ walk:
 		}
 		--walker->level;
 	}
+	pte_smep = ~0ULL;
+#else
+	pte_smep = ~0U;
 #endif
+
 	ASSERT((!is_long_mode(vcpu) && is_pae(vcpu)) ||
 	       (mmu->get_cr3(vcpu) & CR3_NONPAE_RESERVED_BITS) == 0);
 
@@ -234,6 +238,8 @@ walk:
 
 		walker->ptes[walker->level - 1] = pte;
 
+		pte_smep &= pte;
+
 		if ((walker->level == PT_PAGE_TABLE_LEVEL) ||
 		    ((walker->level == PT_DIRECTORY_LEVEL) &&
 				is_large_pte(pte) &&
@@ -246,6 +252,11 @@ walk:
 			gfn_t gfn;
 			u32 ac;
 
+			if (unlikely(fetch_fault && !user_fault))
+				if ((vcpu->arch.cr4 & X86_CR4_SMEP)
+						&& (pte_smep & PT_USER_MASK))
+					eperm = true;
+
 			gfn = gpte_to_gfn_lvl(pte, lvl);
 			gfn += (addr & PT_LVL_OFFSET_MASK(lvl)) >> PAGE_SHIFT;
 
@@ -305,7 +316,7 @@ error:
 
 	walker->fault.error_code |= write_fault | user_fault;
 
-	if (fetch_fault && mmu->nx)
+	if (fetch_fault && (mmu->nx || (vcpu->arch.cr4 & X86_CR4_SMEP)))
 		walker->fault.error_code |= PFERR_FETCH_MASK;
 	if (rsvd_fault)
 		walker->fault.error_code |= PFERR_RSVD_MASK;
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 4c3fa0f..7ad24fd 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -4507,6 +4507,15 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
 			}
 		}
 	}
+
+	best = kvm_find_cpuid_entry(vcpu, 7, 0);
+	if (best && (best->ebx & bit(X86_FEATURE_SMEP))) {
+		if (boot_cpu_has(X86_FEATURE_SMEP))
+			vcpu->arch.cr4_reserved_bits &=
+				~((unsigned long)X86_CR4_SMEP);
+		else
+			best->ebx &= ~(bit(X86_FEATURE_SMEP));
+	}
 }
 
 static void vmx_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 77c9d86..6ead39e 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -598,9 +598,10 @@ static void update_cpuid(struct kvm_vcpu *vcpu)
 int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 {
 	unsigned long old_cr4 = kvm_read_cr4(vcpu);
-	unsigned long pdptr_bits = X86_CR4_PGE | X86_CR4_PSE | X86_CR4_PAE;
+	unsigned long pdptr_bits = X86_CR4_PGE | X86_CR4_PSE
+				 | X86_CR4_PAE | X86_CR4_SMEP;
 
-	if (cr4 & CR4_RESERVED_BITS)
+	if (cr4 & vcpu->arch.cr4_reserved_bits)
 		return 1;
 
 	if (!guest_cpuid_has_xsave(vcpu) && (cr4 & X86_CR4_OSXSAVE))
@@ -6222,6 +6223,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
 
 	kvm_async_pf_hash_reset(vcpu);
 
+	vcpu->arch.cr4_reserved_bits = CR4_RESERVED_BITS;
+
 	return 0;
 fail_free_mce_banks:
 	kfree(vcpu->arch.mce_banks);
-- 
1.7.4.1

RE: [Patch v3] Enable CPU SMEP feature for KVM

[Tian, Kevin]

> From: Yang, Wei Y
> Sent: Thursday, May 26, 2011 9:29 PM
> 
> This patchset enables a new CPU feature SMEP (Supervisor Mode Execution
> Protection) in KVM. SMEP prevents kernel from executing code in application.
> Updated Intel SDM describes this CPU feature. The document will be published
> soon.
> 
> This patchset is based on Fenghua's SMEP patch series, as referred by:
> https://lkml.org/lkml/2011/5/17/523
> 
> Changes since v2: enable SMEP for spt mode.

another change in this version is to avoid adding SMEP to cr4_guest_owned_bits,
because architecturally it's required to flush TLB when CR4.SMEP is changed
which has to be emulated.

Also based on your comment SMEP is now permitted based on cpuid setting. One pending 
issue though is check_cr_write in emulation path. We changed cr4_reserved_bits
to vcpu specific instead of global, but check_cr_write doesn't provide a vcpu parameter.
Looks the whole emulation logic avoids using vcpu context to be neutral. Avi, do you
have any suggestion for a clean change here? Add cr_reserved_bits array into
emulation context instead of hard-coding?

Thanks
Kevin

> 
>  Signed-off-by: Yang Wei <wei.y.yang@intel.com>
>  Signed-off-by: Shan Haitao <haitao.shan@intel.com>
> 
> ---
>  arch/x86/include/asm/kvm_host.h |    1 +
>  arch/x86/kvm/paging_tmpl.h      |   15 +++++++++++++--
>  arch/x86/kvm/vmx.c              |    9 +++++++++
>  arch/x86/kvm/x86.c              |    7 +++++--
>  4 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h
> b/arch/x86/include/asm/kvm_host.h
> index d2ac8e2..154287b 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -306,6 +306,7 @@ struct kvm_vcpu_arch {
>  	unsigned long cr3;
>  	unsigned long cr4;
>  	unsigned long cr4_guest_owned_bits;
> +	unsigned long cr4_reserved_bits;
>  	unsigned long cr8;
>  	u32 hflags;
>  	u64 efer;
> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> index 6c4dc01..7e0b2f8 100644
> --- a/arch/x86/kvm/paging_tmpl.h
> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -120,7 +120,7 @@ static int FNAME(walk_addr_generic)(struct
> guest_walker *walker,
>  				    struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>  				    gva_t addr, u32 access)
>  {
> -	pt_element_t pte;
> +	pt_element_t pte, pte_smep;
>  	pt_element_t __user *ptep_user;
>  	gfn_t table_gfn;
>  	unsigned index, pt_access, uninitialized_var(pte_access);
> @@ -150,7 +150,11 @@ walk:
>  		}
>  		--walker->level;
>  	}
> +	pte_smep = ~0ULL;
> +#else
> +	pte_smep = ~0U;
>  #endif
> +
>  	ASSERT((!is_long_mode(vcpu) && is_pae(vcpu)) ||
>  	       (mmu->get_cr3(vcpu) & CR3_NONPAE_RESERVED_BITS) == 0);
> 
> @@ -234,6 +238,8 @@ walk:
> 
>  		walker->ptes[walker->level - 1] = pte;
> 
> +		pte_smep &= pte;
> +
>  		if ((walker->level == PT_PAGE_TABLE_LEVEL) ||
>  		    ((walker->level == PT_DIRECTORY_LEVEL) &&
>  				is_large_pte(pte) &&
> @@ -246,6 +252,11 @@ walk:
>  			gfn_t gfn;
>  			u32 ac;
> 
> +			if (unlikely(fetch_fault && !user_fault))
> +				if ((vcpu->arch.cr4 & X86_CR4_SMEP)
> +						&& (pte_smep & PT_USER_MASK))
> +					eperm = true;
> +
>  			gfn = gpte_to_gfn_lvl(pte, lvl);
>  			gfn += (addr & PT_LVL_OFFSET_MASK(lvl)) >> PAGE_SHIFT;
> 
> @@ -305,7 +316,7 @@ error:
> 
>  	walker->fault.error_code |= write_fault | user_fault;
> 
> -	if (fetch_fault && mmu->nx)
> +	if (fetch_fault && (mmu->nx || (vcpu->arch.cr4 & X86_CR4_SMEP)))
>  		walker->fault.error_code |= PFERR_FETCH_MASK;
>  	if (rsvd_fault)
>  		walker->fault.error_code |= PFERR_RSVD_MASK;
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 4c3fa0f..7ad24fd 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -4507,6 +4507,15 @@ static void vmx_cpuid_update(struct kvm_vcpu
> *vcpu)
>  			}
>  		}
>  	}
> +
> +	best = kvm_find_cpuid_entry(vcpu, 7, 0);
> +	if (best && (best->ebx & bit(X86_FEATURE_SMEP))) {
> +		if (boot_cpu_has(X86_FEATURE_SMEP))
> +			vcpu->arch.cr4_reserved_bits &=
> +				~((unsigned long)X86_CR4_SMEP);
> +		else
> +			best->ebx &= ~(bit(X86_FEATURE_SMEP));
> +	}
>  }
> 
>  static void vmx_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2
> *entry)
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 77c9d86..6ead39e 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -598,9 +598,10 @@ static void update_cpuid(struct kvm_vcpu *vcpu)
>  int kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
>  {
>  	unsigned long old_cr4 = kvm_read_cr4(vcpu);
> -	unsigned long pdptr_bits = X86_CR4_PGE | X86_CR4_PSE | X86_CR4_PAE;
> +	unsigned long pdptr_bits = X86_CR4_PGE | X86_CR4_PSE
> +				 | X86_CR4_PAE | X86_CR4_SMEP;
> 
> -	if (cr4 & CR4_RESERVED_BITS)
> +	if (cr4 & vcpu->arch.cr4_reserved_bits)
>  		return 1;
> 
>  	if (!guest_cpuid_has_xsave(vcpu) && (cr4 & X86_CR4_OSXSAVE))
> @@ -6222,6 +6223,8 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
> 
>  	kvm_async_pf_hash_reset(vcpu);
> 
> +	vcpu->arch.cr4_reserved_bits = CR4_RESERVED_BITS;
> +
>  	return 0;
>  fail_free_mce_banks:
>  	kfree(vcpu->arch.mce_banks);
> --
> 1.7.4.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Patch v3] Enable CPU SMEP feature for KVM

[Avi Kivity]

On 05/27/2011 05:56 AM, Tian, Kevin wrote:
> >  From: Yang, Wei Y
> >  Sent: Thursday, May 26, 2011 9:29 PM
> >
> >  This patchset enables a new CPU feature SMEP (Supervisor Mode Execution
> >  Protection) in KVM. SMEP prevents kernel from executing code in application.
> >  Updated Intel SDM describes this CPU feature. The document will be published
> >  soon.
> >
> >  This patchset is based on Fenghua's SMEP patch series, as referred by:
> >  https://lkml.org/lkml/2011/5/17/523
> >
> >  Changes since v2: enable SMEP for spt mode.
>
> another change in this version is to avoid adding SMEP to cr4_guest_owned_bits,
> because architecturally it's required to flush TLB when CR4.SMEP is changed
> which has to be emulated.

That is actually a good change since it allows us to query SMEP without 
a vmcs_readl(GUEST_CR4).

> Also based on your comment SMEP is now permitted based on cpuid setting. One pending
> issue though is check_cr_write in emulation path. We changed cr4_reserved_bits
> to vcpu specific instead of global, but check_cr_write doesn't provide a vcpu parameter.
> Looks the whole emulation logic avoids using vcpu context to be neutral. Avi, do you
> have any suggestion for a clean change here? Add cr_reserved_bits array into
> emulation context instead of hard-coding?

We can have a ->cpuid() callback so we can query it dynamically.  cr4 
writes will be emulated very rarely, after all.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Patch v3] Enable CPU SMEP feature for KVM

[Avi Kivity]

On 05/26/2011 04:28 PM, Yang, Wei Y wrote:
> This patchset enables a new CPU feature SMEP (Supervisor Mode Execution
> Protection) in KVM. SMEP prevents kernel from executing code in application.
> Updated Intel SDM describes this CPU feature. The document will be published soon.
>
> This patchset is based on Fenghua's SMEP patch series, as referred by:
> https://lkml.org/lkml/2011/5/17/523
>
>
>
> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> index 6c4dc01..7e0b2f8 100644
> --- a/arch/x86/kvm/paging_tmpl.h
> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -120,7 +120,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
>   				    struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>   				    gva_t addr, u32 access)
>   {
> -	pt_element_t pte;
> +	pt_element_t pte, pte_smep;
>   	pt_element_t __user *ptep_user;
>   	gfn_t table_gfn;
>   	unsigned index, pt_access, uninitialized_var(pte_access);
> @@ -150,7 +150,11 @@ walk:
>   		}
>   		--walker->level;
>   	}
> +	pte_smep = ~0ULL;
> +#else
> +	pte_smep = ~0U;
>   #endif

Use pte_smep = ~(pt_element_t)0;.  But I don't understand why you don't 
use pt_access instead.

>
> +			if (unlikely(fetch_fault&&  !user_fault))

Why are kernel fetch faults unlikely?  Rather, a SMEP fault is unlikely, 
so annotate that instead.

> +				if ((vcpu->arch.cr4&  X86_CR4_SMEP)
> +						&&  (pte_smep&  PT_USER_MASK))
> +					eperm = true;
> +

kvm_read_cr4_bits()

>   			gfn = gpte_to_gfn_lvl(pte, lvl);
>   			gfn += (addr&  PT_LVL_OFFSET_MASK(lvl))>>  PAGE_SHIFT;
>
> @@ -305,7 +316,7 @@ error:
>
>   	walker->fault.error_code |= write_fault | user_fault;
>
> -	if (fetch_fault&&  mmu->nx)
> +	if (fetch_fault&&  (mmu->nx || (vcpu->arch.cr4&  X86_CR4_SMEP)))

Here, too.

> @@ -4507,6 +4507,15 @@ static void vmx_cpuid_update(struct kvm_vcpu *vcpu)
>   			}
>   		}
>   	}
> +
> +	best = kvm_find_cpuid_entry(vcpu, 7, 0);
> +	if (best&&  (best->ebx&  bit(X86_FEATURE_SMEP))) {
> +		if (boot_cpu_has(X86_FEATURE_SMEP))
> +			vcpu->arch.cr4_reserved_bits&=
> +				~((unsigned long)X86_CR4_SMEP);

Fails if cpuid is updated again to include SMEP.  But I suggest to drop 
cr4_reserved_bits completely and just check cpuid directly in 
kvm_set_cr4(), if cr4.smep is changed, which should be very rare.  See 
how XSAVE is supported, for example.

> +		else
> +			best->ebx&= ~(bit(X86_FEATURE_SMEP));

Not needed - x86.c already masks unsupported features.

> +	}
>   }

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

RE: [Patch v3] Enable CPU SMEP feature for KVM

[Li, Xin]

> > +	best = kvm_find_cpuid_entry(vcpu, 7, 0);
> > +	if (best&&  (best->ebx&  bit(X86_FEATURE_SMEP))) {
> > +		if (boot_cpu_has(X86_FEATURE_SMEP))
> > +			vcpu->arch.cr4_reserved_bits&=
> > +				~((unsigned long)X86_CR4_SMEP);
> 
> Fails if cpuid is updated again to include SMEP.  But I suggest to drop
> cr4_reserved_bits completely and just check cpuid directly in
> kvm_set_cr4(), if cr4.smep is changed, which should be very rare.  See
> how XSAVE is supported, for example.
> 
> > +		else
> > +			best->ebx&= ~(bit(X86_FEATURE_SMEP));
> 
> Not needed - x86.c already masks unsupported features.

Should KVM still support guest SMEP when host disables it thru nosmep?
Thanks!
-Xin

Re: [Patch v3] Enable CPU SMEP feature for KVM

[Avi Kivity]

On 05/29/2011 08:16 AM, Li, Xin wrote:
> >
> >  >  +		else
> >  >  +			best->ebx&= ~(bit(X86_FEATURE_SMEP));
> >
> >  Not needed - x86.c already masks unsupported features.
>
> Should KVM still support guest SMEP when host disables it thru nosmep?

No.  We treat nosmep as "no smep for the kernel and any guests".  NX is 
treated in the same way.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

RE: [Patch v3] Enable CPU SMEP feature for KVM

[Li, Xin]

> > >
> > >  >  +		else
> > >  >  +			best->ebx&= ~(bit(X86_FEATURE_SMEP));
> > >
> > >  Not needed - x86.c already masks unsupported features.
> >
> > Should KVM still support guest SMEP when host disables it thru nosmep?
> 
> No.  We treat nosmep as "no smep for the kernel and any guests".  NX is
> treated in the same way.

Then we need to add logic to do_cpuid_ent to mask off leaf 7 features when host
disables it.
Thanks!
-Xin

Re: [Patch v3] Enable CPU SMEP feature for KVM

[Avi Kivity]

On 05/29/2011 10:15 AM, Li, Xin wrote:
> >  >  >
> >  >  >   >   +		else
> >  >  >   >   +			best->ebx&= ~(bit(X86_FEATURE_SMEP));
> >  >  >
> >  >  >   Not needed - x86.c already masks unsupported features.
> >  >
> >  >  Should KVM still support guest SMEP when host disables it thru nosmep?
> >
> >  No.  We treat nosmep as "no smep for the kernel and any guests".  NX is
> >  treated in the same way.
>
> Then we need to add logic to do_cpuid_ent to mask off leaf 7 features when host
> disables it.

Yes.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.