* [PATCH 0/2] KVM: nSVM: Check MBZ bits in CR3 and CR4 on vmrun of nested guests
@ 2020-05-04 22:35 Krish Sadhukhan
2020-05-04 22:35 ` [PATCH 1/2] KVM: nSVM: Check that MBZ bits in CR3 and CR4 are not set " Krish Sadhukhan
2020-05-04 22:35 ` [PATCH 2/2] kvm-unit-tests: nSVM: Test " Krish Sadhukhan
0 siblings, 2 replies; 4+ messages in thread
From: Krish Sadhukhan @ 2020-05-04 22:35 UTC (permalink / raw)
To: kvm; +Cc: pbonzini
Patch# 1: Adds the required KVM check.
Patch# 2: Adds the kvm-unit-test.
[PATCH 1/2] KVM: nSVM: Check that MBZ bits in CR3 and CR4 are not set on vmrun
[PATCH 2/2] kvm-unit-tests: nSVM: Test that MBZ bits in CR3 and CR4 are not set on vmrun
arch/x86/kvm/svm/nested.c | 18 ++++++++++++++++++
arch/x86/kvm/svm/svm.h | 7 ++++++-
2 files changed, 24 insertions(+), 1 deletion(-)
Krish Sadhukhan (1):
nSVM: Check that MBZ bits in CR3 and CR4 are not set on vmrun of nested gu
x86/svm.h | 6 ++++
x86/svm_tests.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++-------
2 files changed, 99 insertions(+), 12 deletions(-)
Krish Sadhukhan (1):
nSVM: Test that MBZ bits in CR3 and CR4 are not set on vmrun of nested gue
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH 1/2] KVM: nSVM: Check that MBZ bits in CR3 and CR4 are not set on vmrun of nested guests 2020-05-04 22:35 [PATCH 0/2] KVM: nSVM: Check MBZ bits in CR3 and CR4 on vmrun of nested guests Krish Sadhukhan @ 2020-05-04 22:35 ` Krish Sadhukhan 2020-05-07 13:46 ` Paolo Bonzini 2020-05-04 22:35 ` [PATCH 2/2] kvm-unit-tests: nSVM: Test " Krish Sadhukhan 1 sibling, 1 reply; 4+ messages in thread From: Krish Sadhukhan @ 2020-05-04 22:35 UTC (permalink / raw) To: kvm; +Cc: pbonzini According to section "Canonicalization and Consistency Checks" in APM vol. 2 the following guest state is illegal: "Any MBZ bit of CR3 is set." "Any MBZ bit of CR4 is set." Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> --- arch/x86/kvm/svm/nested.c | 18 ++++++++++++++++++ arch/x86/kvm/svm/svm.h | 7 ++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index 90a1ca9..1804a97 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -207,6 +207,24 @@ static bool nested_vmcb_checks(struct vmcb *vmcb) if ((vmcb->save.efer & EFER_SVME) == 0) return false; + if (!(vmcb->save.efer & EFER_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; + } + if (vmcb->save.cr4 & MSR_CR4_LEGACY_RESERVED_MASK) + return false; + } else { + if ((vmcb->save.cr4 & X86_CR4_PAE) && + (vmcb->save.cr3 & MSR_CR3_LONG_RESERVED_MASK)) + return false; + if (vmcb->save.cr4 & MSR_CR4_RESERVED_MASK) + return false; + } + if ((vmcb->control.intercept & (1ULL << INTERCEPT_VMRUN)) == 0) return false; diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h index df3474f..796c083 100644 --- a/arch/x86/kvm/svm/svm.h +++ b/arch/x86/kvm/svm/svm.h @@ -354,7 +354,12 @@ static inline bool gif_set(struct vcpu_svm *svm) } /* svm.c */ -#define MSR_INVALID 0xffffffffU +#define MSR_CR3_LEGACY_RESERVED_MASK 0xfe7U +#define MSR_CR3_LEGACY_PAE_RESERVED_MASK 0x7U +#define MSR_CR3_LONG_RESERVED_MASK 0xfff0000000000fe7U +#define MSR_CR4_LEGACY_RESERVED_MASK 0xffbaf000U +#define MSR_CR4_RESERVED_MASK 0xffffffffffbaf000U +#define MSR_INVALID 0xffffffffU u32 svm_msrpm_offset(u32 msr); void svm_set_efer(struct kvm_vcpu *vcpu, u64 efer); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] KVM: nSVM: Check that MBZ bits in CR3 and CR4 are not set on vmrun of nested guests 2020-05-04 22:35 ` [PATCH 1/2] KVM: nSVM: Check that MBZ bits in CR3 and CR4 are not set " Krish Sadhukhan @ 2020-05-07 13:46 ` Paolo Bonzini 0 siblings, 0 replies; 4+ messages in thread From: Paolo Bonzini @ 2020-05-07 13:46 UTC (permalink / raw) To: Krish Sadhukhan, kvm, Wei Huang On 05/05/20 00:35, Krish Sadhukhan wrote: > + if (!(vmcb->save.efer & EFER_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; > + } > + if (vmcb->save.cr4 & MSR_CR4_LEGACY_RESERVED_MASK) > + return false; > + } else { > + if ((vmcb->save.cr4 & X86_CR4_PAE) && > + (vmcb->save.cr3 & MSR_CR3_LONG_RESERVED_MASK)) > + return false; > + if (vmcb->save.cr4 & MSR_CR4_RESERVED_MASK) > + return false; > + } > + > if ((vmcb->control.intercept & (1ULL << INTERCEPT_VMRUN)) == 0) > return false; > I think checking LMA from the guest state is incorrect, the number of bits in CR3 and CR4 remains 64 as long as the host processor is 64-bits. This of course is unless you have reproduced on bare metal that a hypervisor running in 32-bit mode ignores the top 32 bits. Also, the checks for CR4 must use the guest's reserved bits, using kvm_valid_cr4. However this can be a bit slow so it is probably a good idea to cache the bits in kvm_update_cpuid. Thanks, Paolo > diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h > index df3474f..796c083 100644 > --- a/arch/x86/kvm/svm/svm.h > +++ b/arch/x86/kvm/svm/svm.h > @@ -354,7 +354,12 @@ static inline bool gif_set(struct vcpu_svm *svm) > } > > /* svm.c */ > -#define MSR_INVALID 0xffffffffU > +#define MSR_CR3_LEGACY_RESERVED_MASK 0xfe7U > +#define MSR_CR3_LEGACY_PAE_RESERVED_MASK 0x7U > +#define MSR_CR3_LONG_RESERVED_MASK 0xfff0000000000fe7U > +#define MSR_CR4_LEGACY_RESERVED_MASK 0xffbaf000U > +#define MSR_CR4_RESERVED_MASK 0xffffffffffbaf000U > +#define MSR_INVALID 0xffffffffU > ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 2/2] kvm-unit-tests: nSVM: Test that MBZ bits in CR3 and CR4 are not set on vmrun of nested guests 2020-05-04 22:35 [PATCH 0/2] KVM: nSVM: Check MBZ bits in CR3 and CR4 on vmrun of nested guests Krish Sadhukhan 2020-05-04 22:35 ` [PATCH 1/2] KVM: nSVM: Check that MBZ bits in CR3 and CR4 are not set " Krish Sadhukhan @ 2020-05-04 22:35 ` Krish Sadhukhan 1 sibling, 0 replies; 4+ messages in thread From: Krish Sadhukhan @ 2020-05-04 22:35 UTC (permalink / raw) To: kvm; +Cc: pbonzini According to section "Canonicalization and Consistency Checks" in APM vol. 2, the following guest state is illegal: "Any MBZ bit of CR3 is set." "Any MBZ bit of CR4 is set." Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> --- x86/svm.h | 6 ++++ x86/svm_tests.c | 105 +++++++++++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 99 insertions(+), 12 deletions(-) diff --git a/x86/svm.h b/x86/svm.h index 645deb7..3d5b79c 100644 --- a/x86/svm.h +++ b/x86/svm.h @@ -323,6 +323,12 @@ struct __attribute__ ((__packed__)) vmcb { #define SVM_EXIT_ERR -1 #define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP) +#define SVM_CR0_RESERVED_MASK 0xffffffff00000000U +#define SVM_CR3_LEGACY_RESERVED_MASK 0xfe7U +#define SVM_CR3_LEGACY_PAE_RESERVED_MASK 0x7U +#define SVM_CR3_LONG_RESERVED_MASK 0xfff0000000000fe7U +#define SVM_CR4_LEGACY_RESERVED_MASK 0xffbaf000U +#define SVM_CR4_RESERVED_MASK 0xffffffffffbaf000U #define MSR_BITMAP_SIZE 8192 diff --git a/x86/svm_tests.c b/x86/svm_tests.c index 5a571eb..5d57ad0 100644 --- a/x86/svm_tests.c +++ b/x86/svm_tests.c @@ -1596,6 +1596,31 @@ static void basic_guest_main(struct svm_test *test) { } +#define SVM_TEST_CR_RESERVED_BITS(start, end, inc, cr, val, resv_mask) \ +{ \ + u64 tmp, mask; \ + int i; \ + \ + for (i = start; i <= end; i = i + inc) { \ + mask = 1ull << i; \ + if (!(mask & resv_mask)) \ + continue; \ + tmp = val | mask; \ + switch (cr) { \ + case 0: \ + vmcb->save.cr0 = tmp; \ + break; \ + case 3: \ + vmcb->save.cr3 = tmp; \ + break; \ + case 4: \ + vmcb->save.cr4 = tmp; \ + } \ + report(svm_vmrun() == SVM_EXIT_ERR, "Test CR%d %d:%d: %lx",\ + cr, end, start, tmp); \ + } \ +} + static void svm_guest_state_test(void) { test_set_guest(basic_guest_main); @@ -1621,32 +1646,88 @@ static void svm_guest_state_test(void) cr0 |= X86_CR0_CD; cr0 &= ~X86_CR0_NW; vmcb->save.cr0 = cr0; - report (svm_vmrun() == SVM_EXIT_VMMCALL, "CR0: %lx", cr0); + report (svm_vmrun() == SVM_EXIT_VMMCALL, "Test CR0 CD=1,NW=0: %lx", + cr0); cr0 |= X86_CR0_NW; vmcb->save.cr0 = cr0; - report (svm_vmrun() == SVM_EXIT_VMMCALL, "CR0: %lx", cr0); + report (svm_vmrun() == SVM_EXIT_VMMCALL, "Test CR0 CD=1,NW=1: %lx", + cr0); cr0 &= ~X86_CR0_NW; cr0 &= ~X86_CR0_CD; vmcb->save.cr0 = cr0; - report (svm_vmrun() == SVM_EXIT_VMMCALL, "CR0: %lx", cr0); + report (svm_vmrun() == SVM_EXIT_VMMCALL, "Test CR0 CD=0,NW=0: %lx", + cr0); cr0 |= X86_CR0_NW; vmcb->save.cr0 = cr0; - report (svm_vmrun() == SVM_EXIT_ERR, "CR0: %lx", cr0); + report (svm_vmrun() == SVM_EXIT_VMMCALL, "Test CR0 CD=0,NW=1: %lx", + cr0); vmcb->save.cr0 = cr0_saved; /* * CR0[63:32] are not zero */ - int i; - cr0 = cr0_saved; - for (i = 32; i < 63; i = i + 4) { - cr0 = cr0_saved | (1ull << i); - vmcb->save.cr0 = cr0; - report (svm_vmrun() == SVM_EXIT_ERR, "CR0[63:32]: %lx", - cr0 >> 32); - } + + SVM_TEST_CR_RESERVED_BITS(32, 63, 4, 0, cr0_saved, + SVM_CR0_RESERVED_MASK); vmcb->save.cr0 = cr0_saved; + + /* + * CR3 MBZ bits based on different modes: + * [2:0] - legacy PAE + * [2:0], [11:5] - legacy non-PAE + * [2:0], [11:5], [63:52] - long mode + */ + u64 cr3_saved = vmcb->save.cr3; + u64 cr4_saved = vmcb->save.cr4; + u64 cr4 = cr4_saved; + efer_saved = vmcb->save.efer; + efer = efer_saved; + + efer &= ~EFER_LMA; + vmcb->save.efer = efer; + cr4 |= X86_CR4_PAE; + vmcb->save.cr4 = cr4; + SVM_TEST_CR_RESERVED_BITS(0, 2, 1, 3, cr3_saved, + SVM_CR3_LEGACY_PAE_RESERVED_MASK); + + cr4 = cr4_saved & ~X86_CR4_PAE; + vmcb->save.cr4 = cr4; + SVM_TEST_CR_RESERVED_BITS(0, 11, 2, 3, cr3_saved, + SVM_CR3_LEGACY_RESERVED_MASK); + + cr4 |= X86_CR4_PAE; + vmcb->save.cr4 = cr4; + efer |= EFER_LMA; + vmcb->save.efer = efer; + SVM_TEST_CR_RESERVED_BITS(0, 63, 2, 3, cr3_saved, + SVM_CR3_LONG_RESERVED_MASK); + + vmcb->save.cr4 = cr4_saved; + vmcb->save.cr3 = cr3_saved; + vmcb->save.efer = efer_saved; + + /* + * CR4 MBZ bits based on different modes: + * [15:12], 17, 19, [31:22] - legacy mode + * [15:12], 17, 19, [63:22] - long mode + */ + cr4_saved = vmcb->save.cr4; + efer_saved = vmcb->save.efer; + efer &= ~EFER_LMA; + vmcb->save.efer = efer; + SVM_TEST_CR_RESERVED_BITS(12, 31, 2, 4, cr4_saved, + SVM_CR4_LEGACY_RESERVED_MASK); + + efer |= EFER_LMA; + vmcb->save.efer = efer; + SVM_TEST_CR_RESERVED_BITS(12, 31, 2, 4, cr4_saved, + SVM_CR4_RESERVED_MASK); + SVM_TEST_CR_RESERVED_BITS(32, 63, 4, 4, cr4_saved, + SVM_CR4_RESERVED_MASK); + + vmcb->save.cr4 = cr4_saved; + vmcb->save.efer = efer_saved; } struct svm_test svm_tests[] = { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-05-07 13:46 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-05-04 22:35 [PATCH 0/2] KVM: nSVM: Check MBZ bits in CR3 and CR4 on vmrun of nested guests Krish Sadhukhan 2020-05-04 22:35 ` [PATCH 1/2] KVM: nSVM: Check that MBZ bits in CR3 and CR4 are not set " Krish Sadhukhan 2020-05-07 13:46 ` Paolo Bonzini 2020-05-04 22:35 ` [PATCH 2/2] kvm-unit-tests: nSVM: Test " Krish Sadhukhan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox