* [PATCH 0/3 v2] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests @ 2021-01-16 2:20 Krish Sadhukhan 2021-01-16 2:20 ` [PATCH 1/3 v2] nSVM: Check addresses of MSR and IO bitmap Krish Sadhukhan ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Krish Sadhukhan @ 2021-01-16 2:20 UTC (permalink / raw) To: kvm; +Cc: pbonzini, jmattson, seanjc v1 -> v2: In patch# 1, the parameter to nested_vmcb_check_controls() has been changed to 'struct kvm_vcpu' from 'struct vcpu_svm'. This necessiated relevant changes to its callers. Also, the new checks in nested_vmcb_check_controls() now use page_address_valid() instead of duplicating code. It has also been rebased to v5.11-rc3. [PATCH 1/3 v2] nSVM: Check addresses of MSR and IO bitmap [PATCH 2/3 v2] Test: nSVM: Test MSR and IO bitmap address [PATCH 3/3 v2] Test: SVM: Use ALIGN macro when aligning 'io_bitmap_area' arch/x86/kvm/svm/nested.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 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] 6+ messages in thread
* [PATCH 1/3 v2] nSVM: Check addresses of MSR and IO bitmap 2021-01-16 2:20 [PATCH 0/3 v2] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests Krish Sadhukhan @ 2021-01-16 2:20 ` Krish Sadhukhan 2021-01-20 0:45 ` Sean Christopherson 2021-01-16 2:20 ` [PATCH 2/3 v2] Test: nSVM: Test MSR and IO bitmap address Krish Sadhukhan 2021-01-16 2:20 ` [PATCH 3/3 v2] Test: SVM: Use ALIGN macro when aligning 'io_bitmap_area' Krish Sadhukhan 2 siblings, 1 reply; 6+ messages in thread From: Krish Sadhukhan @ 2021-01-16 2:20 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 | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index cb4c6ee10029..2419f392a13d 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -211,7 +211,8 @@ 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 kvm_vcpu *vcpu, + struct vmcb_control_area *control) { if ((vmcb_is_intercept(control, INTERCEPT_VMRUN)) == 0) return false; @@ -223,10 +224,15 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control) !npt_enabled) return false; + if (!page_address_valid(vcpu, control->msrpm_base_pa)) + return false; + if (!page_address_valid(vcpu, control->iopm_base_pa)) + return false; + return true; } -static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb12) +static bool nested_vmcb_checks(struct kvm_vcpu *vcpu, struct vmcb *vmcb12) { bool vmcb12_lma; @@ -255,10 +261,10 @@ static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb12) (vmcb12->save.cr3 & MSR_CR3_LONG_MBZ_MASK)) return false; } - if (!kvm_is_valid_cr4(&svm->vcpu, vmcb12->save.cr4)) + if (!kvm_is_valid_cr4(vcpu, vmcb12->save.cr4)) return false; - return nested_vmcb_check_controls(&vmcb12->control); + return nested_vmcb_check_controls(vcpu, &vmcb12->control); } static void load_nested_vmcb_control(struct vcpu_svm *svm, @@ -485,7 +491,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm) if (WARN_ON_ONCE(!svm->nested.initialized)) return -EINVAL; - if (!nested_vmcb_checks(svm, vmcb12)) { + if (!nested_vmcb_checks(&svm->vcpu, vmcb12)) { vmcb12->control.exit_code = SVM_EXIT_ERR; vmcb12->control.exit_code_hi = 0; vmcb12->control.exit_info_1 = 0; @@ -1173,7 +1179,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(vcpu, ctl)) goto out_free; /* -- 2.27.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3 v2] nSVM: Check addresses of MSR and IO bitmap 2021-01-16 2:20 ` [PATCH 1/3 v2] nSVM: Check addresses of MSR and IO bitmap Krish Sadhukhan @ 2021-01-20 0:45 ` Sean Christopherson 2021-01-21 0:36 ` Krish Sadhukhan 0 siblings, 1 reply; 6+ messages in thread From: Sean Christopherson @ 2021-01-20 0:45 UTC (permalink / raw) To: Krish Sadhukhan; +Cc: kvm, pbonzini, jmattson On Sat, Jan 16, 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 | 18 ++++++++++++------ > 1 file changed, 12 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > index cb4c6ee10029..2419f392a13d 100644 > --- a/arch/x86/kvm/svm/nested.c > +++ b/arch/x86/kvm/svm/nested.c > @@ -211,7 +211,8 @@ 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 kvm_vcpu *vcpu, > + struct vmcb_control_area *control) > { > if ((vmcb_is_intercept(control, INTERCEPT_VMRUN)) == 0) > return false; > @@ -223,10 +224,15 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control) > !npt_enabled) > return false; > > + if (!page_address_valid(vcpu, control->msrpm_base_pa)) > + return false; > + if (!page_address_valid(vcpu, control->iopm_base_pa)) These checks are wrong. The MSRPM is 8kb in size, and the IOPM is 12kb, and the APM explicitly states that the last byte is checked: if the address of the last byte in the table is greater than or equal to the maximum supported physical address, this is treated as illegal VMCB state and causes a #VMEXIT(VMEXIT_INVALID). KVM can't check just the last byte, as that would fail to detect a wrap of the 64-bit boundary. Might be worth adding yet another helper? I think this will work, though I'm sure Paolo has a much more clever solution :-) static inline bool page_range_valid(struct kvm_vcpu *vcpu, gpa_t gpa, int size) { gpa_t last_page = gpa + size - PAGE_SIZE; if (last_page < gpa) return false; return page_address_valid(last_page); } Note, the IOPM is 12kb in size, but KVM allocates and initializes 16kb, i.e. using IOPM_ALLOC_ORDER for the check would be wrong. Maybe define the actual size for both bitmaps and use get_order() instead of hardcoding the order? That would make it easy to "fix" svm_hardware_setup() so that it doesn't initialize unused memory. > + return false; > + > return true; > } > > -static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb12) > +static bool nested_vmcb_checks(struct kvm_vcpu *vcpu, struct vmcb *vmcb12) > { > bool vmcb12_lma; > > @@ -255,10 +261,10 @@ static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb12) > (vmcb12->save.cr3 & MSR_CR3_LONG_MBZ_MASK)) > return false; > } > - if (!kvm_is_valid_cr4(&svm->vcpu, vmcb12->save.cr4)) > + if (!kvm_is_valid_cr4(vcpu, vmcb12->save.cr4)) > return false; > > - return nested_vmcb_check_controls(&vmcb12->control); > + return nested_vmcb_check_controls(vcpu, &vmcb12->control); > } > > static void load_nested_vmcb_control(struct vcpu_svm *svm, > @@ -485,7 +491,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm) > if (WARN_ON_ONCE(!svm->nested.initialized)) > return -EINVAL; > > - if (!nested_vmcb_checks(svm, vmcb12)) { > + if (!nested_vmcb_checks(&svm->vcpu, vmcb12)) { > vmcb12->control.exit_code = SVM_EXIT_ERR; > vmcb12->control.exit_code_hi = 0; > vmcb12->control.exit_info_1 = 0; > @@ -1173,7 +1179,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(vcpu, ctl)) > goto out_free; > > /* > -- > 2.27.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3 v2] nSVM: Check addresses of MSR and IO bitmap 2021-01-20 0:45 ` Sean Christopherson @ 2021-01-21 0:36 ` Krish Sadhukhan 0 siblings, 0 replies; 6+ messages in thread From: Krish Sadhukhan @ 2021-01-21 0:36 UTC (permalink / raw) To: Sean Christopherson; +Cc: kvm, pbonzini, jmattson On 1/19/21 4:45 PM, Sean Christopherson wrote: > On Sat, Jan 16, 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 | 18 ++++++++++++------ >> 1 file changed, 12 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c >> index cb4c6ee10029..2419f392a13d 100644 >> --- a/arch/x86/kvm/svm/nested.c >> +++ b/arch/x86/kvm/svm/nested.c >> @@ -211,7 +211,8 @@ 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 kvm_vcpu *vcpu, >> + struct vmcb_control_area *control) >> { >> if ((vmcb_is_intercept(control, INTERCEPT_VMRUN)) == 0) >> return false; >> @@ -223,10 +224,15 @@ static bool nested_vmcb_check_controls(struct vmcb_control_area *control) >> !npt_enabled) >> return false; >> >> + if (!page_address_valid(vcpu, control->msrpm_base_pa)) >> + return false; >> + if (!page_address_valid(vcpu, control->iopm_base_pa)) > These checks are wrong. The MSRPM is 8kb in size, and the IOPM is 12kb, and the > APM explicitly states that the last byte is checked: > > if the address of the last byte in the table is greater than or equal to the > maximum supported physical address, this is treated as illegal VMCB state and > causes a #VMEXIT(VMEXIT_INVALID). > > KVM can't check just the last byte, as that would fail to detect a wrap of the > 64-bit boundary. Might be worth adding yet another helper? I think this will > work, though I'm sure Paolo has a much more clever solution :-) > > static inline bool page_range_valid(struct kvm_vcpu *vcpu, gpa_t gpa, int size) > { > gpa_t last_page = gpa + size - PAGE_SIZE; > > if (last_page < gpa) > return false; > > return page_address_valid(last_page); > } > > Note, the IOPM is 12kb in size, but KVM allocates and initializes 16kb, i.e. > using IOPM_ALLOC_ORDER for the check would be wrong. Maybe define the actual > size for both bitmaps and use get_order() instead of hardcoding the order? That > would make it easy to "fix" svm_hardware_setup() so that it doesn't initialize > unused memory. Is there any issues with using alloc_pages_exact() instead of alloc_pages() for allocating the IOPM bitmap ? > >> + return false; >> + >> return true; >> } >> >> -static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb12) >> +static bool nested_vmcb_checks(struct kvm_vcpu *vcpu, struct vmcb *vmcb12) >> { >> bool vmcb12_lma; >> >> @@ -255,10 +261,10 @@ static bool nested_vmcb_checks(struct vcpu_svm *svm, struct vmcb *vmcb12) >> (vmcb12->save.cr3 & MSR_CR3_LONG_MBZ_MASK)) >> return false; >> } >> - if (!kvm_is_valid_cr4(&svm->vcpu, vmcb12->save.cr4)) >> + if (!kvm_is_valid_cr4(vcpu, vmcb12->save.cr4)) >> return false; >> >> - return nested_vmcb_check_controls(&vmcb12->control); >> + return nested_vmcb_check_controls(vcpu, &vmcb12->control); >> } >> >> static void load_nested_vmcb_control(struct vcpu_svm *svm, >> @@ -485,7 +491,7 @@ int nested_svm_vmrun(struct vcpu_svm *svm) >> if (WARN_ON_ONCE(!svm->nested.initialized)) >> return -EINVAL; >> >> - if (!nested_vmcb_checks(svm, vmcb12)) { >> + if (!nested_vmcb_checks(&svm->vcpu, vmcb12)) { >> vmcb12->control.exit_code = SVM_EXIT_ERR; >> vmcb12->control.exit_code_hi = 0; >> vmcb12->control.exit_info_1 = 0; >> @@ -1173,7 +1179,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(vcpu, ctl)) >> goto out_free; >> >> /* >> -- >> 2.27.0 >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/3 v2] Test: nSVM: Test MSR and IO bitmap address 2021-01-16 2:20 [PATCH 0/3 v2] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests Krish Sadhukhan 2021-01-16 2:20 ` [PATCH 1/3 v2] nSVM: Check addresses of MSR and IO bitmap Krish Sadhukhan @ 2021-01-16 2:20 ` Krish Sadhukhan 2021-01-16 2:20 ` [PATCH 3/3 v2] Test: SVM: Use ALIGN macro when aligning 'io_bitmap_area' Krish Sadhukhan 2 siblings, 0 replies; 6+ messages in thread From: Krish Sadhukhan @ 2021-01-16 2:20 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] 6+ messages in thread
* [PATCH 3/3 v2] Test: SVM: Use ALIGN macro when aligning 'io_bitmap_area' 2021-01-16 2:20 [PATCH 0/3 v2] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests Krish Sadhukhan 2021-01-16 2:20 ` [PATCH 1/3 v2] nSVM: Check addresses of MSR and IO bitmap Krish Sadhukhan 2021-01-16 2:20 ` [PATCH 2/3 v2] Test: nSVM: Test MSR and IO bitmap address Krish Sadhukhan @ 2021-01-16 2:20 ` Krish Sadhukhan 2 siblings, 0 replies; 6+ messages in thread From: Krish Sadhukhan @ 2021-01-16 2:20 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] 6+ messages in thread
end of thread, other threads:[~2021-01-21 1:10 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-16 2:20 [PATCH 0/3 v2] KVM: nSVM: Check addresses of MSR bitmap and IO bitmap tables on vmrun of nested guests Krish Sadhukhan 2021-01-16 2:20 ` [PATCH 1/3 v2] nSVM: Check addresses of MSR and IO bitmap Krish Sadhukhan 2021-01-20 0:45 ` Sean Christopherson 2021-01-21 0:36 ` Krish Sadhukhan 2021-01-16 2:20 ` [PATCH 2/3 v2] Test: nSVM: Test MSR and IO bitmap address Krish Sadhukhan 2021-01-16 2:20 ` [PATCH 3/3 v2] 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).