* [PATCH 0/4 v3] KVM: nSVM: Add checks for CR3 and CR4 reserved bits to svm_set_nested_state() and test CR3 non-MBZ reserved bits
@ 2020-10-06 19:06 Krish Sadhukhan
2020-10-06 19:06 ` [PATCH 1/4 v3] KVM: nSVM: CR3 MBZ bits are only 63:52 Krish Sadhukhan
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: Krish Sadhukhan @ 2020-10-06 19:06 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, sean.j.christopherson
v2 -> v3:
Patch# 2: The local variable "nested_vmcb_lma" in
nested_vmcb_check_cr3_cr4() has been removed.
Patch# 3: Commit message has been enhanced to explain what the test
is doing and why, when testing the 1-setting of the
non-MBZ-reserved bits.
Also, the test for legacy-PAE mode has been added. Commit
header reflects this addition.
[PATCH 1/4 v3] KVM: nSVM: CR3 MBZ bits are only 63:52
[PATCH 2/4 v3] KVM: nSVM: Add check for reserved bits for CR3, CR4, DR6,
[PATCH 3/4 v3] nSVM: Test non-MBZ reserved bits in CR3 in long mode and
[PATCH 4/4 v3] KVM: nSVM: nested_vmcb_checks() needs to check all bits
arch/x86/kvm/svm/nested.c | 52 ++++++++++++++++++++++++++---------------------
arch/x86/kvm/svm/svm.h | 2 +-
2 files changed, 30 insertions(+), 24 deletions(-)
Krish Sadhukhan (3):
KVM: nSVM: CR3 MBZ bits are only 63:52
KVM: nSVM: Add check for reserved bits for CR3, CR4, DR6, DR7 and EFER to svm_set_nested_state()
KVM: nSVM: nested_vmcb_checks() needs to check all bits of EFER
x86/svm.h | 4 +++-
x86/svm_tests.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++------
2 files changed, 63 insertions(+), 7 deletions(-)
Krish Sadhukhan (1):
nSVM: Test non-MBZ reserved bits in CR3 in long mode and legacy PAE mode
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4 v3] KVM: nSVM: CR3 MBZ bits are only 63:52
2020-10-06 19:06 [PATCH 0/4 v3] KVM: nSVM: Add checks for CR3 and CR4 reserved bits to svm_set_nested_state() and test CR3 non-MBZ reserved bits Krish Sadhukhan
@ 2020-10-06 19:06 ` Krish Sadhukhan
2020-10-06 19:06 ` [PATCH 2/4 v3] KVM: nSVM: Add check for reserved bits for CR3, CR4, DR6, DR7 and EFER to svm_set_nested_state() Krish Sadhukhan
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Krish Sadhukhan @ 2020-10-06 19:06 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, sean.j.christopherson
Commit 761e4169346553c180bbd4a383aedd72f905bc9a created a wrong mask for the
CR3 MBZ bits. According to APM vol 2, only the upper 12 bits are MBZ.
(Fixes 761e4169346553c180bbd4a383aedd72f905bc9a)
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
arch/x86/kvm/svm/svm.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index a798e1731709..c0d75b1e0664 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -345,7 +345,7 @@ static inline bool gif_set(struct vcpu_svm *svm)
/* svm.c */
#define MSR_CR3_LEGACY_RESERVED_MASK 0xfe7U
#define MSR_CR3_LEGACY_PAE_RESERVED_MASK 0x7U
-#define MSR_CR3_LONG_RESERVED_MASK 0xfff0000000000fe7U
+#define MSR_CR3_LONG_MBZ_MASK 0xfff0000000000000U
#define MSR_INVALID 0xffffffffU
u32 svm_msrpm_offset(u32 msr);
--
2.18.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/4 v3] KVM: nSVM: Add check for reserved bits for CR3, CR4, DR6, DR7 and EFER to svm_set_nested_state()
2020-10-06 19:06 [PATCH 0/4 v3] KVM: nSVM: Add checks for CR3 and CR4 reserved bits to svm_set_nested_state() and test CR3 non-MBZ reserved bits Krish Sadhukhan
2020-10-06 19:06 ` [PATCH 1/4 v3] KVM: nSVM: CR3 MBZ bits are only 63:52 Krish Sadhukhan
@ 2020-10-06 19:06 ` Krish Sadhukhan
2020-10-06 19:06 ` [PATCH 3/4 v3] nSVM: Test non-MBZ reserved bits in CR3 in long mode and legacy PAE mode Krish Sadhukhan
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Krish Sadhukhan @ 2020-10-06 19:06 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, sean.j.christopherson
The path for SVM_SET_NESTED_STATE needs to have the same checks for the CPU
registers, as we have in the VMRUN path for a nested guest. This patch adds
those missing checks to svm_set_nested_state().
Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com>
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
arch/x86/kvm/svm/nested.c | 49 +++++++++++++++++++++------------------
1 file changed, 27 insertions(+), 22 deletions(-)
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index e90bc436f584..28a931fa599e 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -215,9 +215,29 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control)
return true;
}
+static bool nested_vmcb_check_cr3_cr4(struct vcpu_svm *svm,
+ struct vmcb_save_area *save)
+{
+ if ((save->efer & EFER_LME) && (save->cr0 & X86_CR0_PG)) {
+ if (!(save->cr4 & X86_CR4_PAE) || !(save->cr0 & X86_CR0_PE) ||
+ (save->cr3 & MSR_CR3_LONG_MBZ_MASK))
+ return false;
+ } else if (save->cr4 & X86_CR4_PAE) {
+ if (save->cr3 & MSR_CR3_LEGACY_PAE_RESERVED_MASK)
+ return false;
+ } else {
+ if (save->cr3 & MSR_CR3_LEGACY_RESERVED_MASK)
+ return false;
+ }
+
+ if (kvm_valid_cr4(&svm->vcpu, save->cr4))
+ return false;
+
+ return true;
+}
+
static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb)
{
- bool nested_vmcb_lma;
if ((vmcb->save.efer & EFER_SVME) == 0)
return false;
@@ -228,25 +248,7 @@ static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb)
if (!kvm_dr6_valid(vmcb->save.dr6) || !kvm_dr7_valid(vmcb->save.dr7))
return false;
- nested_vmcb_lma =
- (vmcb->save.efer & EFER_LME) &&
- (vmcb->save.cr0 & X86_CR0_PG);
-
- if (!nested_vmcb_lma) {
- if (vmcb->save.cr4 & X86_CR4_PAE) {
- if (vmcb->save.cr3 & MSR_CR3_LEGACY_PAE_RESERVED_MASK)
- return false;
- } else {
- if (vmcb->save.cr3 & MSR_CR3_LEGACY_RESERVED_MASK)
- return false;
- }
- } else {
- if (!(vmcb->save.cr4 & X86_CR4_PAE) ||
- !(vmcb->save.cr0 & X86_CR0_PE) ||
- (vmcb->save.cr3 & MSR_CR3_LONG_RESERVED_MASK))
- return false;
- }
- if (kvm_valid_cr4(&svm->vcpu, vmcb->save.cr4))
+ if (!nested_vmcb_check_cr3_cr4(svm, &(vmcb->save)))
return false;
return nested_vmcb_check_controls(&vmcb->control);
@@ -1116,9 +1118,12 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
/*
* Validate host state saved from before VMRUN (see
* nested_svm_check_permissions).
- * TODO: validate reserved bits for all saved state.
*/
- if (!(save.cr0 & X86_CR0_PG))
+ if (!(save.cr0 & X86_CR0_PG) ||
+ !nested_vmcb_check_cr3_cr4(svm, &save) ||
+ !kvm_dr6_valid(save.dr6) ||
+ !kvm_dr7_valid(save.dr7) ||
+ !kvm_valid_efer(vcpu, save.efer))
return -EINVAL;
/*
--
2.18.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/4 v3] nSVM: Test non-MBZ reserved bits in CR3 in long mode and legacy PAE mode
2020-10-06 19:06 [PATCH 0/4 v3] KVM: nSVM: Add checks for CR3 and CR4 reserved bits to svm_set_nested_state() and test CR3 non-MBZ reserved bits Krish Sadhukhan
2020-10-06 19:06 ` [PATCH 1/4 v3] KVM: nSVM: CR3 MBZ bits are only 63:52 Krish Sadhukhan
2020-10-06 19:06 ` [PATCH 2/4 v3] KVM: nSVM: Add check for reserved bits for CR3, CR4, DR6, DR7 and EFER to svm_set_nested_state() Krish Sadhukhan
@ 2020-10-06 19:06 ` Krish Sadhukhan
2020-10-06 19:06 ` [PATCH 4/4 v3] KVM: nSVM: nested_vmcb_checks() needs to check all bits of EFER Krish Sadhukhan
2020-10-19 16:08 ` [PATCH 0/4 v3] KVM: nSVM: Add checks for CR3 and CR4 reserved bits to svm_set_nested_state() and test CR3 non-MBZ reserved bits Paolo Bonzini
4 siblings, 0 replies; 6+ messages in thread
From: Krish Sadhukhan @ 2020-10-06 19:06 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, sean.j.christopherson
According to section "CR3" in APM vol. 2, the non-MBZ reserved bits in CR3
need to be set by software as follows:
"Reserved Bits. Reserved fields should be cleared to 0 by software
when writing CR3."
But experiments show that consistency checking in SVM ignores these
non-MBZ-reserved bits in CR3, meaning they can be set to 1 also. However,
setting them to 1 may cause guest crashes in some modes and in bare metal
environments. Hence, this test induces an #NPF by clearing the "P" bit in
the highest level page table, when testing the 1-setting of these bits.
Inducing an #NPF causes the guest to exit to userspace before any guest
instruction is executed thus avoiding any crash.
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
x86/svm.h | 4 ++-
x86/svm_tests.c | 66 ++++++++++++++++++++++++++++++++++++++++++++-----
2 files changed, 63 insertions(+), 7 deletions(-)
diff --git a/x86/svm.h b/x86/svm.h
index 15e0f18..d657592 100644
--- a/x86/svm.h
+++ b/x86/svm.h
@@ -325,7 +325,9 @@ struct __attribute__ ((__packed__)) vmcb {
#define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP)
#define SVM_CR0_RESERVED_MASK 0xffffffff00000000U
-#define SVM_CR3_LONG_RESERVED_MASK 0xfff0000000000000U
+#define SVM_CR3_LONG_MBZ_MASK 0xfff0000000000000U
+#define SVM_CR3_LONG_RESERVED_MASK 0x0000000000000fe7U
+#define SVM_CR3_PAE_LEGACY_RESERVED_MASK 0x0000000000000007U
#define SVM_CR4_LEGACY_RESERVED_MASK 0xff88f000U
#define SVM_CR4_RESERVED_MASK 0xffffffffff88f000U
#define SVM_DR6_RESERVED_MASK 0xffffffffffff1ff0U
diff --git a/x86/svm_tests.c b/x86/svm_tests.c
index 1908c7c..ed21d21 100644
--- a/x86/svm_tests.c
+++ b/x86/svm_tests.c
@@ -1913,7 +1913,8 @@ static void basic_guest_main(struct svm_test *test)
} \
}
-#define SVM_TEST_CR_RESERVED_BITS(start, end, inc, cr, val, resv_mask) \
+#define SVM_TEST_CR_RESERVED_BITS(start, end, inc, cr, val, resv_mask, \
+ exit_code) \
{ \
u64 tmp, mask; \
int i; \
@@ -1933,7 +1934,7 @@ static void basic_guest_main(struct svm_test *test)
case 4: \
vmcb->save.cr4 = tmp; \
} \
- report(svm_vmrun() == SVM_EXIT_ERR, "Test CR%d %d:%d: %lx",\
+ report(svm_vmrun() == exit_code, "Test CR%d %d:%d: %lx",\
cr, end, start, tmp); \
} \
}
@@ -2012,9 +2013,62 @@ static void test_cr3(void)
u64 cr3_saved = vmcb->save.cr3;
SVM_TEST_CR_RESERVED_BITS(0, 63, 1, 3, cr3_saved,
- SVM_CR3_LONG_RESERVED_MASK);
+ SVM_CR3_LONG_MBZ_MASK, SVM_EXIT_ERR);
+
+ vmcb->save.cr3 = cr3_saved & ~SVM_CR3_LONG_MBZ_MASK;
+ report(svm_vmrun() == SVM_EXIT_VMMCALL, "Test CR3 63:0: %lx",
+ vmcb->save.cr3);
+
+ /*
+ * CR3 non-MBZ reserved bits based on different modes:
+ * [11:5] [2:0] - long mode
+ * [2:0] - PAE legacy mode
+ */
+ u64 cr4_saved = vmcb->save.cr4;
+ u64 *pdpe = npt_get_pml4e();
+
+ /*
+ * Long mode
+ */
+ if (this_cpu_has(X86_FEATURE_PCID)) {
+ vmcb->save.cr4 = cr4_saved | X86_CR4_PCIDE;
+ SVM_TEST_CR_RESERVED_BITS(0, 11, 1, 3, cr3_saved,
+ SVM_CR3_LONG_RESERVED_MASK, SVM_EXIT_VMMCALL);
+
+ vmcb->save.cr3 = cr3_saved & ~SVM_CR3_LONG_RESERVED_MASK;
+ report(svm_vmrun() == SVM_EXIT_VMMCALL, "Test CR3 63:0: %lx",
+ vmcb->save.cr3);
+ } else {
+
+ vmcb->save.cr4 = cr4_saved & ~X86_CR4_PCIDE;
+
+ /* Clear P (Present) bit in NPT in order to trigger #NPF */
+ pdpe[0] &= ~1ULL;
+
+ SVM_TEST_CR_RESERVED_BITS(0, 11, 1, 3, cr3_saved,
+ SVM_CR3_LONG_RESERVED_MASK, SVM_EXIT_NPF);
+
+ pdpe[0] |= 1ULL;
+ vmcb->save.cr3 = cr3_saved & ~SVM_CR3_LONG_RESERVED_MASK;
+ report(svm_vmrun() == SVM_EXIT_VMMCALL, "Test CR3 63:0: %lx",
+ vmcb->save.cr3);
+ }
+
+ /*
+ * PAE legacy
+ */
+ pdpe[0] &= ~1ULL;
+ vmcb->save.cr4 = cr4_saved | X86_CR4_PAE;
+ SVM_TEST_CR_RESERVED_BITS(0, 2, 1, 3, cr3_saved,
+ SVM_CR3_PAE_LEGACY_RESERVED_MASK, SVM_EXIT_NPF);
+
+ pdpe[0] |= 1ULL;
+ vmcb->save.cr3 = cr3_saved & ~SVM_CR3_PAE_LEGACY_RESERVED_MASK;
+ report(svm_vmrun() == SVM_EXIT_VMMCALL, "Test CR3 63:0: %lx",
+ vmcb->save.cr3);
vmcb->save.cr3 = cr3_saved;
+ vmcb->save.cr4 = cr4_saved;
}
static void test_cr4(void)
@@ -2031,14 +2085,14 @@ static void test_cr4(void)
efer &= ~EFER_LME;
vmcb->save.efer = efer;
SVM_TEST_CR_RESERVED_BITS(12, 31, 1, 4, cr4_saved,
- SVM_CR4_LEGACY_RESERVED_MASK);
+ SVM_CR4_LEGACY_RESERVED_MASK, SVM_EXIT_ERR);
efer |= EFER_LME;
vmcb->save.efer = efer;
SVM_TEST_CR_RESERVED_BITS(12, 31, 1, 4, cr4_saved,
- SVM_CR4_RESERVED_MASK);
+ SVM_CR4_RESERVED_MASK, SVM_EXIT_ERR);
SVM_TEST_CR_RESERVED_BITS(32, 63, 4, 4, cr4_saved,
- SVM_CR4_RESERVED_MASK);
+ SVM_CR4_RESERVED_MASK, SVM_EXIT_ERR);
vmcb->save.cr4 = cr4_saved;
vmcb->save.efer = efer_saved;
--
2.18.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 4/4 v3] KVM: nSVM: nested_vmcb_checks() needs to check all bits of EFER
2020-10-06 19:06 [PATCH 0/4 v3] KVM: nSVM: Add checks for CR3 and CR4 reserved bits to svm_set_nested_state() and test CR3 non-MBZ reserved bits Krish Sadhukhan
` (2 preceding siblings ...)
2020-10-06 19:06 ` [PATCH 3/4 v3] nSVM: Test non-MBZ reserved bits in CR3 in long mode and legacy PAE mode Krish Sadhukhan
@ 2020-10-06 19:06 ` Krish Sadhukhan
2020-10-19 16:08 ` [PATCH 0/4 v3] KVM: nSVM: Add checks for CR3 and CR4 reserved bits to svm_set_nested_state() and test CR3 non-MBZ reserved bits Paolo Bonzini
4 siblings, 0 replies; 6+ messages in thread
From: Krish Sadhukhan @ 2020-10-06 19:06 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, sean.j.christopherson
Current implementation of nested_vmcb_checks() checks only the SVME bit in
EFER. We need to check all other bits of EFER including the reserved bits.
This patch enhances nested_vmcb_checks() by calling kvm_valid_efer() which
checks all bits of EFER.
Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
arch/x86/kvm/svm/nested.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index 28a931fa599e..2426f50226d8 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -238,7 +238,8 @@ static bool nested_vmcb_check_cr3_cr4(struct vcpu_svm *svm,
static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb)
{
- if ((vmcb->save.efer & EFER_SVME) == 0)
+ if (((vmcb->save.efer & EFER_SVME) == 0) ||
+ !kvm_valid_efer(&(svm->vcpu), vmcb->save.efer))
return false;
if (((vmcb->save.cr0 & X86_CR0_CD) == 0) &&
--
2.18.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 0/4 v3] KVM: nSVM: Add checks for CR3 and CR4 reserved bits to svm_set_nested_state() and test CR3 non-MBZ reserved bits
2020-10-06 19:06 [PATCH 0/4 v3] KVM: nSVM: Add checks for CR3 and CR4 reserved bits to svm_set_nested_state() and test CR3 non-MBZ reserved bits Krish Sadhukhan
` (3 preceding siblings ...)
2020-10-06 19:06 ` [PATCH 4/4 v3] KVM: nSVM: nested_vmcb_checks() needs to check all bits of EFER Krish Sadhukhan
@ 2020-10-19 16:08 ` Paolo Bonzini
4 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2020-10-19 16:08 UTC (permalink / raw)
To: Krish Sadhukhan, kvm; +Cc: jmattson, sean.j.christopherson
On 06/10/20 21:06, Krish Sadhukhan wrote:
> v2 -> v3:
> Patch# 2: The local variable "nested_vmcb_lma" in
> nested_vmcb_check_cr3_cr4() has been removed.
> Patch# 3: Commit message has been enhanced to explain what the test
> is doing and why, when testing the 1-setting of the
> non-MBZ-reserved bits.
> Also, the test for legacy-PAE mode has been added. Commit
> header reflects this addition.
>
>
> [PATCH 1/4 v3] KVM: nSVM: CR3 MBZ bits are only 63:52
> [PATCH 2/4 v3] KVM: nSVM: Add check for reserved bits for CR3, CR4, DR6,
> [PATCH 3/4 v3] nSVM: Test non-MBZ reserved bits in CR3 in long mode and
> [PATCH 4/4 v3] KVM: nSVM: nested_vmcb_checks() needs to check all bits
>
> arch/x86/kvm/svm/nested.c | 52 ++++++++++++++++++++++++++---------------------
> arch/x86/kvm/svm/svm.h | 2 +-
> 2 files changed, 30 insertions(+), 24 deletions(-)
>
> Krish Sadhukhan (3):
> KVM: nSVM: CR3 MBZ bits are only 63:52
> KVM: nSVM: Add check for reserved bits for CR3, CR4, DR6, DR7 and EFER to svm_set_nested_state()
> KVM: nSVM: nested_vmcb_checks() needs to check all bits of EFER
>
> x86/svm.h | 4 +++-
> x86/svm_tests.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 63 insertions(+), 7 deletions(-)
>
> Krish Sadhukhan (1):
> nSVM: Test non-MBZ reserved bits in CR3 in long mode and legacy PAE mode
>
Queued, but I don't really like the duplication in patch 2 so I'll
probably punt it to 5.11 and fix it up.
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-19 16:08 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-06 19:06 [PATCH 0/4 v3] KVM: nSVM: Add checks for CR3 and CR4 reserved bits to svm_set_nested_state() and test CR3 non-MBZ reserved bits Krish Sadhukhan
2020-10-06 19:06 ` [PATCH 1/4 v3] KVM: nSVM: CR3 MBZ bits are only 63:52 Krish Sadhukhan
2020-10-06 19:06 ` [PATCH 2/4 v3] KVM: nSVM: Add check for reserved bits for CR3, CR4, DR6, DR7 and EFER to svm_set_nested_state() Krish Sadhukhan
2020-10-06 19:06 ` [PATCH 3/4 v3] nSVM: Test non-MBZ reserved bits in CR3 in long mode and legacy PAE mode Krish Sadhukhan
2020-10-06 19:06 ` [PATCH 4/4 v3] KVM: nSVM: nested_vmcb_checks() needs to check all bits of EFER Krish Sadhukhan
2020-10-19 16:08 ` [PATCH 0/4 v3] KVM: nSVM: Add checks for CR3 and CR4 reserved bits to svm_set_nested_state() and test CR3 non-MBZ reserved bits Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).