[PATCH 0/2 v2]nVMX: Check Host Segment Registers and Descriptor Tables on vmentry of nested guests

[PATCH 0/2 v2]nVMX: Check Host Segment Registers and Descriptor Tables on vmentry of nested guests

[Krish Sadhukhan]

v1 -> v2:
        In patch# 2, make_non_canonical() has been made 'inline' to fix a
        compilation error.



Patch# 1 implements the following checks, from Intel SDM vol 3C, on
vmentry of nested guests:

   - In the selector field for each of CS, SS, DS, ES, FS, GS and TR, the
     RPL (bits 1:0) and the TI flag (bit 2) must be 0.
   - The selector fields for CS and TR cannot be 0000H.
   - The selector field for SS cannot be 0000H if the "host address-space
     size" VM-exit control is 0.
   - On processors that support Intel 64 architecture, the base-address
     fields for FS, GS and TR must contain canonical addresses.

Patch# 2 adds kvm-unit-tests for the above checks.


[PATCH 1/2] KVM nVMX: Check Host Segment Registers and Descriptor Tables on
[PATCH 2/2] kvm-unit-test nVMX: Test Host Segment Registers and Descriptor Tables on

 arch/x86/kvm/vmx/nested.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

Krish Sadhukhan (1):
      nVMX: Check Host Segment Registers and Descriptor Tables on vmentry of nested guests

 lib/x86/processor.h |   5 ++
 x86/vmx_tests.c     | 159 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 164 insertions(+)

Krish Sadhukhan (1):
      nVMX: Test Host Segment Registers and Descriptor Tables on vmentry of nested guests

[PATCH 1/2 v2] KVM nVMX: Check Host Segment Registers and Descriptor Tables on vmentry of nested guests

[Krish Sadhukhan]

According to section "Checks on Host Segment and Descriptor-Table
Registers" in Intel SDM vol 3C, the following checks are performed on
vmentry of nested guests:

   - In the selector field for each of CS, SS, DS, ES, FS, GS and TR, the
     RPL (bits 1:0) and the TI flag (bit 2) must be 0.
   - The selector fields for CS and TR cannot be 0000H.
   - The selector field for SS cannot be 0000H if the "host address-space
     size" VM-exit control is 0.
   - On processors that support Intel 64 architecture, the base-address
     fields for FS, GS and TR must contain canonical addresses.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 arch/x86/kvm/vmx/nested.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index f1a69117ac0f..856a83aa42f5 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2609,6 +2609,30 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 	    !kvm_pat_valid(vmcs12->host_ia32_pat))
 		return -EINVAL;
 
+	ia32e = (vmcs12->vm_exit_controls &
+		 VM_EXIT_HOST_ADDR_SPACE_SIZE) != 0;
+
+	if (vmcs12->host_cs_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
+	    vmcs12->host_ss_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
+	    vmcs12->host_ds_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
+	    vmcs12->host_es_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
+	    vmcs12->host_fs_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
+	    vmcs12->host_gs_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
+	    vmcs12->host_tr_selector & (SEGMENT_RPL_MASK | SEGMENT_TI_MASK) ||
+	    vmcs12->host_cs_selector == 0 ||
+	    vmcs12->host_tr_selector == 0 ||
+	    (vmcs12->host_ss_selector == 0 && !ia32e))
+		return -EINVAL;
+
+#ifdef CONFIG_X86_64
+	if (is_noncanonical_address(vmcs12->host_fs_base, vcpu) ||
+	    is_noncanonical_address(vmcs12->host_gs_base, vcpu) ||
+	    is_noncanonical_address(vmcs12->host_gdtr_base, vcpu) ||
+	    is_noncanonical_address(vmcs12->host_idtr_base, vcpu) ||
+	    is_noncanonical_address(vmcs12->host_tr_base, vcpu))
+		return -EINVAL;
+#endif
+
 	/*
 	 * If the load IA32_EFER VM-exit control is 1, bits reserved in the
 	 * IA32_EFER MSR must be 0 in the field for that register. In addition,
@@ -2616,8 +2640,6 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu,
 	 * the host address-space size VM-exit control.
 	 */
 	if (vmcs12->vm_exit_controls & VM_EXIT_LOAD_IA32_EFER) {
-		ia32e = (vmcs12->vm_exit_controls &
-			 VM_EXIT_HOST_ADDR_SPACE_SIZE) != 0;
 		if (!kvm_valid_efer(vcpu, vmcs12->host_ia32_efer) ||
 		    ia32e != !!(vmcs12->host_ia32_efer & EFER_LMA) ||
 		    ia32e != !!(vmcs12->host_ia32_efer & EFER_LME))
-- 
2.20.1

[PATCH 2/2 v2]kvm-unit-test: nVMX: Test Host Segment Registers and Descriptor Tables on vmentry of nested guests

[Krish Sadhukhan]

According to section "Checks on Host Segment and Descriptor-Table
Registers" in Intel SDM vol 3C, the following checks are performed on
vmentry of nested guests:

    - In the selector field for each of CS, SS, DS, ES, FS, GS and TR, the
      RPL (bits 1:0) and the TI flag (bit 2) must be 0.
    - The selector fields for CS and TR cannot be 0000H.
    - The selector field for SS cannot be 0000H if the "host address-space
      size" VM-exit control is 0.
    - On processors that support Intel 64 architecture, the base-address
      fields for FS, GS, GDTR, IDTR, and TR must contain canonical
      addresses.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
---
 lib/x86/processor.h |   5 ++
 x86/vmx_tests.c     | 159 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 164 insertions(+)

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 4fef0bc..8b8bb7a 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -461,6 +461,11 @@ static inline void write_pkru(u32 pkru)
         : : "a" (eax), "c" (ecx), "d" (edx));
 }
 
+static inline u64 make_non_canonical(u64 addr)
+{
+	return (addr | 1ull << 48);
+}
+
 static inline bool is_canonical(u64 addr)
 {
 	return (s64)(addr << 16) >> 16 == addr;
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index b50d858..5911a60 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -6938,6 +6938,163 @@ static void test_load_host_pat(void)
 	test_pat(HOST_PAT, "HOST_PAT", EXI_CONTROLS, EXI_LOAD_PAT);
 }
 
+/*
+ * Test a value for the given VMCS field.
+ *
+ *  "field" - VMCS field
+ *  "field_name" - string name of VMCS field
+ *  "bit_start" - starting bit
+ *  "bit_end" - ending bit
+ *  "val" - value that the bit range must or must not contain
+ *  "valid_val" - whether value given in 'val' must be valid or not
+ *  "error" - expected VMCS error when vmentry fails for an invalid value
+ */
+static void test_vmcs_field(u64 field, const char *field_name, u32 bit_start,
+			    u32 bit_end, u64 val, bool valid_val, u32 error)
+{
+	u64 field_saved = vmcs_read(field);
+	u32 i;
+	u64 tmp;
+	u32 bit_on;
+	u64 mask = ~0ull;
+
+	mask = (mask >> bit_end) << bit_end;
+	mask = mask | ((1 << bit_start) - 1);
+	tmp = (field_saved & mask) | (val << bit_start);
+
+	vmcs_write(field, tmp);
+	report_prefix_pushf("%s %lx", field_name, tmp);
+	if (valid_val)
+		test_vmx_vmlaunch(0, false);
+	else
+		test_vmx_vmlaunch(error, false);
+	report_prefix_pop();
+
+	for (i = bit_start; i <= bit_end; i = i + 2) {
+		bit_on = ((1ull < i) & (val << bit_start)) ? 0 : 1;
+		if (bit_on)
+			tmp = field_saved | (1ull << i);
+		else
+			tmp = field_saved & ~(1ull << i);
+		vmcs_write(field, tmp);
+		report_prefix_pushf("%s %lx", field_name, tmp);
+		if (valid_val)
+			test_vmx_vmlaunch(error, false);
+		else
+			test_vmx_vmlaunch(0, false);
+		report_prefix_pop();
+	}
+
+	vmcs_write(field, field_saved);
+}
+
+static void test_canonical(u64 field, const char * field_name)
+{
+	u64 addr_saved = vmcs_read(field);
+	u64 addr = addr_saved;
+
+	report_prefix_pushf("%s %lx", field_name, addr);
+	if (is_canonical(addr)) {
+		test_vmx_vmlaunch(0, false);
+		report_prefix_pop();
+
+		addr = make_non_canonical(addr);
+		vmcs_write(field, addr);
+		report_prefix_pushf("%s %lx", field_name, addr);
+		test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
+				  false);
+
+		vmcs_write(field, addr_saved);
+	} else {
+		test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
+				  false);
+	}
+	report_prefix_pop();
+}
+
+/*
+ * 1. In the selector field for each of CS, SS, DS, ES, FS, GS and TR, the
+ *    RPL (bits 1:0) and the TI flag (bit 2) must be 0.
+ * 2. The selector fields for CS and TR cannot be 0000H.
+ * 3. The selector field for SS cannot be 0000H if the "host address-space
+ *    size" VM-exit control is 0.
+ * 4. On processors that support Intel 64 architecture, the base-address
+ *    fields for FS, GS and TR must contain canonical addresses.
+ */
+static void test_host_segment_regs(void)
+{
+	u32 exit_ctrl_saved = vmcs_read(EXI_CONTROLS);
+	u16 selector_saved;
+
+	/*
+	 * Test RPL and TI flags
+	 */
+	test_vmcs_field(HOST_SEL_CS, "HOST_SEL_CS", 0, 2, 0x0, true,
+		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+	test_vmcs_field(HOST_SEL_SS, "HOST_SEL_SS", 0, 2, 0x0, true,
+		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+	test_vmcs_field(HOST_SEL_DS, "HOST_SEL_DS", 0, 2, 0x0, true,
+		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+	test_vmcs_field(HOST_SEL_ES, "HOST_SEL_ES", 0, 2, 0x0, true,
+		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+	test_vmcs_field(HOST_SEL_FS, "HOST_SEL_FS", 0, 2, 0x0, true,
+		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+	test_vmcs_field(HOST_SEL_GS, "HOST_SEL_GS", 0, 2, 0x0, true,
+		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+	test_vmcs_field(HOST_SEL_TR, "HOST_SEL_TR", 0, 2, 0x0, true,
+		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+
+	/*
+	 * Test that CS and TR fields can not be 0x0000
+	 */
+	test_vmcs_field(HOST_SEL_CS, "HOST_SEL_CS", 3, 15, 0x0000, false,
+			     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+	test_vmcs_field(HOST_SEL_TR, "HOST_SEL_TR", 3, 15, 0x0000, false,
+			     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
+
+	/*
+	 * SS field can not be 0x0000 if "host address-space size" VM-exit
+	 * control is 0
+	 */
+	selector_saved = vmcs_read(HOST_SEL_SS);
+	vmcs_write(HOST_SEL_SS, 0);
+	if (exit_ctrl_saved & EXI_HOST_64) {
+		report_prefix_pushf("HOST_SEL_SS 0");
+		test_vmx_vmlaunch(0, false);
+		report_prefix_pop();
+
+		vmcs_write(EXI_CONTROLS, exit_ctrl_saved & ~EXI_HOST_64);
+	}
+
+	report_prefix_pushf("HOST_SEL_SS 0");
+	test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD, false);
+	report_prefix_pop();
+
+	vmcs_write(HOST_SEL_SS, selector_saved);
+	vmcs_write(EXI_CONTROLS, exit_ctrl_saved);
+
+#ifdef __x86_64__
+	/*
+	 * Base address for FS, GS and TR must be canonical
+	 */
+	test_canonical(HOST_BASE_FS, "HOST_BASE_FS");
+	test_canonical(HOST_BASE_GS, "HOST_BASE_GS");
+	test_canonical(HOST_BASE_TR, "HOST_BASE_TR");
+#endif
+}
+
+/*
+ *  On processors that support Intel 64 architecture, the base-address
+ *  fields for GDTR and IDTR must contain canonical addresses.
+ */
+static void test_host_desc_tables(void)
+{
+#ifdef __x86_64__
+	test_canonical(HOST_BASE_GDTR, "HOST_BASE_GDTR");
+	test_canonical(HOST_BASE_IDTR, "HOST_BASE_IDTR");
+#endif
+}
+
 /*
  * Check that the virtual CPU checks the VMX Host State Area as
  * documented in the Intel SDM.
@@ -6958,6 +7115,8 @@ static void vmx_host_state_area_test(void)
 
 	test_host_efer();
 	test_load_host_pat();
+	test_host_segment_regs();
+	test_host_desc_tables();
 }
 
 /*
-- 
2.20.1

Re: [PATCH 2/2 v2]kvm-unit-test: nVMX: Test Host Segment Registers and Descriptor Tables on vmentry of nested guests

[Nadav Amit]

> On Jul 3, 2019, at 4:54 PM, Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
> 
> According to section "Checks on Host Segment and Descriptor-Table
> Registers" in Intel SDM vol 3C, the following checks are performed on
> vmentry of nested guests:
> 
>    - In the selector field for each of CS, SS, DS, ES, FS, GS and TR, the
>      RPL (bits 1:0) and the TI flag (bit 2) must be 0.
>    - The selector fields for CS and TR cannot be 0000H.
>    - The selector field for SS cannot be 0000H if the "host address-space
>      size" VM-exit control is 0.
>    - On processors that support Intel 64 architecture, the base-address
>      fields for FS, GS, GDTR, IDTR, and TR must contain canonical
>      addresses.

As I noted on v1, this patch causes the test to fail on bare-metal:

 FAIL: HOST_SEL_SS 0: VMX inst error is 8 (actual 7)

I don’t know what the root-cause is, but I don't think that tests that
fail on bare-metal (excluding because of CPU errata) should be included.

Re: [PATCH 2/2 v2]kvm-unit-test: nVMX: Test Host Segment Registers and Descriptor Tables on vmentry of nested guests

[Sean Christopherson]

On Sun, Jul 21, 2019 at 11:26:24AM -0700, Nadav Amit wrote:
> > On Jul 3, 2019, at 4:54 PM, Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
> > 
> > According to section "Checks on Host Segment and Descriptor-Table
> > Registers" in Intel SDM vol 3C, the following checks are performed on
> > vmentry of nested guests:
> > 
> >    - In the selector field for each of CS, SS, DS, ES, FS, GS and TR, the
> >      RPL (bits 1:0) and the TI flag (bit 2) must be 0.
> >    - The selector fields for CS and TR cannot be 0000H.
> >    - The selector field for SS cannot be 0000H if the "host address-space
> >      size" VM-exit control is 0.
> >    - On processors that support Intel 64 architecture, the base-address
> >      fields for FS, GS, GDTR, IDTR, and TR must contain canonical
> >      addresses.
> 
> As I noted on v1, this patch causes the test to fail on bare-metal:
> 
>  FAIL: HOST_SEL_SS 0: VMX inst error is 8 (actual 7)
> 
> I don’t know what the root-cause is, but I don't think that tests that
> fail on bare-metal (excluding because of CPU errata) should be included.

A 64-bit VMM isn't allowed to transition to 32-bit mode by way of VM-Exit,
and the VMX tests are 64-bit only.

  If the logical processor is in IA-32e mode (if IA32_EFER.LMA=1) at the
  time of VM entry, the "host address space size" VM-exit control must be 1.

Re: [PATCH 2/2 v2]kvm-unit-test: nVMX: Test Host Segment Registers and Descriptor Tables on vmentry of nested guests

[Sean Christopherson]

On Wed, Jul 03, 2019 at 07:54:36PM -0400, Krish Sadhukhan wrote:
> According to section "Checks on Host Segment and Descriptor-Table
> Registers" in Intel SDM vol 3C, the following checks are performed on
> vmentry of nested guests:
> 
>     - In the selector field for each of CS, SS, DS, ES, FS, GS and TR, the
>       RPL (bits 1:0) and the TI flag (bit 2) must be 0.
>     - The selector fields for CS and TR cannot be 0000H.
>     - The selector field for SS cannot be 0000H if the "host address-space
>       size" VM-exit control is 0.
>     - On processors that support Intel 64 architecture, the base-address
>       fields for FS, GS, GDTR, IDTR, and TR must contain canonical
>       addresses.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
> ---
>  lib/x86/processor.h |   5 ++
>  x86/vmx_tests.c     | 159 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 164 insertions(+)
> 
> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> index 4fef0bc..8b8bb7a 100644
> --- a/lib/x86/processor.h
> +++ b/lib/x86/processor.h
> @@ -461,6 +461,11 @@ static inline void write_pkru(u32 pkru)
>          : : "a" (eax), "c" (ecx), "d" (edx));
>  }
>  
> +static inline u64 make_non_canonical(u64 addr)
> +{
> +	return (addr | 1ull << 48);

This isn't guaranteed to work.  It assumes a 48-bit address space and
also assumes addr is in the lower half of the address space.  In fact, I'm
feeling a bit of deja vu...

https://patchwork.kernel.org/patch/10798645/#22464371

> +}
> +
>  static inline bool is_canonical(u64 addr)
>  {
>  	return (s64)(addr << 16) >> 16 == addr;
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index b50d858..5911a60 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -6938,6 +6938,163 @@ static void test_load_host_pat(void)
>  	test_pat(HOST_PAT, "HOST_PAT", EXI_CONTROLS, EXI_LOAD_PAT);
>  }
>  
> +/*
> + * Test a value for the given VMCS field.
> + *
> + *  "field" - VMCS field
> + *  "field_name" - string name of VMCS field
> + *  "bit_start" - starting bit
> + *  "bit_end" - ending bit
> + *  "val" - value that the bit range must or must not contain
> + *  "valid_val" - whether value given in 'val' must be valid or not
> + *  "error" - expected VMCS error when vmentry fails for an invalid value

Comments are great, but they should use kernel-doc style.

> + */
> +static void test_vmcs_field(u64 field, const char *field_name, u32 bit_start,
> +			    u32 bit_end, u64 val, bool valid_val, u32 error)
> +{
> +	u64 field_saved = vmcs_read(field);
> +	u32 i;
> +	u64 tmp;
> +	u32 bit_on;
> +	u64 mask = ~0ull;
> +
> +	mask = (mask >> bit_end) << bit_end;
> +	mask = mask | ((1 << bit_start) - 1);
> +	tmp = (field_saved & mask) | (val << bit_start);
> +
> +	vmcs_write(field, tmp);
> +	report_prefix_pushf("%s %lx", field_name, tmp);
> +	if (valid_val)
> +		test_vmx_vmlaunch(0, false);
> +	else
> +		test_vmx_vmlaunch(error, false);
> +	report_prefix_pop();
> +
> +	for (i = bit_start; i <= bit_end; i = i + 2) {
> +		bit_on = ((1ull < i) & (val << bit_start)) ? 0 : 1;
> +		if (bit_on)
> +			tmp = field_saved | (1ull << i);
> +		else
> +			tmp = field_saved & ~(1ull << i);
> +		vmcs_write(field, tmp);
> +		report_prefix_pushf("%s %lx", field_name, tmp);
> +		if (valid_val)
> +			test_vmx_vmlaunch(error, false);
> +		else
> +			test_vmx_vmlaunch(0, false);
> +		report_prefix_pop();
> +	}
> +
> +	vmcs_write(field, field_saved);
> +}
> +
> +static void test_canonical(u64 field, const char * field_name)
> +{
> +	u64 addr_saved = vmcs_read(field);
> +	u64 addr = addr_saved;
> +
> +	report_prefix_pushf("%s %lx", field_name, addr);
> +	if (is_canonical(addr)) {
> +		test_vmx_vmlaunch(0, false);
> +		report_prefix_pop();
> +
> +		addr = make_non_canonical(addr);
> +		vmcs_write(field, addr);
> +		report_prefix_pushf("%s %lx", field_name, addr);
> +		test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
> +				  false);
> +
> +		vmcs_write(field, addr_saved);
> +	} else {
> +		test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
> +				  false);
> +	}
> +	report_prefix_pop();
> +}
> +
> +/*
> + * 1. In the selector field for each of CS, SS, DS, ES, FS, GS and TR, the
> + *    RPL (bits 1:0) and the TI flag (bit 2) must be 0.
> + * 2. The selector fields for CS and TR cannot be 0000H.
> + * 3. The selector field for SS cannot be 0000H if the "host address-space
> + *    size" VM-exit control is 0.
> + * 4. On processors that support Intel 64 architecture, the base-address
> + *    fields for FS, GS and TR must contain canonical addresses.
> + */
> +static void test_host_segment_regs(void)
> +{
> +	u32 exit_ctrl_saved = vmcs_read(EXI_CONTROLS);
> +	u16 selector_saved;
> +
> +	/*
> +	 * Test RPL and TI flags
> +	 */
> +	test_vmcs_field(HOST_SEL_CS, "HOST_SEL_CS", 0, 2, 0x0, true,
> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> +	test_vmcs_field(HOST_SEL_SS, "HOST_SEL_SS", 0, 2, 0x0, true,
> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> +	test_vmcs_field(HOST_SEL_DS, "HOST_SEL_DS", 0, 2, 0x0, true,
> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> +	test_vmcs_field(HOST_SEL_ES, "HOST_SEL_ES", 0, 2, 0x0, true,
> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> +	test_vmcs_field(HOST_SEL_FS, "HOST_SEL_FS", 0, 2, 0x0, true,
> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> +	test_vmcs_field(HOST_SEL_GS, "HOST_SEL_GS", 0, 2, 0x0, true,
> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> +	test_vmcs_field(HOST_SEL_TR, "HOST_SEL_TR", 0, 2, 0x0, true,
> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);

A few layered helpers would reduce the boilerplate code by a substantial
amount.

> +
> +	/*
> +	 * Test that CS and TR fields can not be 0x0000
> +	 */
> +	test_vmcs_field(HOST_SEL_CS, "HOST_SEL_CS", 3, 15, 0x0000, false,
> +			     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> +	test_vmcs_field(HOST_SEL_TR, "HOST_SEL_TR", 3, 15, 0x0000, false,
> +			     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> +
> +	/*
> +	 * SS field can not be 0x0000 if "host address-space size" VM-exit
> +	 * control is 0
> +	 */

As Nadav pointed out, this test is broken as a 64-bit host must configure
VM-Exit to return to 64-bit mode.

> +	selector_saved = vmcs_read(HOST_SEL_SS);
> +	vmcs_write(HOST_SEL_SS, 0);
> +	if (exit_ctrl_saved & EXI_HOST_64) {
> +		report_prefix_pushf("HOST_SEL_SS 0");
> +		test_vmx_vmlaunch(0, false);
> +		report_prefix_pop();
> +
> +		vmcs_write(EXI_CONTROLS, exit_ctrl_saved & ~EXI_HOST_64);
> +	}
> +
> +	report_prefix_pushf("HOST_SEL_SS 0");
> +	test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD, false);
> +	report_prefix_pop();
> +
> +	vmcs_write(HOST_SEL_SS, selector_saved);
> +	vmcs_write(EXI_CONTROLS, exit_ctrl_saved);
> +
> +#ifdef __x86_64__

Unnecessary, the VMX tests are 64-bit only.  Adding 32-bit support would
require a massive rewrite, i.e. this would be the least of our problems.

> +	/*
> +	 * Base address for FS, GS and TR must be canonical
> +	 */
> +	test_canonical(HOST_BASE_FS, "HOST_BASE_FS");
> +	test_canonical(HOST_BASE_GS, "HOST_BASE_GS");
> +	test_canonical(HOST_BASE_TR, "HOST_BASE_TR");
> +#endif
> +}
> +
> +/*
> + *  On processors that support Intel 64 architecture, the base-address
> + *  fields for GDTR and IDTR must contain canonical addresses.
> + */
> +static void test_host_desc_tables(void)
> +{
> +#ifdef __x86_64__
> +	test_canonical(HOST_BASE_GDTR, "HOST_BASE_GDTR");
> +	test_canonical(HOST_BASE_IDTR, "HOST_BASE_IDTR");
> +#endif
> +}
> +
>  /*
>   * Check that the virtual CPU checks the VMX Host State Area as
>   * documented in the Intel SDM.
> @@ -6958,6 +7115,8 @@ static void vmx_host_state_area_test(void)
>  
>  	test_host_efer();
>  	test_load_host_pat();
> +	test_host_segment_regs();
> +	test_host_desc_tables();
>  }
>  
>  /*
> -- 
> 2.20.1
> 

Re: [PATCH 2/2 v2]kvm-unit-test: nVMX: Test Host Segment Registers and Descriptor Tables on vmentry of nested guests

[Krish Sadhukhan]

On 07/24/2019 09:12 AM, Sean Christopherson wrote:
> On Wed, Jul 03, 2019 at 07:54:36PM -0400, Krish Sadhukhan wrote:
>> According to section "Checks on Host Segment and Descriptor-Table
>> Registers" in Intel SDM vol 3C, the following checks are performed on
>> vmentry of nested guests:
>>
>>      - In the selector field for each of CS, SS, DS, ES, FS, GS and TR, the
>>        RPL (bits 1:0) and the TI flag (bit 2) must be 0.
>>      - The selector fields for CS and TR cannot be 0000H.
>>      - The selector field for SS cannot be 0000H if the "host address-space
>>        size" VM-exit control is 0.
>>      - On processors that support Intel 64 architecture, the base-address
>>        fields for FS, GS, GDTR, IDTR, and TR must contain canonical
>>        addresses.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com>
>> ---
>>   lib/x86/processor.h |   5 ++
>>   x86/vmx_tests.c     | 159 ++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 164 insertions(+)
>>
>> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
>> index 4fef0bc..8b8bb7a 100644
>> --- a/lib/x86/processor.h
>> +++ b/lib/x86/processor.h
>> @@ -461,6 +461,11 @@ static inline void write_pkru(u32 pkru)
>>           : : "a" (eax), "c" (ecx), "d" (edx));
>>   }
>>   
>> +static inline u64 make_non_canonical(u64 addr)
>> +{
>> +	return (addr | 1ull << 48);
> This isn't guaranteed to work.  It assumes a 48-bit address space and
> also assumes addr is in the lower half of the address space.  In fact, I'm
> feeling a bit of deja vu...
>
> https://patchwork.kernel.org/patch/10798645/#22464371

Yes, I should have used NONCANONICAL instead of creating a new 
function.  My bad !  Will fix it.

>
>> +}
>> +
>>   static inline bool is_canonical(u64 addr)
>>   {
>>   	return (s64)(addr << 16) >> 16 == addr;
>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> index b50d858..5911a60 100644
>> --- a/x86/vmx_tests.c
>> +++ b/x86/vmx_tests.c
>> @@ -6938,6 +6938,163 @@ static void test_load_host_pat(void)
>>   	test_pat(HOST_PAT, "HOST_PAT", EXI_CONTROLS, EXI_LOAD_PAT);
>>   }
>>   
>> +/*
>> + * Test a value for the given VMCS field.
>> + *
>> + *  "field" - VMCS field
>> + *  "field_name" - string name of VMCS field
>> + *  "bit_start" - starting bit
>> + *  "bit_end" - ending bit
>> + *  "val" - value that the bit range must or must not contain
>> + *  "valid_val" - whether value given in 'val' must be valid or not
>> + *  "error" - expected VMCS error when vmentry fails for an invalid value
> Comments are great, but they should use kernel-doc style.

Will fix it.

>
>> + */
>> +static void test_vmcs_field(u64 field, const char *field_name, u32 bit_start,
>> +			    u32 bit_end, u64 val, bool valid_val, u32 error)
>> +{
>> +	u64 field_saved = vmcs_read(field);
>> +	u32 i;
>> +	u64 tmp;
>> +	u32 bit_on;
>> +	u64 mask = ~0ull;
>> +
>> +	mask = (mask >> bit_end) << bit_end;
>> +	mask = mask | ((1 << bit_start) - 1);
>> +	tmp = (field_saved & mask) | (val << bit_start);
>> +
>> +	vmcs_write(field, tmp);
>> +	report_prefix_pushf("%s %lx", field_name, tmp);
>> +	if (valid_val)
>> +		test_vmx_vmlaunch(0, false);
>> +	else
>> +		test_vmx_vmlaunch(error, false);
>> +	report_prefix_pop();
>> +
>> +	for (i = bit_start; i <= bit_end; i = i + 2) {
>> +		bit_on = ((1ull < i) & (val << bit_start)) ? 0 : 1;
>> +		if (bit_on)
>> +			tmp = field_saved | (1ull << i);
>> +		else
>> +			tmp = field_saved & ~(1ull << i);
>> +		vmcs_write(field, tmp);
>> +		report_prefix_pushf("%s %lx", field_name, tmp);
>> +		if (valid_val)
>> +			test_vmx_vmlaunch(error, false);
>> +		else
>> +			test_vmx_vmlaunch(0, false);
>> +		report_prefix_pop();
>> +	}
>> +
>> +	vmcs_write(field, field_saved);
>> +}
>> +
>> +static void test_canonical(u64 field, const char * field_name)
>> +{
>> +	u64 addr_saved = vmcs_read(field);
>> +	u64 addr = addr_saved;
>> +
>> +	report_prefix_pushf("%s %lx", field_name, addr);
>> +	if (is_canonical(addr)) {
>> +		test_vmx_vmlaunch(0, false);
>> +		report_prefix_pop();
>> +
>> +		addr = make_non_canonical(addr);
>> +		vmcs_write(field, addr);
>> +		report_prefix_pushf("%s %lx", field_name, addr);
>> +		test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
>> +				  false);
>> +
>> +		vmcs_write(field, addr_saved);
>> +	} else {
>> +		test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD,
>> +				  false);
>> +	}
>> +	report_prefix_pop();
>> +}
>> +
>> +/*
>> + * 1. In the selector field for each of CS, SS, DS, ES, FS, GS and TR, the
>> + *    RPL (bits 1:0) and the TI flag (bit 2) must be 0.
>> + * 2. The selector fields for CS and TR cannot be 0000H.
>> + * 3. The selector field for SS cannot be 0000H if the "host address-space
>> + *    size" VM-exit control is 0.
>> + * 4. On processors that support Intel 64 architecture, the base-address
>> + *    fields for FS, GS and TR must contain canonical addresses.
>> + */
>> +static void test_host_segment_regs(void)
>> +{
>> +	u32 exit_ctrl_saved = vmcs_read(EXI_CONTROLS);
>> +	u16 selector_saved;
>> +
>> +	/*
>> +	 * Test RPL and TI flags
>> +	 */
>> +	test_vmcs_field(HOST_SEL_CS, "HOST_SEL_CS", 0, 2, 0x0, true,
>> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
>> +	test_vmcs_field(HOST_SEL_SS, "HOST_SEL_SS", 0, 2, 0x0, true,
>> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
>> +	test_vmcs_field(HOST_SEL_DS, "HOST_SEL_DS", 0, 2, 0x0, true,
>> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
>> +	test_vmcs_field(HOST_SEL_ES, "HOST_SEL_ES", 0, 2, 0x0, true,
>> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
>> +	test_vmcs_field(HOST_SEL_FS, "HOST_SEL_FS", 0, 2, 0x0, true,
>> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
>> +	test_vmcs_field(HOST_SEL_GS, "HOST_SEL_GS", 0, 2, 0x0, true,
>> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
>> +	test_vmcs_field(HOST_SEL_TR, "HOST_SEL_TR", 0, 2, 0x0, true,
>> +		     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
> A few layered helpers would reduce the boilerplate code by a substantial
> amount.
>
>> +
>> +	/*
>> +	 * Test that CS and TR fields can not be 0x0000
>> +	 */
>> +	test_vmcs_field(HOST_SEL_CS, "HOST_SEL_CS", 3, 15, 0x0000, false,
>> +			     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
>> +	test_vmcs_field(HOST_SEL_TR, "HOST_SEL_TR", 3, 15, 0x0000, false,
>> +			     VMXERR_ENTRY_INVALID_HOST_STATE_FIELD);
>> +
>> +	/*
>> +	 * SS field can not be 0x0000 if "host address-space size" VM-exit
>> +	 * control is 0
>> +	 */
> As Nadav pointed out, this test is broken as a 64-bit host must configure
> VM-Exit to return to 64-bit mode.

That means any vmentry rule that requires setting "host address space 
size" VM-exit control to zero, can not be really tested !   I will 
remove this test then.

As an aside,  I am curious to know why the hardware threw "invalid 
VMX-control" error on bare-metal.  My idea was that since the hardware 
checks VMX controls before Host State and since bit# 9 in VMX 
Exit-control is unset, we are seeing "invalid VMX-control" error instead 
of "invalid Host State" error.

>
>> +	selector_saved = vmcs_read(HOST_SEL_SS);
>> +	vmcs_write(HOST_SEL_SS, 0);
>> +	if (exit_ctrl_saved & EXI_HOST_64) {
>> +		report_prefix_pushf("HOST_SEL_SS 0");
>> +		test_vmx_vmlaunch(0, false);
>> +		report_prefix_pop();
>> +
>> +		vmcs_write(EXI_CONTROLS, exit_ctrl_saved & ~EXI_HOST_64);
>> +	}
>> +
>> +	report_prefix_pushf("HOST_SEL_SS 0");
>> +	test_vmx_vmlaunch(VMXERR_ENTRY_INVALID_HOST_STATE_FIELD, false);
>> +	report_prefix_pop();
>> +
>> +	vmcs_write(HOST_SEL_SS, selector_saved);
>> +	vmcs_write(EXI_CONTROLS, exit_ctrl_saved);
>> +
>> +#ifdef __x86_64__
> Unnecessary, the VMX tests are 64-bit only.  Adding 32-bit support would
> require a massive rewrite, i.e. this would be the least of our problems.

We also have the following in the vmx test:

#ifdef __x86_64__
extern void insn_cr8_load(void);
extern void insn_cr8_store(void);
#endif


Is this unnecessary also ?

>
>> +	/*
>> +	 * Base address for FS, GS and TR must be canonical
>> +	 */
>> +	test_canonical(HOST_BASE_FS, "HOST_BASE_FS");
>> +	test_canonical(HOST_BASE_GS, "HOST_BASE_GS");
>> +	test_canonical(HOST_BASE_TR, "HOST_BASE_TR");
>> +#endif
>> +}
>> +
>> +/*
>> + *  On processors that support Intel 64 architecture, the base-address
>> + *  fields for GDTR and IDTR must contain canonical addresses.
>> + */
>> +static void test_host_desc_tables(void)
>> +{
>> +#ifdef __x86_64__
>> +	test_canonical(HOST_BASE_GDTR, "HOST_BASE_GDTR");
>> +	test_canonical(HOST_BASE_IDTR, "HOST_BASE_IDTR");
>> +#endif
>> +}
>> +
>>   /*
>>    * Check that the virtual CPU checks the VMX Host State Area as
>>    * documented in the Intel SDM.
>> @@ -6958,6 +7115,8 @@ static void vmx_host_state_area_test(void)
>>   
>>   	test_host_efer();
>>   	test_load_host_pat();
>> +	test_host_segment_regs();
>> +	test_host_desc_tables();
>>   }
>>   
>>   /*
>> -- 
>> 2.20.1
>>

Re: [PATCH 2/2 v2]kvm-unit-test: nVMX: Test Host Segment Registers and Descriptor Tables on vmentry of nested guests

[Jim Mattson]

On Thu, Jul 25, 2019 at 3:32 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
> As an aside,  I am curious to know why the hardware threw "invalid
> VMX-control" error on bare-metal.  My idea was that since the hardware
> checks VMX controls before Host State and since bit# 9 in VMX
> Exit-control is unset, we are seeing "invalid VMX-control" error instead
> of "invalid Host State" error.

See the SDM, volume 3, 26.2 CHECKS ON VMX CONTROLS AND HOST-STATE AREA:

These checks may be performed in any order. Thus, an indication by
error number of one cause (for example, host state) does not imply
that there are not also other errors.