* [PATCH 0/3] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests @ 2021-01-13 2:46 Krish Sadhukhan 2021-01-13 2:46 ` [PATCH 1/3] KVM: nSVM: Check addresses of MSR and IO bitmap Krish Sadhukhan ` (2 more replies) 0 siblings, 3 replies; 5+ messages in thread From: Krish Sadhukhan @ 2021-01-13 2:46 UTC (permalink / raw) To: kvm; +Cc: pbonzini, jmattson, seanjc According to section "Canonicalization and Consistency Checks" in APM vol 2, the following guest state is illegal: "The MSR or IOIO intercept tables extend to a physical address that is greater than or equal to the maximum supported physical address." Patch# 1: Adds the KVM checks. Patch# 2: Adds a test Patch# 3: Replaces a hard-coded value with an available macro. [PATCH 1/3] KVM: nSVM: Check addresses of MSR and IO bitmap [PATCH 2/3] Test: nSVM: Test MSR and IO bitmap address [PATCH 3/3] Test: SVM: Use ALIGN macro when aligning 'io_bitmap_area' arch/x86/kvm/svm/nested.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) Krish Sadhukhan (1): nSVM: Check addresses of MSR and IO bitmap x86/svm.c | 2 +- x86/svm_tests.c | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) Krish Sadhukhan (2): nSVM: Test MSR and IO bitmap address SVM: Use ALIGN macro when aligning 'io_bitmap_area' ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/3] KVM: nSVM: Check addresses of MSR and IO bitmap 2021-01-13 2:46 [PATCH 0/3] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests Krish Sadhukhan @ 2021-01-13 2:46 ` Krish Sadhukhan 2021-01-13 17:43 ` Sean Christopherson 2021-01-13 2:46 ` [PATCH 2/3] Test: nSVM: Test MSR and IO bitmap address Krish Sadhukhan 2021-01-13 2:46 ` [PATCH 3/3] Test: SVM: Use ALIGN macro when aligning 'io_bitmap_area' Krish Sadhukhan 2 siblings, 1 reply; 5+ messages in thread From: Krish Sadhukhan @ 2021-01-13 2:46 UTC (permalink / raw) To: kvm; +Cc: pbonzini, jmattson, seanjc According to section "Canonicalization and Consistency Checks" in APM vol 2, the following guest state is illegal: "The MSR or IOIO intercept tables extend to a physical address that is greater than or equal to the maximum supported physical address." Also check that these addresses are aligned on page boundary. Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> --- arch/x86/kvm/svm/nested.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index cb4c6ee10029..389a8108ddb5 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -211,8 +211,11 @@ static bool svm_get_nested_state_pages(struct kvm_vcpu *vcpu) return true; } -static bool nested_vmcb_check_controls(struct vmcb_control_area *control) +static bool nested_vmcb_check_controls(struct vcpu_svm *svm, + struct vmcb_control_area *control) { + int maxphyaddr; + if ((vmcb_is_intercept(control, INTERCEPT_VMRUN)) == 0) return false; @@ -223,6 +226,14 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control) !npt_enabled) return false; + maxphyaddr = cpuid_maxphyaddr(&svm->vcpu); + if (!IS_ALIGNED(control->msrpm_base_pa, PAGE_SIZE) || + control->msrpm_base_pa >> maxphyaddr) + return false; + if (!IS_ALIGNED(control->iopm_base_pa, PAGE_SIZE) || + control->iopm_base_pa >> maxphyaddr) + return false; + return true; } @@ -258,7 +269,7 @@ static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb12) if (!kvm_is_valid_cr4(&svm->vcpu, vmcb12->save.cr4)) return false; - return nested_vmcb_check_controls(&vmcb12->control); + return nested_vmcb_check_controls(svm, &vmcb12->control); } static void load_nested_vmcb_control(struct vcpu_svm *svm, @@ -1173,7 +1184,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, goto out_free; ret = -EINVAL; - if (!nested_vmcb_check_controls(ctl)) + if (!nested_vmcb_check_controls(svm, ctl)) goto out_free; /* -- 2.27.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/3] KVM: nSVM: Check addresses of MSR and IO bitmap 2021-01-13 2:46 ` [PATCH 1/3] KVM: nSVM: Check addresses of MSR and IO bitmap Krish Sadhukhan @ 2021-01-13 17:43 ` Sean Christopherson 0 siblings, 0 replies; 5+ messages in thread From: Sean Christopherson @ 2021-01-13 17:43 UTC (permalink / raw) To: Krish Sadhukhan; +Cc: kvm, pbonzini, jmattson On Wed, Jan 13, 2021, Krish Sadhukhan wrote: > According to section "Canonicalization and Consistency Checks" in APM vol 2, > the following guest state is illegal: > > "The MSR or IOIO intercept tables extend to a physical address that > is greater than or equal to the maximum supported physical address." > > Also check that these addresses are aligned on page boundary. > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > --- > arch/x86/kvm/svm/nested.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > index cb4c6ee10029..389a8108ddb5 100644 > --- a/arch/x86/kvm/svm/nested.c > +++ b/arch/x86/kvm/svm/nested.c > @@ -211,8 +211,11 @@ static bool svm_get_nested_state_pages(struct kvm_vcpu *vcpu) > return true; > } > > -static bool nested_vmcb_check_controls(struct vmcb_control_area *control) > +static bool nested_vmcb_check_controls(struct vcpu_svm *svm, It's probably worth passing vcpu instead of svm. svm_set_nested_state() already takes vcpu, and nested_vmcb_checks() could easily do the same (in a separate cleanup), especially if nested_svm_vmrun() were cleaned up to capture vcpu in a local variable instead of constantly doing &svm->vcpu. > + struct vmcb_control_area *control) > { > + int maxphyaddr; > + > if ((vmcb_is_intercept(control, INTERCEPT_VMRUN)) == 0) > return false; > > @@ -223,6 +226,14 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control) > !npt_enabled) > return false; > > + maxphyaddr = cpuid_maxphyaddr(&svm->vcpu); > + if (!IS_ALIGNED(control->msrpm_base_pa, PAGE_SIZE) || > + control->msrpm_base_pa >> maxphyaddr) These can use page_address_valid(). Unrelated to this patch, we really should consolidate all the different flavors of open-coded variants of maxphyaddr checks to use kvm_vcpu_is_illegal_gpa(), and maybe add a helper to do an arbitrary alignment check. VMX has a handful of checks that fit that pattern and aren't exactly intuitive at first glance. We could also add a kvm_vcpu_rsvd_gpa_bits() too. Somehow even that has multiple open-coded variants. I'll add those cleanups to the todo list. > + return false; > + if (!IS_ALIGNED(control->iopm_base_pa, PAGE_SIZE) || > + control->iopm_base_pa >> maxphyaddr) > + return false; > + > return true; > } > > @@ -258,7 +269,7 @@ static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb12) > if (!kvm_is_valid_cr4(&svm->vcpu, vmcb12->save.cr4)) > return false; > > - return nested_vmcb_check_controls(&vmcb12->control); > + return nested_vmcb_check_controls(svm, &vmcb12->control); > } > > static void load_nested_vmcb_control(struct vcpu_svm *svm, > @@ -1173,7 +1184,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, > goto out_free; > > ret = -EINVAL; > - if (!nested_vmcb_check_controls(ctl)) > + if (!nested_vmcb_check_controls(svm, ctl)) > goto out_free; > > /* > -- > 2.27.0 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/3] Test: nSVM: Test MSR and IO bitmap address 2021-01-13 2:46 [PATCH 0/3] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests Krish Sadhukhan 2021-01-13 2:46 ` [PATCH 1/3] KVM: nSVM: Check addresses of MSR and IO bitmap Krish Sadhukhan @ 2021-01-13 2:46 ` Krish Sadhukhan 2021-01-13 2:46 ` [PATCH 3/3] Test: SVM: Use ALIGN macro when aligning 'io_bitmap_area' Krish Sadhukhan 2 siblings, 0 replies; 5+ messages in thread From: Krish Sadhukhan @ 2021-01-13 2:46 UTC (permalink / raw) To: kvm; +Cc: pbonzini, jmattson, seanjc According to section "Canonicalization and Consistency Checks" in APM vol 2, the following guest state is illegal: "The MSR or IOIO intercept tables extend to a physical address that is greater than or equal to the maximum supported physical address." Also test that these addresses are aligned on page boundary. Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> --- x86/svm_tests.c | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/x86/svm_tests.c b/x86/svm_tests.c index dc86efd..929a3e1 100644 --- a/x86/svm_tests.c +++ b/x86/svm_tests.c @@ -2304,6 +2304,43 @@ static void test_dr(void) vmcb->save.dr7 = dr_saved; } +extern u8 msr_bitmap_area[]; +extern u8 io_bitmap_area[]; + +#define TEST_BITMAP_ADDR(prot_type, bitmap_addr, msg) { \ + vmcb->control.intercept = 1ULL << prot_type; \ + addr_unalign = virt_to_phys(bitmap_addr); \ + if (prot_type == INTERCEPT_MSR_PROT) \ + vmcb->control.msrpm_base_pa = addr_unalign; \ + else \ + vmcb->control.iopm_base_pa = addr_unalign; \ + report(svm_vmrun() == SVM_EXIT_ERR, "Test %s address: %lx", msg,\ + addr_unalign); \ + vmcb->control.msrpm_base_pa = addr_spill_beyond_ram; \ + report(svm_vmrun() == SVM_EXIT_ERR, "Test %s address: %lx", msg,\ + addr_spill_beyond_ram); \ +} \ + +/* + * If the MSR or IOIO intercept table extends to a physical address that + * is greater than or equal to the maximum supported physical address, the + * guest state is illegal. + * + * [ APM vol 2] + */ +static void test_msrpm_iopm_bitmap_addrs(void) +{ + u64 addr_unalign; + u64 addr_spill_beyond_ram = + (u64)(((u64)1 << cpuid_maxphyaddr()) - 4096); + + /* MSR bitmap address */ + TEST_BITMAP_ADDR(INTERCEPT_MSR_PROT, msr_bitmap_area, "MSRPM"); + + /* MSR bitmap address */ + TEST_BITMAP_ADDR(INTERCEPT_IOIO_PROT, io_bitmap_area, "IOPM"); +} + static void svm_guest_state_test(void) { test_set_guest(basic_guest_main); @@ -2313,6 +2350,7 @@ static void svm_guest_state_test(void) test_cr3(); test_cr4(); test_dr(); + test_msrpm_iopm_bitmap_addrs(); } struct svm_test svm_tests[] = { -- 2.27.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] Test: SVM: Use ALIGN macro when aligning 'io_bitmap_area' 2021-01-13 2:46 [PATCH 0/3] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests Krish Sadhukhan 2021-01-13 2:46 ` [PATCH 1/3] KVM: nSVM: Check addresses of MSR and IO bitmap Krish Sadhukhan 2021-01-13 2:46 ` [PATCH 2/3] Test: nSVM: Test MSR and IO bitmap address Krish Sadhukhan @ 2021-01-13 2:46 ` Krish Sadhukhan 2 siblings, 0 replies; 5+ messages in thread From: Krish Sadhukhan @ 2021-01-13 2:46 UTC (permalink / raw) To: kvm; +Cc: pbonzini, jmattson, seanjc Since the macro is available and we already use it for MSR bitmap table, use it for aligning IO bitmap table also. Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> --- x86/svm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x86/svm.c b/x86/svm.c index a1808c7..846cf2a 100644 --- a/x86/svm.c +++ b/x86/svm.c @@ -298,7 +298,7 @@ static void setup_svm(void) wrmsr(MSR_EFER, rdmsr(MSR_EFER) | EFER_SVME); wrmsr(MSR_EFER, rdmsr(MSR_EFER) | EFER_NX); - io_bitmap = (void *) (((ulong)io_bitmap_area + 4095) & ~4095); + io_bitmap = (void *) ALIGN((ulong)io_bitmap_area, PAGE_SIZE); msr_bitmap = (void *) ALIGN((ulong)msr_bitmap_area, PAGE_SIZE); -- 2.27.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-13 17:44 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-13 2:46 [PATCH 0/3] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests Krish Sadhukhan 2021-01-13 2:46 ` [PATCH 1/3] KVM: nSVM: Check addresses of MSR and IO bitmap Krish Sadhukhan 2021-01-13 17:43 ` Sean Christopherson 2021-01-13 2:46 ` [PATCH 2/3] Test: nSVM: Test MSR and IO bitmap address Krish Sadhukhan 2021-01-13 2:46 ` [PATCH 3/3] Test: SVM: Use ALIGN macro when aligning 'io_bitmap_area' Krish Sadhukhan
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).