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. Paolo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/4] KVM: Add SMAP support when setting CR4
-Original Message- From: kvm-ow...@vger.kernel.org [mailto:kvm-ow...@vger.kernel.org] On Behalf Of Paolo Bonzini Sent: Friday, March 28, 2014 2:23 PM To: Zhang, Yang Z; Wu, Feng; g...@redhat.com; h...@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 CPL3. 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Thanks, Feng -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/4] KVM: Add SMAP support when setting CR4
-Original Message-
From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo
Bonzini
Sent: Thursday, March 27, 2014 7:47 PM
To: Wu, Feng; g...@redhat.com; h...@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
+* - !(CPL3 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
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] KVM: Add SMAP support when setting CR4
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 CPL3. 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 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
+* - !(CPL3 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
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 3/4] KVM: Add SMAP support when setting CR4
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
+ * - !(CPL3 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
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
