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@xxxxxxxxxx>
> Reviewed-by: Mihai Carabas <mihai.carabas@xxxxxxxxxx>
> ---
> 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
>
