[4/4,v2,kvm-unit-test,nVMX] : Test HOST_SYSENTER_ESP and HOST_SYSENTER_EIP fields on vmentry of L2 guests
diff mbox series

Message ID 20190205223427.7387-5-krish.sadhukhan@oracle.com
State New
Headers show
Series
  • [1/4,v2,nVMX] : Add a vmentry check for HOST_SYSENTER_ESP and HOST_SYSENTER_EIP fields
Related show

Commit Message

Krish Sadhukhan Feb. 5, 2019, 10:34 p.m. UTC
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(+)

Comments

Sean Christopherson Feb. 5, 2019, 11:50 p.m. UTC | #1
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
>
Krish Sadhukhan Feb. 6, 2019, 2:28 a.m. UTC | #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
>>

Patch
diff mbox series

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),