Patchwork [4/4,v3,kvm-unit-test,nVMX] : Test HOST_SYSENTER_ESP and HOST_SYSENTER_EIP fields on vmentry of L2 guests

login
register
mail settings
Submitter Krish Sadhukhan
Date Feb. 7, 2019, 7:05 p.m.
Message ID <20190207190533.12438-5-krish.sadhukhan@oracle.com>
Download mbox | patch
Permalink /patch/721163/
State New
Headers show

Comments

Krish Sadhukhan - Feb. 7, 2019, 7:05 p.m.
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(+)
Christopherson, Sean J - Feb. 7, 2019, 7:51 p.m.
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
>
Krish Sadhukhan - Feb. 7, 2019, 8:27 p.m.
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
>>
Christopherson, Sean J - Feb. 12, 2019, 4:34 p.m.
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

Patch

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