| Message ID | 20190205223427.7387-5-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4,v2,nVMX] : Add a vmentry check for HOST_SYSENTER_ESP and HOST_SYSENTER_EIP fields | expand |
On Tue, Feb 05, 2019 at 05:34:27PM -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> > --- > x86/unittests.cfg | 6 ++++ > x86/vmx_tests.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 81 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] Personal preference, but I like vmx_host_state, i.e. drop "_area". To me "area" makes it sounds like were somehow testing an actual physical part of the VMCS. > +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 b69a7d9..487eb6f 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -4935,6 +4935,80 @@ 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_host_ctl_regs(void) SYSENTER_{EIP,ESP} are MSRs, not control registers. And I don't think you need this function anyways (see bottom). > +{ > + u64 addr_saved = vmcs_read(HOST_SYSENTER_ESP); > + u64 addr = addr_saved; > + > + if (!is_canonical(addr)) { Consuming unknown prior state is a big no-no for unit tests, e.g. this behavior could cause all sorts of reproducibility problems if someone adds a test that happens to write HOST_SYSENTER_ESP. The KVM unit tests don't always do a great job of sanitizing dependent state but we should at least explicitly set the field being tested. > + report_prefix_pushf("HOST_SYSENTER_ESP non-canonical"); > + test_vmlaunch(false, false, > + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); > + report_prefix_pop(); > + } else { > + report_prefix_pushf("HOST_SYSENTER_ESP canonical"); > + test_vmlaunch(true, false, > + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); > + report_prefix_pop(); > + > + addr |= 1ull << 48; This will break if 5 level paging is configured, or if @addr contains a value with the upper bits set. Write a predetermined value that is guaranteed to cause a non-canonical violation. There's even a #define for this: NONCANONICAL. > + vmcs_write(HOST_SYSENTER_ESP, addr); > + report_prefix_pushf("HOST_SYSENTER_ESP non-canonical"); > + test_vmlaunch(false, false, > + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); > + report_prefix_pop(); > + > + vmcs_write(HOST_SYSENTER_ESP, addr_saved); > + } > + > + addr_saved = vmcs_read(HOST_SYSENTER_EIP); > + addr = addr_saved; > + > + if (!is_canonical(addr)) { > + report_prefix_pushf("HOST_SYSENTER_EIP non-canonical"); > + test_vmlaunch(false, false, > + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); > + report_prefix_pop(); > + } else { > + report_prefix_pushf("HOST_SYSENTER_EIP canonical"); > + test_vmlaunch(true, false, > + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); > + report_prefix_pop(); > + > + addr |= 1ull << 48; > + vmcs_write(HOST_SYSENTER_EIP, addr); > + report_prefix_pushf("HOST_SYSENTER_EIP non-canonical"); > + test_vmlaunch(false, false, > + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); > + report_prefix_pop(); > + > + vmcs_write(HOST_SYSENTER_EIP, addr_saved); > + } This code is basically identical to HOST_SYSENTER_ESP, reusing the code via a helper is trivial, e.g.: static void test_sysenter_field(u32 field, const char *name) { u64 addr_saved = vmcs_read(field); bool success; vmcs_write(field, NONCANONICAL); report_prefix_pushf("%s non-canonical", name); success = vmlaunch_succeeds(); report("vmlaunch %s", !success, "fails"); if (!success) check_vmx_instr_err(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); report_prefix_pop(); vmcs_write(field, 0xffffffff); report_prefix_pushf("%s canonical", name); report("vmlaunch %s", success, "succeeds"); report_prefix_pop(); vmcs_write(HOST_SYSENTER_ESP, 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_host_ctl_regs(); > +} > + > static bool valid_vmcs_for_vmentry(void) > { > struct vmcs *current_vmcs = NULL; > @@ -6356,6 +6430,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/05/2019 03:50 PM, Sean Christopherson wrote: > On Tue, Feb 05, 2019 at 05:34:27PM -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> >> --- >> x86/unittests.cfg | 6 ++++ >> x86/vmx_tests.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 81 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] > Personal preference, but I like vmx_host_state, i.e. drop "_area". To me > "area" makes it sounds like were somehow testing an actual physical part > of the VMCS. The Intel SDM consistently refers to this part of the VMCS as "Host State Area". I just wanted to be consistent with their nomenclature. :-) > >> +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 b69a7d9..487eb6f 100644 >> --- a/x86/vmx_tests.c >> +++ b/x86/vmx_tests.c >> @@ -4935,6 +4935,80 @@ 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_host_ctl_regs(void) > SYSENTER_{EIP,ESP} are MSRs, not control registers. And I don't think > you need this function anyways (see bottom). > >> +{ >> + u64 addr_saved = vmcs_read(HOST_SYSENTER_ESP); >> + u64 addr = addr_saved; >> + >> + if (!is_canonical(addr)) { > Consuming unknown prior state is a big no-no for unit tests, e.g. this > behavior could cause all sorts of reproducibility problems if someone > adds a test that happens to write HOST_SYSENTER_ESP. The KVM unit tests > don't always do a great job of sanitizing dependent state but we should > at least explicitly set the field being tested. > >> + report_prefix_pushf("HOST_SYSENTER_ESP non-canonical"); >> + test_vmlaunch(false, false, >> + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); >> + report_prefix_pop(); >> + } else { >> + report_prefix_pushf("HOST_SYSENTER_ESP canonical"); >> + test_vmlaunch(true, false, >> + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); >> + report_prefix_pop(); >> + >> + addr |= 1ull << 48; > This will break if 5 level paging is configured, or if @addr contains a > value with the upper bits set. Write a predetermined value that is > guaranteed to cause a non-canonical violation. There's even a #define > for this: NONCANONICAL. > >> + vmcs_write(HOST_SYSENTER_ESP, addr); >> + report_prefix_pushf("HOST_SYSENTER_ESP non-canonical"); >> + test_vmlaunch(false, false, >> + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); >> + report_prefix_pop(); >> + >> + vmcs_write(HOST_SYSENTER_ESP, addr_saved); >> + } >> + >> + addr_saved = vmcs_read(HOST_SYSENTER_EIP); >> + addr = addr_saved; >> + >> + if (!is_canonical(addr)) { >> + report_prefix_pushf("HOST_SYSENTER_EIP non-canonical"); >> + test_vmlaunch(false, false, >> + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); >> + report_prefix_pop(); >> + } else { >> + report_prefix_pushf("HOST_SYSENTER_EIP canonical"); >> + test_vmlaunch(true, false, >> + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); >> + report_prefix_pop(); >> + >> + addr |= 1ull << 48; >> + vmcs_write(HOST_SYSENTER_EIP, addr); >> + report_prefix_pushf("HOST_SYSENTER_EIP non-canonical"); >> + test_vmlaunch(false, false, >> + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); >> + report_prefix_pop(); >> + >> + vmcs_write(HOST_SYSENTER_EIP, addr_saved); >> + } > This code is basically identical to HOST_SYSENTER_ESP, reusing the code > via a helper is trivial, e.g.: > > static void test_sysenter_field(u32 field, const char *name) > { > u64 addr_saved = vmcs_read(field); > bool success; > > vmcs_write(field, NONCANONICAL); > report_prefix_pushf("%s non-canonical", name); > success = vmlaunch_succeeds(); > report("vmlaunch %s", !success, "fails"); > if (!success) > check_vmx_instr_err(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); > report_prefix_pop(); > > vmcs_write(field, 0xffffffff); > report_prefix_pushf("%s canonical", name); > report("vmlaunch %s", success, "succeeds"); > report_prefix_pop(); > > vmcs_write(HOST_SYSENTER_ESP, 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_host_ctl_regs(); >> +} >> + >> static bool valid_vmcs_for_vmentry(void) >> { >> struct vmcs *current_vmcs = NULL; >> @@ -6356,6 +6430,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 >>
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 b69a7d9..487eb6f 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -4935,6 +4935,80 @@ 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_host_ctl_regs(void) +{ + u64 addr_saved = vmcs_read(HOST_SYSENTER_ESP); + u64 addr = addr_saved; + + if (!is_canonical(addr)) { + report_prefix_pushf("HOST_SYSENTER_ESP non-canonical"); + test_vmlaunch(false, false, + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); + report_prefix_pop(); + } else { + report_prefix_pushf("HOST_SYSENTER_ESP canonical"); + test_vmlaunch(true, false, + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); + report_prefix_pop(); + + addr |= 1ull << 48; + vmcs_write(HOST_SYSENTER_ESP, addr); + report_prefix_pushf("HOST_SYSENTER_ESP non-canonical"); + test_vmlaunch(false, false, + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); + report_prefix_pop(); + + vmcs_write(HOST_SYSENTER_ESP, addr_saved); + } + + addr_saved = vmcs_read(HOST_SYSENTER_EIP); + addr = addr_saved; + + if (!is_canonical(addr)) { + report_prefix_pushf("HOST_SYSENTER_EIP non-canonical"); + test_vmlaunch(false, false, + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); + report_prefix_pop(); + } else { + report_prefix_pushf("HOST_SYSENTER_EIP canonical"); + test_vmlaunch(true, false, + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); + report_prefix_pop(); + + addr |= 1ull << 48; + vmcs_write(HOST_SYSENTER_EIP, addr); + report_prefix_pushf("HOST_SYSENTER_EIP non-canonical"); + test_vmlaunch(false, false, + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); + report_prefix_pop(); + + vmcs_write(HOST_SYSENTER_EIP, 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_host_ctl_regs(); +} + static bool valid_vmcs_for_vmentry(void) { struct vmcs *current_vmcs = NULL; @@ -6356,6 +6430,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> --- x86/unittests.cfg | 6 ++++ x86/vmx_tests.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+)