* 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
2014-03-27 12:25 ` [PATCH 1/4] KVM: expose SMAP feature to guest Feng Wu
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ 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] 16+ 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; 16+ 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] 16+ messages in thread
* [PATCH 1/4] KVM: expose SMAP feature to guest
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 12:25 ` Feng Wu
2014-03-27 12:25 ` [PATCH 2/4] KVM: Remove SMAP bit from CR4_RESERVED_BITS Feng Wu
` (2 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Feng Wu @ 2014-03-27 12:25 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] 16+ messages in thread* [PATCH 2/4] KVM: Remove SMAP bit from CR4_RESERVED_BITS.
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 12:25 ` [PATCH 1/4] KVM: expose SMAP feature to guest Feng Wu
@ 2014-03-27 12:25 ` Feng Wu
2014-03-27 12:25 ` [PATCH 3/4] KVM: Add SMAP support when setting CR4 Feng Wu
2014-03-27 12:25 ` [PATCH 4/4] KVM: Disable SMAP for guests in EPT realmode and EPT unpaging mode Feng Wu
4 siblings, 0 replies; 16+ messages in thread
From: Feng Wu @ 2014-03-27 12:25 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 ae5d783..b673925 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] 16+ messages in thread
* [PATCH 3/4] KVM: Add SMAP support when setting CR4
2014-03-27 12:25 [PATCH 0/4] KVM: enable Intel SMAP for KVM Feng Wu
` (2 preceding siblings ...)
2014-03-27 12:25 ` [PATCH 2/4] KVM: Remove SMAP bit from CR4_RESERVED_BITS Feng Wu
@ 2014-03-27 12:25 ` Feng Wu
2014-03-27 11:46 ` Paolo Bonzini
2014-03-27 12:25 ` [PATCH 4/4] KVM: Disable SMAP for guests in EPT realmode and EPT unpaging mode Feng Wu
4 siblings, 1 reply; 16+ messages in thread
From: Feng Wu @ 2014-03-27 12:25 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 | 22 +++++++++++++++++++---
arch/x86/kvm/mmu.h | 2 ++
arch/x86/kvm/x86.c | 6 ++++++
4 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index f1e4895..63124a2 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 40772ef..33e656c 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3591,14 +3591,15 @@ 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, 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;
@@ -3617,11 +3618,26 @@ 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
+ * - !(CPL<3 && X86_EFLAGS_AC is set)
+ * - Page fault in kernel mode
+ */
+ smap = smap && u && !uf &&
+ !((kvm_x86_ops->get_cpl(vcpu) < 3) &&
+ ((kvm_x86_ops->get_rflags(vcpu) &
+ X86_EFLAGS_AC) == 1));
} 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..8820f78 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)
{
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4e33b85..f8293fb 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -630,6 +630,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;
@@ -658,6 +661,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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 3/4] KVM: Add SMAP support when setting CR4
2014-03-27 12:25 ` [PATCH 3/4] KVM: Add SMAP support when setting CR4 Feng Wu
@ 2014-03-27 11:46 ` Paolo Bonzini
2014-03-28 5:47 ` Zhang, Yang Z
2014-03-28 9:35 ` Wu, Feng
0 siblings, 2 replies; 16+ messages in thread
From: Paolo Bonzini @ 2014-03-27 11:46 UTC (permalink / raw)
To: Feng Wu, gleb, hpa, kvm
Il 27/03/2014 13:25, Feng Wu ha scritto:
> +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, 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;
> @@ -3617,11 +3618,26 @@ 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
> + * - !(CPL<3 && X86_EFLAGS_AC is set)
> + * - Page fault in kernel mode
> + */
> + smap = smap && u && !uf &&
> + !((kvm_x86_ops->get_cpl(vcpu) < 3) &&
> + ((kvm_x86_ops->get_rflags(vcpu) &
> + X86_EFLAGS_AC) == 1));
Unfortunately this doesn't work.
The reason is that changing X86_EFLAGS_AC doesn't trigger
update_permission_bitmask. So the value of CPL < 3 && AC = 1 must not
be checked in update_permission_bitmask; instead, it must be included in
the index into the permissions array. You can reuse the PFERR_RSVD_MASK
bit, like
smapf = pfec & PFERR_RSVD_MASK;
...
smap = smap && smapf u && !uf;
The VCPU can then be passed to permission_fault in order to get the
value of the CPL and the AC bit.
Please test nested virtualization too. I think PFERR_RSVD_MASK should
be removed in translate_nested_gpa.
Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread* RE: [PATCH 3/4] KVM: Add SMAP support when setting CR4
2014-03-27 11:46 ` Paolo Bonzini
@ 2014-03-28 5:47 ` Zhang, Yang Z
2014-03-28 6:23 ` Paolo Bonzini
2014-03-28 9:35 ` Wu, Feng
1 sibling, 1 reply; 16+ messages in thread
From: Zhang, Yang Z @ 2014-03-28 5:47 UTC (permalink / raw)
To: Paolo Bonzini, Wu, Feng, gleb, hpa, kvm
Paolo Bonzini wrote on 2014-03-27:
> Il 27/03/2014 13:25, Feng Wu ha scritto:
>> +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, 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;
>> @@ -3617,11 +3618,26 @@ 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
>> + * - !(CPL<3 && X86_EFLAGS_AC is set)
>> + * - Page fault in kernel mode
>> + */
>> + smap = smap && u && !uf &&
>> + !((kvm_x86_ops->get_cpl(vcpu) < 3) &&
>> + ((kvm_x86_ops->get_rflags(vcpu) &
>> + X86_EFLAGS_AC) == 1));
>
> Unfortunately this doesn't work.
>
> The reason is that changing X86_EFLAGS_AC doesn't trigger
> update_permission_bitmask. So the value of CPL < 3 && AC = 1 must not
> be checked in update_permission_bitmask; instead, it must be included
> in the index into the permissions array. You can reuse the
> PFERR_RSVD_MASK bit, like
>
> smapf = pfec & PFERR_RSVD_MASK;
> ...
> smap = smap && smapf u && !uf;
>
> The VCPU can then be passed to permission_fault in order to get the
> value of the CPL and the AC bit.
Is CPL check needed? Shouldn't it already have been covered by U bit?
>
> Please test nested virtualization too. I think PFERR_RSVD_MASK should
> be removed in translate_nested_gpa.
>
> Paolo
Best regards,
Yang
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 3/4] KVM: Add SMAP support when setting CR4
2014-03-28 5:47 ` Zhang, Yang Z
@ 2014-03-28 6:23 ` Paolo Bonzini
2014-03-28 7:33 ` Wu, Feng
0 siblings, 1 reply; 16+ messages in thread
From: Paolo Bonzini @ 2014-03-28 6:23 UTC (permalink / raw)
To: Zhang, Yang Z, Wu, Feng, gleb, hpa, kvm
Il 28/03/2014 06:47, Zhang, Yang Z ha scritto:
>>> >> + smap = smap && u && !uf &&
>>> >> + !((kvm_x86_ops->get_cpl(vcpu) < 3) &&
>>> >> + ((kvm_x86_ops->get_rflags(vcpu) &
>>> >> + X86_EFLAGS_AC) == 1));
>> >
>> > Unfortunately this doesn't work.
>> >
>> > The reason is that changing X86_EFLAGS_AC doesn't trigger
>> > update_permission_bitmask. So the value of CPL < 3 && AC = 1 must not
>> > be checked in update_permission_bitmask; instead, it must be included
>> > in the index into the permissions array. You can reuse the
>> > PFERR_RSVD_MASK bit, like
>> >
>> > smapf = pfec & PFERR_RSVD_MASK;
>> > ...
>> > smap = smap && smapf u && !uf;
>> >
>> > The VCPU can then be passed to permission_fault in order to get the
>> > value of the CPL and the AC bit.
>
> Is CPL check needed? Shouldn't it already have been covered by U bit?
It is not needed but actually it is covered by !uf, I think.
Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 3/4] KVM: Add SMAP support when setting CR4
2014-03-28 6:23 ` Paolo Bonzini
@ 2014-03-28 7:33 ` Wu, Feng
2014-03-28 14:09 ` Paolo Bonzini
0 siblings, 1 reply; 16+ messages in thread
From: Wu, Feng @ 2014-03-28 7:33 UTC (permalink / raw)
To: Paolo Bonzini, Zhang, Yang Z, gleb, hpa, kvm
> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
> Behalf Of Paolo Bonzini
> Sent: Friday, March 28, 2014 2:23 PM
> To: Zhang, Yang Z; Wu, Feng; gleb@redhat.com; hpa@zytor.com;
> kvm@vger.kernel.org
> Subject: Re: [PATCH 3/4] KVM: Add SMAP support when setting CR4
>
> Il 28/03/2014 06:47, Zhang, Yang Z ha scritto:
> >>> >> + smap = smap && u && !uf &&
> >>> >> + !((kvm_x86_ops->get_cpl(vcpu) < 3) &&
> >>> >> + ((kvm_x86_ops->get_rflags(vcpu) &
> >>> >> + X86_EFLAGS_AC) == 1));
> >> >
> >> > Unfortunately this doesn't work.
> >> >
> >> > The reason is that changing X86_EFLAGS_AC doesn't trigger
> >> > update_permission_bitmask. So the value of CPL < 3 && AC = 1 must not
> >> > be checked in update_permission_bitmask; instead, it must be included
> >> > in the index into the permissions array. You can reuse the
> >> > PFERR_RSVD_MASK bit, like
> >> >
> >> > smapf = pfec & PFERR_RSVD_MASK;
> >> > ...
> >> > smap = smap && smapf u && !uf;
> >> >
> >> > The VCPU can then be passed to permission_fault in order to get the
> >> > value of the CPL and the AC bit.
> >
> > Is CPL check needed? Shouldn't it already have been covered by U bit?
>
> It is not needed but actually it is covered by !uf, I think.
In my understanding it is needed, from Intel SDM:
"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."
>From the above SDM, we can see supervisor-mode access can also
happen when CPL equals 3.
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 when we check the value of EFLAGS.AC, we also need to check CPL, since AC
bit only takes effect when CPL<3.
U==1 means user-mode access are allowed, while !uf means it is a fault
from Supervisor-mode access, I think both *u* and *uf* cannot reflect the
value of CPL.
Correct me if I am wrong. Thanks a lot!
>
> Paolo
> --
> 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
Thanks,
Feng
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/4] KVM: Add SMAP support when setting CR4
2014-03-28 7:33 ` Wu, Feng
@ 2014-03-28 14:09 ` Paolo Bonzini
0 siblings, 0 replies; 16+ messages in thread
From: Paolo Bonzini @ 2014-03-28 14:09 UTC (permalink / raw)
To: Wu, Feng, Zhang, Yang Z, gleb, hpa, kvm
Il 28/03/2014 08:33, Wu, Feng ha scritto:
> In my understanding it is needed, from Intel SDM:
>
> "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."
>
> From the above SDM, we can see supervisor-mode access can also
> happen when CPL equals 3.
>
> 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 when we check the value of EFLAGS.AC, we also need to check CPL, since AC
> bit only takes effect when CPL<3.
>
> U==1 means user-mode access are allowed, while !uf means it is a fault
> from Supervisor-mode access, I think both *u* and *uf* cannot reflect the
> value of CPL.
>
> Correct me if I am wrong. Thanks a lot!
You're right!
Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 3/4] KVM: Add SMAP support when setting CR4
2014-03-27 11:46 ` Paolo Bonzini
2014-03-28 5:47 ` Zhang, Yang Z
@ 2014-03-28 9:35 ` Wu, Feng
1 sibling, 0 replies; 16+ messages in thread
From: Wu, Feng @ 2014-03-28 9:35 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: Thursday, March 27, 2014 7:47 PM
> To: Wu, Feng; gleb@redhat.com; hpa@zytor.com; kvm@vger.kernel.org
> Subject: Re: [PATCH 3/4] KVM: Add SMAP support when setting CR4
>
> Il 27/03/2014 13:25, Feng Wu ha scritto:
> > +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, 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;
> > @@ -3617,11 +3618,26 @@ 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
> > + * - !(CPL<3 && X86_EFLAGS_AC is set)
> > + * - Page fault in kernel mode
> > + */
> > + smap = smap && u && !uf &&
> > + !((kvm_x86_ops->get_cpl(vcpu) < 3) &&
> > + ((kvm_x86_ops->get_rflags(vcpu) &
> > + X86_EFLAGS_AC) == 1));
>
> Unfortunately this doesn't work.
>
> The reason is that changing X86_EFLAGS_AC doesn't trigger
> update_permission_bitmask. So the value of CPL < 3 && AC = 1 must not
> be checked in update_permission_bitmask; instead, it must be included in
> the index into the permissions array. You can reuse the PFERR_RSVD_MASK
> bit, like
>
> smapf = pfec & PFERR_RSVD_MASK;
> ...
> smap = smap && smapf u && !uf;
>
> The VCPU can then be passed to permission_fault in order to get the
> value of the CPL and the AC bit.
>
> Please test nested virtualization too. I think PFERR_RSVD_MASK should
> be removed in translate_nested_gpa.
Thanks very much for your comments, I changed the code according to it and
the patch v2 will be sent out for a review. Since setting up the environment for
nested virtualization is a little time consuming and it is in progress now,
could you please have a look at it to see if it is what you expected first?
Any comments are appreciated!
>
> Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 4/4] KVM: Disable SMAP for guests in EPT realmode and EPT unpaging mode
2014-03-27 12:25 [PATCH 0/4] KVM: enable Intel SMAP for KVM Feng Wu
` (3 preceding siblings ...)
2014-03-27 12:25 ` [PATCH 3/4] KVM: Add SMAP support when setting CR4 Feng Wu
@ 2014-03-27 12:25 ` Feng Wu
2014-03-27 16:14 ` Jan Kiszka
4 siblings, 1 reply; 16+ messages in thread
From: Feng Wu @ 2014-03-27 12:25 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 | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index dcc4de3..1d37e50 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3421,13 +3421,15 @@ 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_SMAP;
} else if (!(cr4 & X86_CR4_PAE)) {
hw_cr4 &= ~X86_CR4_PAE;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH 4/4] KVM: Disable SMAP for guests in EPT realmode and EPT unpaging mode
2014-03-27 12:25 ` [PATCH 4/4] KVM: Disable SMAP for guests in EPT realmode and EPT unpaging mode Feng Wu
@ 2014-03-27 16:14 ` Jan Kiszka
2014-03-28 0:41 ` Wu, Feng
0 siblings, 1 reply; 16+ messages in thread
From: Jan Kiszka @ 2014-03-27 16:14 UTC (permalink / raw)
To: Feng Wu, pbonzini, gleb, hpa, kvm
[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]
On 2014-03-27 13:25, Feng Wu wrote:
> 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 | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index dcc4de3..1d37e50 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -3421,13 +3421,15 @@ 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_SMAP;
Why not
hw_cr4 &= ~(X86_CR4_SMEP | X86_CR4_SMAP);
?
Jan
> } else if (!(cr4 & X86_CR4_PAE)) {
> hw_cr4 &= ~X86_CR4_PAE;
> }
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread* RE: [PATCH 4/4] KVM: Disable SMAP for guests in EPT realmode and EPT unpaging mode
2014-03-27 16:14 ` Jan Kiszka
@ 2014-03-28 0:41 ` Wu, Feng
0 siblings, 0 replies; 16+ messages in thread
From: Wu, Feng @ 2014-03-28 0:41 UTC (permalink / raw)
To: Jan Kiszka, pbonzini, gleb, hpa, kvm
> -----Original Message-----
> From: kvm-owner@vger.kernel.org [mailto:kvm-owner@vger.kernel.org] On
> Behalf Of Jan Kiszka
> Sent: Friday, March 28, 2014 12:15 AM
> To: Wu, Feng; pbonzini@redhat.com; gleb@redhat.com; hpa@zytor.com;
> kvm@vger.kernel.org
> Subject: Re: [PATCH 4/4] KVM: Disable SMAP for guests in EPT realmode and
> EPT unpaging mode
>
> On 2014-03-27 13:25, Feng Wu wrote:
> > 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 | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index dcc4de3..1d37e50 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -3421,13 +3421,15 @@ 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_SMAP;
>
> Why not
>
> hw_cr4 &= ~(X86_CR4_SMEP | X86_CR4_SMAP);
>
> ?
Sure, your suggestion is cleaner, I will change it in the next version, thanks for the comments!
>
> Jan
>
> > } else if (!(cr4 & X86_CR4_PAE)) {
> > hw_cr4 &= ~X86_CR4_PAE;
> > }
> >
>
Thanks,
Feng
^ permalink raw reply [flat|nested] 16+ messages in thread