* Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4
2014-03-28 17:36 ` [PATCH 2/4] KVM: Add SMAP support when setting CR4 Feng Wu
@ 2014-03-28 12:03 ` Paolo Bonzini
2014-03-28 14:03 ` Wu, Feng
2014-03-31 6:16 ` Wu, Feng
0 siblings, 2 replies; 14+ messages in thread
From: Paolo Bonzini @ 2014-03-28 12:03 UTC (permalink / raw)
To: Feng Wu, gleb, hpa, kvm
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 2/4] KVM: Add SMAP support when setting CR4
2014-03-28 12:03 ` Paolo Bonzini
@ 2014-03-28 14:03 ` Wu, Feng
2014-03-31 6:16 ` Wu, Feng
1 sibling, 0 replies; 14+ messages in thread
From: Wu, Feng @ 2014-03-28 14:03 UTC (permalink / raw)
To: Paolo Bonzini, gleb, hpa, kvm
> -----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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 0/4] KVM: enable Intel SMAP for KVM
@ 2014-03-28 17:36 Feng Wu
2014-03-28 17:36 ` [PATCH 1/4] KVM: Remove SMAP bit from CR4_RESERVED_BITS Feng Wu
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Feng Wu @ 2014-03-28 17:36 UTC (permalink / raw)
To: pbonzini, gleb, hpa, kvm; +Cc: 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/4] KVM: Remove SMAP bit from CR4_RESERVED_BITS.
2014-03-28 17:36 [PATCH 0/4] KVM: enable Intel SMAP for KVM Feng Wu
@ 2014-03-28 17:36 ` Feng Wu
2014-03-28 17:36 ` [PATCH 2/4] KVM: Add SMAP support when setting CR4 Feng Wu
` (2 subsequent siblings)
3 siblings, 0 replies; 14+ messages in thread
From: Feng Wu @ 2014-03-28 17:36 UTC (permalink / raw)
To: pbonzini, gleb, hpa, kvm; +Cc: 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/4] KVM: Add SMAP support when setting CR4
2014-03-28 17:36 [PATCH 0/4] KVM: enable Intel SMAP for KVM Feng Wu
2014-03-28 17:36 ` [PATCH 1/4] KVM: Remove SMAP bit from CR4_RESERVED_BITS Feng Wu
@ 2014-03-28 17:36 ` Feng Wu
2014-03-28 12:03 ` Paolo Bonzini
2014-03-28 17:36 ` [PATCH 3/4] KVM: Disable SMAP for guests in EPT realmode and EPT unpaging mode Feng Wu
2014-03-28 17:36 ` [PATCH 4/4] KVM: expose SMAP feature to guest Feng Wu
3 siblings, 1 reply; 14+ messages in thread
From: Feng Wu @ 2014-03-28 17:36 UTC (permalink / raw)
To: pbonzini, gleb, hpa, kvm; +Cc: 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/4] KVM: Disable SMAP for guests in EPT realmode and EPT unpaging mode
2014-03-28 17:36 [PATCH 0/4] KVM: enable Intel SMAP for KVM Feng Wu
2014-03-28 17:36 ` [PATCH 1/4] KVM: Remove SMAP bit from CR4_RESERVED_BITS Feng Wu
2014-03-28 17:36 ` [PATCH 2/4] KVM: Add SMAP support when setting CR4 Feng Wu
@ 2014-03-28 17:36 ` Feng Wu
2014-03-28 17:36 ` [PATCH 4/4] KVM: expose SMAP feature to guest Feng Wu
3 siblings, 0 replies; 14+ messages in thread
From: Feng Wu @ 2014-03-28 17:36 UTC (permalink / raw)
To: pbonzini, gleb, hpa, kvm; +Cc: 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/4] KVM: expose SMAP feature to guest
2014-03-28 17:36 [PATCH 0/4] KVM: enable Intel SMAP for KVM Feng Wu
` (2 preceding siblings ...)
2014-03-28 17:36 ` [PATCH 3/4] KVM: Disable SMAP for guests in EPT realmode and EPT unpaging mode Feng Wu
@ 2014-03-28 17:36 ` Feng Wu
3 siblings, 0 replies; 14+ messages in thread
From: Feng Wu @ 2014-03-28 17:36 UTC (permalink / raw)
To: pbonzini, gleb, hpa, kvm; +Cc: 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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* RE: [PATCH 2/4] KVM: Add SMAP support when setting CR4
2014-03-28 12:03 ` Paolo Bonzini
2014-03-28 14:03 ` Wu, Feng
@ 2014-03-31 6:16 ` Wu, Feng
2014-03-31 7:28 ` Paolo Bonzini
1 sibling, 1 reply; 14+ messages in thread
From: Wu, Feng @ 2014-03-31 6:16 UTC (permalink / raw)
To: Paolo Bonzini, gleb, hpa, kvm
> -----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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4
2014-03-31 6:16 ` Wu, Feng
@ 2014-03-31 7:28 ` Paolo Bonzini
2014-03-31 8:06 ` Wu, Feng
0 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2014-03-31 7:28 UTC (permalink / raw)
To: Wu, Feng, gleb, hpa, kvm
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH 2/4] KVM: Add SMAP support when setting CR4
2014-03-31 7:28 ` Paolo Bonzini
@ 2014-03-31 8:06 ` Wu, Feng
2014-03-31 8:45 ` Paolo Bonzini
0 siblings, 1 reply; 14+ messages in thread
From: Wu, Feng @ 2014-03-31 8:06 UTC (permalink / raw)
To: Paolo Bonzini, gleb, hpa, kvm
> -----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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] KVM: Add SMAP support when setting CR4
2014-03-31 8:06 ` Wu, Feng
@ 2014-03-31 8:45 ` Paolo Bonzini
0 siblings, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2014-03-31 8:45 UTC (permalink / raw)
To: Wu, Feng, gleb, hpa, kvm
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/4] KVM: enable Intel SMAP for KVM
2014-03-27 11:50 ` Paolo Bonzini
@ 2014-03-27 17:52 ` H. Peter Anvin
0 siblings, 0 replies; 14+ messages in thread
From: H. Peter Anvin @ 2014-03-27 17:52 UTC (permalink / raw)
To: Paolo Bonzini, Feng Wu, gleb, kvm
On 03/27/2014 04:50 AM, Paolo Bonzini wrote:
>
> You also need a matching QEMU patch to enable SMAP on Haswell CPUs (do
> all Haswells have SMAP?), though only for a 2.1 or newer machine type.
> But this can be covered later.
>
Haswell does not have SMAP (Ivy Bridge and Haswell do have SMEP, however.)
-hpa
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 0/4] KVM: enable Intel SMAP for KVM
@ 2014-03-27 12:25 Feng Wu
2014-03-27 11:50 ` Paolo Bonzini
0 siblings, 1 reply; 14+ messages in thread
From: Feng Wu @ 2014-03-27 12:25 UTC (permalink / raw)
To: pbonzini, gleb, hpa, kvm; +Cc: 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.
Feng Wu (4):
KVM: expose SMAP feature to guest
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
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 | 22 +++++++++++++++++++---
arch/x86/kvm/mmu.h | 2 ++
arch/x86/kvm/vmx.c | 10 ++++++----
arch/x86/kvm/x86.c | 6 ++++++
7 files changed, 43 insertions(+), 9 deletions(-)
--
1.8.3.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/4] KVM: enable Intel SMAP for KVM
2014-03-27 12:25 [PATCH 0/4] KVM: enable Intel SMAP for KVM Feng Wu
@ 2014-03-27 11:50 ` Paolo Bonzini
2014-03-27 17:52 ` H. Peter Anvin
0 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2014-03-27 11:50 UTC (permalink / raw)
To: Feng Wu, gleb, hpa, kvm
Il 27/03/2014 13:25, Feng Wu ha scritto:
> 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.
There is a problem in patch 3, to which I replied separately.
Also, patch 1 should be last in the series.
You also need a matching QEMU patch to enable SMAP on Haswell CPUs (do
all Haswells have SMAP?), though only for a 2.1 or newer machine type.
But this can be covered later.
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2014-03-31 8:45 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-28 17:36 [PATCH 0/4] KVM: enable Intel SMAP for KVM Feng Wu
2014-03-28 17:36 ` [PATCH 1/4] KVM: Remove SMAP bit from CR4_RESERVED_BITS Feng Wu
2014-03-28 17:36 ` [PATCH 2/4] KVM: Add SMAP support when setting CR4 Feng Wu
2014-03-28 12:03 ` Paolo Bonzini
2014-03-28 14:03 ` Wu, Feng
2014-03-31 6:16 ` Wu, Feng
2014-03-31 7:28 ` Paolo Bonzini
2014-03-31 8:06 ` Wu, Feng
2014-03-31 8:45 ` Paolo Bonzini
2014-03-28 17:36 ` [PATCH 3/4] KVM: Disable SMAP for guests in EPT realmode and EPT unpaging mode Feng Wu
2014-03-28 17:36 ` [PATCH 4/4] KVM: expose SMAP feature to guest Feng Wu
-- strict thread matches above, loose matches on Subject: below --
2014-03-27 12:25 [PATCH 0/4] KVM: enable Intel SMAP for KVM Feng Wu
2014-03-27 11:50 ` Paolo Bonzini
2014-03-27 17:52 ` H. Peter Anvin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.