| Message ID | 20190207190533.12438-5-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4,v3,nVMX] : Add a vmentry check for HOST_SYSENTER_ESP and HOST_SYSENTER_EIP fields | expand |
On Thu, Feb 07, 2019 at 02:05:33PM -0500, Krish Sadhukhan wrote: > According to section "Checks on VMX Controls" in Intel SDM vol 3C, the > following check is performed on vmentry of L2 guests: > > On processors that support Intel 64 architecture, the IA32_SYSENTER_ESP > field and the IA32_SYSENTER_EIP field must each contain a canonical > address. > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > x86/unittests.cfg | 6 ++++++ > x86/vmx_tests.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/x86/unittests.cfg b/x86/unittests.cfg > index d7975e9..580dd8c 100644 > --- a/x86/unittests.cfg > +++ b/x86/unittests.cfg > @@ -542,6 +542,12 @@ extra_params = -cpu host,+vmx -m 2560 -append vmx_controls_test > arch = x86_64 > groups = vmx > > +[vmx_host_state_area] > +file = vmx.flat > +extra_params = -cpu host,+vmx -m 2560 -append vmx_host_state_area_test > +arch = x86_64 > +groups = vmx > + > [vmx_vmentry_movss_shadow_test] > file = vmx.flat > extra_params = -cpu host,+vmx -m 2560 -append vmentry_movss_shadow_test > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > index 1637717..66a87f6 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -4971,6 +4971,47 @@ static void vmx_controls_test(void) > test_vm_entry_ctls(); > } > > +/* > + * On processors that support Intel 64 architecture, the IA32_SYSENTER_ESP > + * field and the IA32_SYSENTER_EIP field must each contain a canonical > + * address. > + * > + * [Intel SDM] > + */ > +static void test_sysenter_field(u32 field, const char *name) > +{ > + u64 addr_saved = vmcs_read(field); > + > + vmcs_write(field, NONCANONICAL); > + report_prefix_pushf("%s non-canonical", name); > + test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD, false); > + report_prefix_pop(); > + > + vmcs_write(field, 0xffffffff); > + report_prefix_pushf("%s canonical", name); > + test_vmx_vmlaunch(0, false); > + report_prefix_pop(); > + > + vmcs_write(field, addr_saved); > +} > + > +/* > + * Check that the virtual CPU checks the VMX Host State Area as > + * documented in the Intel SDM. > + */ > +static void vmx_host_state_area_test(void) > +{ > + /* > + * Bit 1 of the guest's RFLAGS must be 1, or VM-entry will > + * fail due to invalid guest state, should we make it that > + * far. > + */ > + vmcs_write(GUEST_RFLAGS, 0); Doesn't need to be done with this series, but stuffing GUEST_RFLAGS to induce a VM-Exit on VM-Entery probably belongs in vmlaunch_succeeds(), which even states: Returns true if VMLAUNCH makes it at least as far as the guest-state checks Or maybe just another helper function, putting it in vmlaunch_succeeds() would add a VMWRITE to every test. Anyways, point is that it'd be nice to not have this code and comment sprinkled throughout the tests. > + test_sysenter_field(HOST_SYSENTER_ESP, "HOST_SYSENTER_ESP"); > + test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP"); > +} > + > static bool valid_vmcs_for_vmentry(void) > { > struct vmcs *current_vmcs = NULL; > @@ -6392,6 +6433,7 @@ struct vmx_test vmx_tests[] = { > TEST(invvpid_test_v2), > /* VM-entry tests */ > TEST(vmx_controls_test), > + TEST(vmx_host_state_area_test), > TEST(vmentry_movss_shadow_test), > /* APICv tests */ > TEST(vmx_eoi_bitmap_ioapic_scan_test), > -- > 2.17.2 >
On 02/07/2019 11:51 AM, Sean Christopherson wrote: > On Thu, Feb 07, 2019 at 02:05:33PM -0500, Krish Sadhukhan wrote: >> According to section "Checks on VMX Controls" in Intel SDM vol 3C, the >> following check is performed on vmentry of L2 guests: >> >> On processors that support Intel 64 architecture, the IA32_SYSENTER_ESP >> field and the IA32_SYSENTER_EIP field must each contain a canonical >> address. >> >> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> >> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> >> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> >> --- >> x86/unittests.cfg | 6 ++++++ >> x86/vmx_tests.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 48 insertions(+) >> >> diff --git a/x86/unittests.cfg b/x86/unittests.cfg >> index d7975e9..580dd8c 100644 >> --- a/x86/unittests.cfg >> +++ b/x86/unittests.cfg >> @@ -542,6 +542,12 @@ extra_params = -cpu host,+vmx -m 2560 -append vmx_controls_test >> arch = x86_64 >> groups = vmx >> >> +[vmx_host_state_area] >> +file = vmx.flat >> +extra_params = -cpu host,+vmx -m 2560 -append vmx_host_state_area_test >> +arch = x86_64 >> +groups = vmx >> + >> [vmx_vmentry_movss_shadow_test] >> file = vmx.flat >> extra_params = -cpu host,+vmx -m 2560 -append vmentry_movss_shadow_test >> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c >> index 1637717..66a87f6 100644 >> --- a/x86/vmx_tests.c >> +++ b/x86/vmx_tests.c >> @@ -4971,6 +4971,47 @@ static void vmx_controls_test(void) >> test_vm_entry_ctls(); >> } >> >> +/* >> + * On processors that support Intel 64 architecture, the IA32_SYSENTER_ESP >> + * field and the IA32_SYSENTER_EIP field must each contain a canonical >> + * address. >> + * >> + * [Intel SDM] >> + */ >> +static void test_sysenter_field(u32 field, const char *name) >> +{ >> + u64 addr_saved = vmcs_read(field); >> + >> + vmcs_write(field, NONCANONICAL); >> + report_prefix_pushf("%s non-canonical", name); >> + test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD, false); >> + report_prefix_pop(); >> + >> + vmcs_write(field, 0xffffffff); >> + report_prefix_pushf("%s canonical", name); >> + test_vmx_vmlaunch(0, false); >> + report_prefix_pop(); >> + >> + vmcs_write(field, addr_saved); >> +} >> + >> +/* >> + * Check that the virtual CPU checks the VMX Host State Area as >> + * documented in the Intel SDM. >> + */ >> +static void vmx_host_state_area_test(void) >> +{ >> + /* >> + * Bit 1 of the guest's RFLAGS must be 1, or VM-entry will >> + * fail due to invalid guest state, should we make it that >> + * far. >> + */ >> + vmcs_write(GUEST_RFLAGS, 0); > Doesn't need to be done with this series, but stuffing GUEST_RFLAGS to > induce a VM-Exit on VM-Entery probably belongs in vmlaunch_succeeds(), > which even states: > > Returns true if VMLAUNCH makes it at least as far as the guest-state checks > That makes sense. Not sure why that vmcs_write was put outside of vmlaunch_succeeds() in the first place. I will make that change via another patch outside of this series then. > Or maybe just another helper function, putting it in vmlaunch_succeeds() > would add a VMWRITE to every test. Anyways, point is that it'd be nice > to not have this code and comment sprinkled throughout the tests. > > >> + test_sysenter_field(HOST_SYSENTER_ESP, "HOST_SYSENTER_ESP"); >> + test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP"); >> +} >> + >> static bool valid_vmcs_for_vmentry(void) >> { >> struct vmcs *current_vmcs = NULL; >> @@ -6392,6 +6433,7 @@ struct vmx_test vmx_tests[] = { >> TEST(invvpid_test_v2), >> /* VM-entry tests */ >> TEST(vmx_controls_test), >> + TEST(vmx_host_state_area_test), >> TEST(vmentry_movss_shadow_test), >> /* APICv tests */ >> TEST(vmx_eoi_bitmap_ioapic_scan_test), >> -- >> 2.17.2 >>
On Thu, Feb 07, 2019 at 02:05:33PM -0500, Krish Sadhukhan wrote: > According to section "Checks on VMX Controls" in Intel SDM vol 3C, the > following check is performed on vmentry of L2 guests: > > On processors that support Intel 64 architecture, the IA32_SYSENTER_ESP > field and the IA32_SYSENTER_EIP field must each contain a canonical > address. > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> > Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > x86/unittests.cfg | 6 ++++++ > x86/vmx_tests.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 48 insertions(+) > > diff --git a/x86/unittests.cfg b/x86/unittests.cfg > index d7975e9..580dd8c 100644 > --- a/x86/unittests.cfg > +++ b/x86/unittests.cfg > @@ -542,6 +542,12 @@ extra_params = -cpu host,+vmx -m 2560 -append vmx_controls_test > arch = x86_64 > groups = vmx > > +[vmx_host_state_area] > +file = vmx.flat > +extra_params = -cpu host,+vmx -m 2560 -append vmx_host_state_area_test This test doesn't actually need 2gb of memory, i.e. "-m 2560" isn't required. Only the ept_access_* tests allocate a 1gb page. I wouldn't hold up this series for that though, there are a bunch of other VMX tests that specify "-m 2560" when they don't need to, and in most cases the only real impact is that the tests won't run under 32-bit KVM, which is irrevelant here since this is x86_64 only. Cleaning up the issues across all VMX tests would also be a good opportunity to reduce the amount of copy+paste boilerplate in unittests.cfg. > +arch = x86_64 > +groups = vmx > + > [vmx_vmentry_movss_shadow_test] > file = vmx.flat > extra_params = -cpu host,+vmx -m 2560 -append vmentry_movss_shadow_test
diff --git a/x86/unittests.cfg b/x86/unittests.cfg index d7975e9..580dd8c 100644 --- a/x86/unittests.cfg +++ b/x86/unittests.cfg @@ -542,6 +542,12 @@ extra_params = -cpu host,+vmx -m 2560 -append vmx_controls_test arch = x86_64 groups = vmx +[vmx_host_state_area] +file = vmx.flat +extra_params = -cpu host,+vmx -m 2560 -append vmx_host_state_area_test +arch = x86_64 +groups = vmx + [vmx_vmentry_movss_shadow_test] file = vmx.flat extra_params = -cpu host,+vmx -m 2560 -append vmentry_movss_shadow_test diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c index 1637717..66a87f6 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -4971,6 +4971,47 @@ static void vmx_controls_test(void) test_vm_entry_ctls(); } +/* + * On processors that support Intel 64 architecture, the IA32_SYSENTER_ESP + * field and the IA32_SYSENTER_EIP field must each contain a canonical + * address. + * + * [Intel SDM] + */ +static void test_sysenter_field(u32 field, const char *name) +{ + u64 addr_saved = vmcs_read(field); + + vmcs_write(field, NONCANONICAL); + report_prefix_pushf("%s non-canonical", name); + test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD, false); + report_prefix_pop(); + + vmcs_write(field, 0xffffffff); + report_prefix_pushf("%s canonical", name); + test_vmx_vmlaunch(0, false); + report_prefix_pop(); + + vmcs_write(field, addr_saved); +} + +/* + * Check that the virtual CPU checks the VMX Host State Area as + * documented in the Intel SDM. + */ +static void vmx_host_state_area_test(void) +{ + /* + * Bit 1 of the guest's RFLAGS must be 1, or VM-entry will + * fail due to invalid guest state, should we make it that + * far. + */ + vmcs_write(GUEST_RFLAGS, 0); + + test_sysenter_field(HOST_SYSENTER_ESP, "HOST_SYSENTER_ESP"); + test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP"); +} + static bool valid_vmcs_for_vmentry(void) { struct vmcs *current_vmcs = NULL; @@ -6392,6 +6433,7 @@ struct vmx_test vmx_tests[] = { TEST(invvpid_test_v2), /* VM-entry tests */ TEST(vmx_controls_test), + TEST(vmx_host_state_area_test), TEST(vmentry_movss_shadow_test), /* APICv tests */ TEST(vmx_eoi_bitmap_ioapic_scan_test),
According to section "Checks on VMX Controls" in Intel SDM vol 3C, the following check is performed on vmentry of L2 guests: On processors that support Intel 64 architecture, the IA32_SYSENTER_ESP field and the IA32_SYSENTER_EIP field must each contain a canonical address. Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> Reviewed-by: Mihai Carabas <mihai.carabas@oracle.com> Suggested-by: Sean Christopherson <sean.j.christopherson@intel.com> --- x86/unittests.cfg | 6 ++++++ x86/vmx_tests.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+)