[PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers

[PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers

[Krish Sadhukhan]

Even thought today's x86 hardware uses paging and not segmentation for memory
management, it is still good to have some tests that can verify the sanity of
the segment register fields on vmentry of nested guests.

The test on SS Selector field is failing because the hardware (I am using
Intel Xeon Platinum 8167M 2.00GHz) doesn't raise any error even if the
prescribed bit pattern is not set and as a result vmentry succeeds.


[PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment

 lib/x86/processor.h |   1 +
 x86/vmx_tests.c     | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+)

Krish Sadhukhan (1):
      nVMX: Test Selector and Base Address fields of Guest Segment Registers on

[PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Krish Sadhukhan]

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

    Selector fields:
	— TR. The TI flag (bit 2) must be 0.
	— LDTR. If LDTR is usable, the TI flag (bit 2) must be 0.
	— SS. If the guest will not be virtual-8086 and the "unrestricted
	  guest" VM-execution control is 0, the RPL (bits 1:0) must equal
	  the RPL of the selector field for CS.1

    Base-address fields:
	— CS, SS, DS, ES, FS, GS. If the guest will be virtual-8086, the
	  address must be the selector field shifted left 4 bits (multiplied
	  by 16).
	— The following checks are performed on processors that support Intel
	  64 architecture:
		TR, FS, GS. The address must be canonical.
		LDTR. If LDTR is usable, the address must be canonical.
		CS. Bits 63:32 of the address must be zero.
		SS, DS, ES. If the register is usable, bits 63:32 of the
		address must be zero.

Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
---
 lib/x86/processor.h |   1 +
 x86/vmx_tests.c     | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+)

diff --git a/lib/x86/processor.h b/lib/x86/processor.h
index 03fdf64..3642212 100644
--- a/lib/x86/processor.h
+++ b/lib/x86/processor.h
@@ -57,6 +57,7 @@
 #define X86_EFLAGS_OF    0x00000800
 #define X86_EFLAGS_IOPL  0x00003000
 #define X86_EFLAGS_NT    0x00004000
+#define X86_EFLAGS_VM    0x00020000
 #define X86_EFLAGS_AC    0x00040000
 
 #define X86_EFLAGS_ALU (X86_EFLAGS_CF | X86_EFLAGS_PF | X86_EFLAGS_AF | \
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index a7abd63..5e96dfa 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -7681,6 +7681,113 @@ static void test_load_guest_pat(void)
 	test_pat(GUEST_PAT, "GUEST_PAT", ENT_CONTROLS, ENT_LOAD_PAT);
 }
 
+#define	GUEST_SEG_USABLE_MASK	1u << 16
+#define	TEST_SEGMENT_SEL(seg_sel, seg_sel_name, val, val_saved)		\
+	vmcs_write(seg_sel, val);					\
+	enter_guest_with_invalid_guest_state();				\
+	report_guest_state_test(seg_sel_name,			\
+				VMX_ENTRY_FAILURE |			\
+				VMX_FAIL_STATE,				\
+				val, seg_sel_name);			\
+	vmcs_write(seg_sel, val_saved);
+
+/*
+ * The following checks are done on the Selector field of the Guest Segment
+ * Registers:
+ *    — TR. The TI flag (bit 2) must be 0.
+ *    — LDTR. If LDTR is usable, the TI flag (bit 2) must be 0.
+ *    — SS. If the guest will not be virtual-8086 and the "unrestricted
+ *	guest" VM-execution control is 0, the RPL (bits 1:0) must equal
+ *	the RPL of the selector field for CS.
+ *
+ *  [Intel SDM]
+ */
+static void test_guest_segment_sel_fields(void)
+{
+	u16 sel_saved;
+	u16 sel;
+
+	sel_saved = vmcs_read(GUEST_SEL_TR);
+	sel = sel_saved | 0x4;
+	TEST_SEGMENT_SEL(GUEST_SEL_TR, "GUEST_SEL_TR", sel, sel_saved);
+
+	sel_saved = vmcs_read(GUEST_SEL_LDTR);
+	sel = sel_saved | 0x4;
+	TEST_SEGMENT_SEL(GUEST_SEL_LDTR, "GUEST_SEL_LDTR", sel, sel_saved);
+
+	if (!(vmcs_read(GUEST_RFLAGS) & X86_EFLAGS_VM) &&
+	    !(vmcs_read(CPU_SECONDARY) & CPU_URG)) {
+		u16 cs_rpl_bits = vmcs_read(GUEST_SEL_CS) & 0x3;
+		sel_saved = vmcs_read(GUEST_SEL_SS);
+		sel = sel_saved | (~cs_rpl_bits & 0x3);
+		TEST_SEGMENT_SEL(GUEST_SEL_SS, "GUEST_SEL_SS", sel, sel_saved);
+	}
+}
+
+#define	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(seg_base, seg_base_name)	\
+	addr_saved = vmcs_read(seg_base);				\
+	for (i = 32; i < 63; i = i + 4) {				\
+		addr = addr_saved | 1ull << i;				\
+		vmcs_write(seg_base, addr);				\
+		enter_guest_with_invalid_guest_state();			\
+		report_guest_state_test(seg_base_name,			\
+					VMX_ENTRY_FAILURE |		\
+					VMX_FAIL_STATE,			\
+					addr, seg_base_name);		\
+	}								\
+									\
+	vmcs_write(seg_base, addr_saved);
+
+#define	TEST_SEGMENT_BASE_ADDR_CANONICAL(seg_base, seg_base_name)	\
+	addr_saved = vmcs_read(seg_base);				\
+	vmcs_write(seg_base, NONCANONICAL);				\
+	enter_guest_with_invalid_guest_state();				\
+	report_guest_state_test(seg_base_name,				\
+				VMX_ENTRY_FAILURE | VMX_FAIL_STATE,	\
+				NONCANONICAL, seg_base_name);		\
+	vmcs_write(seg_base, addr_saved);
+
+/*
+ * The following checks are done on the Base Address field of the Guest
+ * Segment Registers on processors that support Intel 64 architecture:
+ *    - TR, FS, GS : The address must be canonical.
+ *    - LDTR : If LDTR is usable, the address must be canonical.
+ *    - CS : Bits 63:32 of the address must be zero.
+ *    - SS, DS, ES : If the register is usable, bits 63:32 of the address
+ *	must be zero.
+ *
+ *  [Intel SDM]
+ */
+static void test_guest_segment_base_addr_fields(void)
+{
+	u64 addr_saved, addr;
+	int i;
+
+	/*
+	 * The address of TR, FS, GS and LDTR must be canonical.
+	 */
+	TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_TR, "GUEST_BASE_TR");
+	TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_FS, "GUEST_BASE_FS");
+	TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_GS, "GUEST_BASE_GS");
+	if (!(vmcs_read(GUEST_AR_LDTR) & GUEST_SEG_USABLE_MASK))
+		TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_LDTR,
+						"GUEST_BASE_LDTR");
+
+	/*
+	 * Bits 63:32 in CS, SS, DS and ES base address must be zero
+	 */
+	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_CS, "GUEST_BASE_CS");
+	if (!(vmcs_read(GUEST_AR_SS) & GUEST_SEG_USABLE_MASK))
+		TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_SS,
+						 "GUEST_BASE_SS");
+	if (!(vmcs_read(GUEST_AR_DS) & GUEST_SEG_USABLE_MASK))
+		TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_DS,
+						 "GUEST_BASE_DS");
+	if (!(vmcs_read(GUEST_AR_ES) & GUEST_SEG_USABLE_MASK))
+		TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_ES,
+						 "GUEST_BASE_ES");
+}
+
 /*
  * Check that the virtual CPU checks the VMX Guest State Area as
  * documented in the Intel SDM.
@@ -7701,6 +7808,8 @@ static void vmx_guest_state_area_test(void)
 	test_load_guest_pat();
 	test_guest_efer();
 	test_load_guest_perf_global_ctrl();
+	test_guest_segment_sel_fields();
+	test_guest_segment_base_addr_fields();
 
 	/*
 	 * Let the guest finish execution
-- 
1.8.3.1

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers

[Jim Mattson]

On Tue, Mar 10, 2020 at 4:29 PM Krish Sadhukhan
<krish.sadhukhan@oracle.com> wrote:
>
> Even thought today's x86 hardware uses paging and not segmentation for memory
> management, it is still good to have some tests that can verify the sanity of
> the segment register fields on vmentry of nested guests.
>
> The test on SS Selector field is failing because the hardware (I am using
> Intel Xeon Platinum 8167M 2.00GHz) doesn't raise any error even if the
> prescribed bit pattern is not set and as a result vmentry succeeds.

Are you sure this isn't just an L0 bug? For instance, does your L0 set
"unrestricted guest" in vmcs02, even when L1 doesn't set it in vmcs12?

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Sean Christopherson]

On Tue, Mar 10, 2020 at 06:51:49PM -0400, Krish Sadhukhan wrote:
> According to section "Checks on Guest Segment Registers" in Intel SDM vol 3C,
> the following checks are performed on the Guest Segment Registers on vmentry
> of nested guests:
> 
>     Selector fields:
> 	— TR. The TI flag (bit 2) must be 0.
> 	— LDTR. If LDTR is usable, the TI flag (bit 2) must be 0.
> 	— SS. If the guest will not be virtual-8086 and the "unrestricted
> 	  guest" VM-execution control is 0, the RPL (bits 1:0) must equal
> 	  the RPL of the selector field for CS.1
> 
>     Base-address fields:
> 	— CS, SS, DS, ES, FS, GS. If the guest will be virtual-8086, the
> 	  address must be the selector field shifted left 4 bits (multiplied
> 	  by 16).
> 	— The following checks are performed on processors that support Intel
> 	  64 architecture:
> 		TR, FS, GS. The address must be canonical.
> 		LDTR. If LDTR is usable, the address must be canonical.
> 		CS. Bits 63:32 of the address must be zero.
> 		SS, DS, ES. If the register is usable, bits 63:32 of the
> 		address must be zero.
> 
> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
> ---
>  lib/x86/processor.h |   1 +
>  x86/vmx_tests.c     | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 110 insertions(+)
> 
> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
> index 03fdf64..3642212 100644
> --- a/lib/x86/processor.h
> +++ b/lib/x86/processor.h
> @@ -57,6 +57,7 @@
>  #define X86_EFLAGS_OF    0x00000800
>  #define X86_EFLAGS_IOPL  0x00003000
>  #define X86_EFLAGS_NT    0x00004000
> +#define X86_EFLAGS_VM    0x00020000
>  #define X86_EFLAGS_AC    0x00040000
>  
>  #define X86_EFLAGS_ALU (X86_EFLAGS_CF | X86_EFLAGS_PF | X86_EFLAGS_AF | \
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index a7abd63..5e96dfa 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -7681,6 +7681,113 @@ static void test_load_guest_pat(void)
>  	test_pat(GUEST_PAT, "GUEST_PAT", ENT_CONTROLS, ENT_LOAD_PAT);
>  }
>  
> +#define	GUEST_SEG_USABLE_MASK	1u << 16

s/USABLE/UNUSABLE

The usage below looks correct, just the name is inverted.

> +#define	TEST_SEGMENT_SEL(seg_sel, seg_sel_name, val, val_saved)		\
> +	vmcs_write(seg_sel, val);					\
> +	enter_guest_with_invalid_guest_state();				\
> +	report_guest_state_test(seg_sel_name,			\
> +				VMX_ENTRY_FAILURE |			\
> +				VMX_FAIL_STATE,				\
> +				val, seg_sel_name);			\
> +	vmcs_write(seg_sel, val_saved);
> +
> +/*
> + * The following checks are done on the Selector field of the Guest Segment
> + * Registers:
> + *    — TR. The TI flag (bit 2) must be 0.
> + *    — LDTR. If LDTR is usable, the TI flag (bit 2) must be 0.
> + *    — SS. If the guest will not be virtual-8086 and the "unrestricted
> + *	guest" VM-execution control is 0, the RPL (bits 1:0) must equal
> + *	the RPL of the selector field for CS.
> + *
> + *  [Intel SDM]
> + */
> +static void test_guest_segment_sel_fields(void)
> +{
> +	u16 sel_saved;
> +	u16 sel;
> +
> +	sel_saved = vmcs_read(GUEST_SEL_TR);
> +	sel = sel_saved | 0x4;
> +	TEST_SEGMENT_SEL(GUEST_SEL_TR, "GUEST_SEL_TR", sel, sel_saved);
> +
> +	sel_saved = vmcs_read(GUEST_SEL_LDTR);
> +	sel = sel_saved | 0x4;
> +	TEST_SEGMENT_SEL(GUEST_SEL_LDTR, "GUEST_SEL_LDTR", sel, sel_saved);
> +
> +	if (!(vmcs_read(GUEST_RFLAGS) & X86_EFLAGS_VM) &&
> +	    !(vmcs_read(CPU_SECONDARY) & CPU_URG)) {

Rather than react to the environment, these tests should configure every
relevant aspect and ignore the ones it can't change.  E.g. the unit tests
aren't going to randomly launch a vm86 guest.  Ditto for the unusuable bit,
it's unlikely to be set for most segments and would be something to test
explicitly.

> +		u16 cs_rpl_bits = vmcs_read(GUEST_SEL_CS) & 0x3;
> +		sel_saved = vmcs_read(GUEST_SEL_SS);
> +		sel = sel_saved | (~cs_rpl_bits & 0x3);
> +		TEST_SEGMENT_SEL(GUEST_SEL_SS, "GUEST_SEL_SS", sel, sel_saved);
> +	}
> +}
> +
> +#define	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(seg_base, seg_base_name)	\
> +	addr_saved = vmcs_read(seg_base);				\
> +	for (i = 32; i < 63; i = i + 4) {				\
> +		addr = addr_saved | 1ull << i;				\
> +		vmcs_write(seg_base, addr);				\
> +		enter_guest_with_invalid_guest_state();			\
> +		report_guest_state_test(seg_base_name,			\
> +					VMX_ENTRY_FAILURE |		\
> +					VMX_FAIL_STATE,			\
> +					addr, seg_base_name);		\
> +	}								\
> +									\
> +	vmcs_write(seg_base, addr_saved);
> +
> +#define	TEST_SEGMENT_BASE_ADDR_CANONICAL(seg_base, seg_base_name)	\
> +	addr_saved = vmcs_read(seg_base);				\
> +	vmcs_write(seg_base, NONCANONICAL);				\
> +	enter_guest_with_invalid_guest_state();				\
> +	report_guest_state_test(seg_base_name,				\
> +				VMX_ENTRY_FAILURE | VMX_FAIL_STATE,	\
> +				NONCANONICAL, seg_base_name);		\
> +	vmcs_write(seg_base, addr_saved);
> +
> +/*
> + * The following checks are done on the Base Address field of the Guest
> + * Segment Registers on processors that support Intel 64 architecture:
> + *    - TR, FS, GS : The address must be canonical.
> + *    - LDTR : If LDTR is usable, the address must be canonical.
> + *    - CS : Bits 63:32 of the address must be zero.
> + *    - SS, DS, ES : If the register is usable, bits 63:32 of the address
> + *	must be zero.
> + *
> + *  [Intel SDM]
> + */
> +static void test_guest_segment_base_addr_fields(void)
> +{
> +	u64 addr_saved, addr;
> +	int i;
> +
> +	/*
> +	 * The address of TR, FS, GS and LDTR must be canonical.
> +	 */
> +	TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_TR, "GUEST_BASE_TR");
> +	TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_FS, "GUEST_BASE_FS");
> +	TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_GS, "GUEST_BASE_GS");

FS/GS bases aren't checked if the segment is unusable.

> +	if (!(vmcs_read(GUEST_AR_LDTR) & GUEST_SEG_USABLE_MASK))
> +		TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_LDTR,
> +						"GUEST_BASE_LDTR");
> +
> +	/*
> +	 * Bits 63:32 in CS, SS, DS and ES base address must be zero
> +	 */
> +	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_CS, "GUEST_BASE_CS");
> +	if (!(vmcs_read(GUEST_AR_SS) & GUEST_SEG_USABLE_MASK))
> +		TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_SS,
> +						 "GUEST_BASE_SS");
> +	if (!(vmcs_read(GUEST_AR_DS) & GUEST_SEG_USABLE_MASK))
> +		TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_DS,
> +						 "GUEST_BASE_DS");
> +	if (!(vmcs_read(GUEST_AR_ES) & GUEST_SEG_USABLE_MASK))
> +		TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_ES,
> +						 "GUEST_BASE_ES");
> +}
> +
>  /*
>   * Check that the virtual CPU checks the VMX Guest State Area as
>   * documented in the Intel SDM.
> @@ -7701,6 +7808,8 @@ static void vmx_guest_state_area_test(void)
>  	test_load_guest_pat();
>  	test_guest_efer();
>  	test_load_guest_perf_global_ctrl();
> +	test_guest_segment_sel_fields();
> +	test_guest_segment_base_addr_fields();
>  
>  	/*
>  	 * Let the guest finish execution
> -- 
> 1.8.3.1
> 

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Sean Christopherson]

On Wed, Mar 11, 2020 at 08:05:16AM -0700, Sean Christopherson wrote:
> On Tue, Mar 10, 2020 at 06:51:49PM -0400, Krish Sadhukhan wrote:
> > +	/*
> > +	 * The address of TR, FS, GS and LDTR must be canonical.
> > +	 */
> > +	TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_TR, "GUEST_BASE_TR");
> > +	TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_FS, "GUEST_BASE_FS");
> > +	TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_GS, "GUEST_BASE_GS");
> 
> FS/GS bases aren't checked if the segment is unusable.

Ah, I stand corrected, I misread the section on loading guest segs.   There
is an exception clause for FS/GS base inside the unusuable path.  And the
"checks on guest segments" clearly states FS/GS base are unconditionally
checked.

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Krish Sadhukhan]

On 3/11/20 8:05 AM, Sean Christopherson wrote:
> On Tue, Mar 10, 2020 at 06:51:49PM -0400, Krish Sadhukhan wrote:
>> According to section "Checks on Guest Segment Registers" in Intel SDM vol 3C,
>> the following checks are performed on the Guest Segment Registers on vmentry
>> of nested guests:
>>
>>      Selector fields:
>> 	— TR. The TI flag (bit 2) must be 0.
>> 	— LDTR. If LDTR is usable, the TI flag (bit 2) must be 0.
>> 	— SS. If the guest will not be virtual-8086 and the "unrestricted
>> 	  guest" VM-execution control is 0, the RPL (bits 1:0) must equal
>> 	  the RPL of the selector field for CS.1
>>
>>      Base-address fields:
>> 	— CS, SS, DS, ES, FS, GS. If the guest will be virtual-8086, the
>> 	  address must be the selector field shifted left 4 bits (multiplied
>> 	  by 16).
>> 	— The following checks are performed on processors that support Intel
>> 	  64 architecture:
>> 		TR, FS, GS. The address must be canonical.
>> 		LDTR. If LDTR is usable, the address must be canonical.
>> 		CS. Bits 63:32 of the address must be zero.
>> 		SS, DS, ES. If the register is usable, bits 63:32 of the
>> 		address must be zero.
>>
>> Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com>
>> ---
>>   lib/x86/processor.h |   1 +
>>   x86/vmx_tests.c     | 109 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 110 insertions(+)
>>
>> diff --git a/lib/x86/processor.h b/lib/x86/processor.h
>> index 03fdf64..3642212 100644
>> --- a/lib/x86/processor.h
>> +++ b/lib/x86/processor.h
>> @@ -57,6 +57,7 @@
>>   #define X86_EFLAGS_OF    0x00000800
>>   #define X86_EFLAGS_IOPL  0x00003000
>>   #define X86_EFLAGS_NT    0x00004000
>> +#define X86_EFLAGS_VM    0x00020000
>>   #define X86_EFLAGS_AC    0x00040000
>>   
>>   #define X86_EFLAGS_ALU (X86_EFLAGS_CF | X86_EFLAGS_PF | X86_EFLAGS_AF | \
>> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> index a7abd63..5e96dfa 100644
>> --- a/x86/vmx_tests.c
>> +++ b/x86/vmx_tests.c
>> @@ -7681,6 +7681,113 @@ static void test_load_guest_pat(void)
>>   	test_pat(GUEST_PAT, "GUEST_PAT", ENT_CONTROLS, ENT_LOAD_PAT);
>>   }
>>   
>> +#define	GUEST_SEG_USABLE_MASK	1u << 16
> s/USABLE/UNUSABLE
>
> The usage below looks correct, just the name is inverted.
>
>> +#define	TEST_SEGMENT_SEL(seg_sel, seg_sel_name, val, val_saved)		\
>> +	vmcs_write(seg_sel, val);					\
>> +	enter_guest_with_invalid_guest_state();				\
>> +	report_guest_state_test(seg_sel_name,			\
>> +				VMX_ENTRY_FAILURE |			\
>> +				VMX_FAIL_STATE,				\
>> +				val, seg_sel_name);			\
>> +	vmcs_write(seg_sel, val_saved);
>> +
>> +/*
>> + * The following checks are done on the Selector field of the Guest Segment
>> + * Registers:
>> + *    — TR. The TI flag (bit 2) must be 0.
>> + *    — LDTR. If LDTR is usable, the TI flag (bit 2) must be 0.
>> + *    — SS. If the guest will not be virtual-8086 and the "unrestricted
>> + *	guest" VM-execution control is 0, the RPL (bits 1:0) must equal
>> + *	the RPL of the selector field for CS.
>> + *
>> + *  [Intel SDM]
>> + */
>> +static void test_guest_segment_sel_fields(void)
>> +{
>> +	u16 sel_saved;
>> +	u16 sel;
>> +
>> +	sel_saved = vmcs_read(GUEST_SEL_TR);
>> +	sel = sel_saved | 0x4;
>> +	TEST_SEGMENT_SEL(GUEST_SEL_TR, "GUEST_SEL_TR", sel, sel_saved);
>> +
>> +	sel_saved = vmcs_read(GUEST_SEL_LDTR);
>> +	sel = sel_saved | 0x4;
>> +	TEST_SEGMENT_SEL(GUEST_SEL_LDTR, "GUEST_SEL_LDTR", sel, sel_saved);
>> +
>> +	if (!(vmcs_read(GUEST_RFLAGS) & X86_EFLAGS_VM) &&
>> +	    !(vmcs_read(CPU_SECONDARY) & CPU_URG)) {
> Rather than react to the environment, these tests should configure every
> relevant aspect and ignore the ones it can't change.  E.g. the unit tests
> aren't going to randomly launch a vm86 guest.  Ditto for the unusuable bit,
> it's unlikely to be set for most segments and would be something to test
> explicitly.


Just wanted to clarify on the "unusable bit" part of your comment. Do 
you mean each of the segment register checks from the SDM should have 
two tests, one with the "unusable bit" set and the other with that bit 
not set, irrespective of the checks being conditional on the setting of 
that bit ?

>
>> +		u16 cs_rpl_bits = vmcs_read(GUEST_SEL_CS) & 0x3;
>> +		sel_saved = vmcs_read(GUEST_SEL_SS);
>> +		sel = sel_saved | (~cs_rpl_bits & 0x3);
>> +		TEST_SEGMENT_SEL(GUEST_SEL_SS, "GUEST_SEL_SS", sel, sel_saved);
>> +	}
>> +}
>> +
>> +#define	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(seg_base, seg_base_name)	\
>> +	addr_saved = vmcs_read(seg_base);				\
>> +	for (i = 32; i < 63; i = i + 4) {				\
>> +		addr = addr_saved | 1ull << i;				\
>> +		vmcs_write(seg_base, addr);				\
>> +		enter_guest_with_invalid_guest_state();			\
>> +		report_guest_state_test(seg_base_name,			\
>> +					VMX_ENTRY_FAILURE |		\
>> +					VMX_FAIL_STATE,			\
>> +					addr, seg_base_name);		\
>> +	}								\
>> +									\
>> +	vmcs_write(seg_base, addr_saved);
>> +
>> +#define	TEST_SEGMENT_BASE_ADDR_CANONICAL(seg_base, seg_base_name)	\
>> +	addr_saved = vmcs_read(seg_base);				\
>> +	vmcs_write(seg_base, NONCANONICAL);				\
>> +	enter_guest_with_invalid_guest_state();				\
>> +	report_guest_state_test(seg_base_name,				\
>> +				VMX_ENTRY_FAILURE | VMX_FAIL_STATE,	\
>> +				NONCANONICAL, seg_base_name);		\
>> +	vmcs_write(seg_base, addr_saved);
>> +
>> +/*
>> + * The following checks are done on the Base Address field of the Guest
>> + * Segment Registers on processors that support Intel 64 architecture:
>> + *    - TR, FS, GS : The address must be canonical.
>> + *    - LDTR : If LDTR is usable, the address must be canonical.
>> + *    - CS : Bits 63:32 of the address must be zero.
>> + *    - SS, DS, ES : If the register is usable, bits 63:32 of the address
>> + *	must be zero.
>> + *
>> + *  [Intel SDM]
>> + */
>> +static void test_guest_segment_base_addr_fields(void)
>> +{
>> +	u64 addr_saved, addr;
>> +	int i;
>> +
>> +	/*
>> +	 * The address of TR, FS, GS and LDTR must be canonical.
>> +	 */
>> +	TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_TR, "GUEST_BASE_TR");
>> +	TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_FS, "GUEST_BASE_FS");
>> +	TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_GS, "GUEST_BASE_GS");
> FS/GS bases aren't checked if the segment is unusable.
>
>> +	if (!(vmcs_read(GUEST_AR_LDTR) & GUEST_SEG_USABLE_MASK))
>> +		TEST_SEGMENT_BASE_ADDR_CANONICAL(GUEST_BASE_LDTR,
>> +						"GUEST_BASE_LDTR");
>> +
>> +	/*
>> +	 * Bits 63:32 in CS, SS, DS and ES base address must be zero
>> +	 */
>> +	TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_CS, "GUEST_BASE_CS");
>> +	if (!(vmcs_read(GUEST_AR_SS) & GUEST_SEG_USABLE_MASK))
>> +		TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_SS,
>> +						 "GUEST_BASE_SS");
>> +	if (!(vmcs_read(GUEST_AR_DS) & GUEST_SEG_USABLE_MASK))
>> +		TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_DS,
>> +						 "GUEST_BASE_DS");
>> +	if (!(vmcs_read(GUEST_AR_ES) & GUEST_SEG_USABLE_MASK))
>> +		TEST_SEGMENT_BASE_ADDR_UPPER_BITS(GUEST_BASE_ES,
>> +						 "GUEST_BASE_ES");
>> +}
>> +
>>   /*
>>    * Check that the virtual CPU checks the VMX Guest State Area as
>>    * documented in the Intel SDM.
>> @@ -7701,6 +7808,8 @@ static void vmx_guest_state_area_test(void)
>>   	test_load_guest_pat();
>>   	test_guest_efer();
>>   	test_load_guest_perf_global_ctrl();
>> +	test_guest_segment_sel_fields();
>> +	test_guest_segment_base_addr_fields();
>>   
>>   	/*
>>   	 * Let the guest finish execution
>> -- 
>> 1.8.3.1
>>

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Sean Christopherson]

On Wed, Mar 11, 2020 at 01:38:24PM -0700, Krish Sadhukhan wrote:
> 
> On 3/11/20 8:05 AM, Sean Christopherson wrote:
> >>+static void test_guest_segment_sel_fields(void)
> >>+{
> >>+	u16 sel_saved;
> >>+	u16 sel;
> >>+
> >>+	sel_saved = vmcs_read(GUEST_SEL_TR);
> >>+	sel = sel_saved | 0x4;
> >>+	TEST_SEGMENT_SEL(GUEST_SEL_TR, "GUEST_SEL_TR", sel, sel_saved);
> >>+
> >>+	sel_saved = vmcs_read(GUEST_SEL_LDTR);
> >>+	sel = sel_saved | 0x4;
> >>+	TEST_SEGMENT_SEL(GUEST_SEL_LDTR, "GUEST_SEL_LDTR", sel, sel_saved);
> >>+
> >>+	if (!(vmcs_read(GUEST_RFLAGS) & X86_EFLAGS_VM) &&
> >>+	    !(vmcs_read(CPU_SECONDARY) & CPU_URG)) {
> >Rather than react to the environment, these tests should configure every
> >relevant aspect and ignore the ones it can't change.  E.g. the unit tests
> >aren't going to randomly launch a vm86 guest.  Ditto for the unusuable bit,
> >it's unlikely to be set for most segments and would be something to test
> >explicitly.
> 
> 
> Just wanted to clarify on the "unusable bit" part of your comment. Do you
> mean each of the segment register checks from the SDM should have two tests,
> one with the "unusable bit" set and the other with that bit not set,
> irrespective of the checks being conditional on the setting of that bit ?

Sort of.  In an ideal world, kvm-unit-tests would verify correctness of KVM
for both unusable=1 and unusable=0.  But, the unusable=1 validation space is
enormous, i.e. there are a bazillion combinations of random garbage that can
be thrown into GUEST_*S_{SE,ARBYTE,BASE}.  So yeah, it could be as simple as
running the same test as unusable=0, but expecting VM-Entry to succeed.

That being said, I don't understand the motivation for these tests.  KVM
doesn't have any dedicated logic for checking guest segments, i.e. these
tests are validating hardware behavior, not KVM behavior.  The validation
resources thrown at hardware dwarf what kvm-unit-tests can do, i.e. the
odds of finding a silicon bug are tiny, and the odds of such a bug being
exploitable aginst L0 are downright miniscule.

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Nadav Amit]

> On Mar 11, 2020, at 2:46 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> On Wed, Mar 11, 2020 at 01:38:24PM -0700, Krish Sadhukhan wrote:
>> On 3/11/20 8:05 AM, Sean Christopherson wrote:
>>>> +static void test_guest_segment_sel_fields(void)
>>>> +{
>>>> +	u16 sel_saved;
>>>> +	u16 sel;
>>>> +
>>>> +	sel_saved = vmcs_read(GUEST_SEL_TR);
>>>> +	sel = sel_saved | 0x4;
>>>> +	TEST_SEGMENT_SEL(GUEST_SEL_TR, "GUEST_SEL_TR", sel, sel_saved);
>>>> +
>>>> +	sel_saved = vmcs_read(GUEST_SEL_LDTR);
>>>> +	sel = sel_saved | 0x4;
>>>> +	TEST_SEGMENT_SEL(GUEST_SEL_LDTR, "GUEST_SEL_LDTR", sel, sel_saved);
>>>> +
>>>> +	if (!(vmcs_read(GUEST_RFLAGS) & X86_EFLAGS_VM) &&
>>>> +	    !(vmcs_read(CPU_SECONDARY) & CPU_URG)) {
>>> Rather than react to the environment, these tests should configure every
>>> relevant aspect and ignore the ones it can't change.  E.g. the unit tests
>>> aren't going to randomly launch a vm86 guest.  Ditto for the unusuable bit,
>>> it's unlikely to be set for most segments and would be something to test
>>> explicitly.
>> 
>> 
>> Just wanted to clarify on the "unusable bit" part of your comment. Do you
>> mean each of the segment register checks from the SDM should have two tests,
>> one with the "unusable bit" set and the other with that bit not set,
>> irrespective of the checks being conditional on the setting of that bit ?
> 
> Sort of.  In an ideal world, kvm-unit-tests would verify correctness of KVM
> for both unusable=1 and unusable=0.  But, the unusable=1 validation space is
> enormous, i.e. there are a bazillion combinations of random garbage that can
> be thrown into GUEST_*S_{SE,ARBYTE,BASE}.  So yeah, it could be as simple as
> running the same test as unusable=0, but expecting VM-Entry to succeed.
> 
> That being said, I don't understand the motivation for these tests.  KVM
> doesn't have any dedicated logic for checking guest segments, i.e. these
> tests are validating hardware behavior, not KVM behavior.  The validation
> resources thrown at hardware dwarf what kvm-unit-tests can do, i.e. the
> odds of finding a silicon bug are tiny, and the odds of such a bug being
> exploitable aginst L0 are downright miniscule.

I see no reason for not including such tests. Liran said he uses
kvm-unit-test with WHPX, and I also use it in some non-KVM setups.

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Liran Alon]

On 11/03/2020 23:53, Nadav Amit wrote:
>> On Mar 11, 2020, at 2:46 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>>
>> On Wed, Mar 11, 2020 at 01:38:24PM -0700, Krish Sadhukhan wrote:
>>
>> That being said, I don't understand the motivation for these tests.  KVM
>> doesn't have any dedicated logic for checking guest segments, i.e. these
>> tests are validating hardware behavior, not KVM behavior.  The validation
>> resources thrown at hardware dwarf what kvm-unit-tests can do, i.e. the
>> odds of finding a silicon bug are tiny, and the odds of such a bug being
>> exploitable aginst L0 are downright miniscule.
> I see no reason for not including such tests. Liran said he uses
> kvm-unit-test with WHPX, and I also use it in some non-KVM setups.

+1.
I admit I haven't read this thread at all but I wanted to point out 
something I already told Paolo at KVM Forum:
kvm-unit-tests should be renamed to cpu-unit-tests. i.e. It should be 
treated as a suite of unit-tests that verifies CPU behavior.
It doesn't matter if the CPU it runs on top of is Bare-Metal (As Nadav 
runs it) or on top of a vCPU implementation (E.g. KVM, VirtualBox, 
VMware, Hyper-V, whatever).

There are multiple reasons of why it should be treated like that:
* It allows us to verify that the unit-test indeed pass on a real CPU 
and thus enforce on vCPU implementation a behavior that a real CPU 
performs. Instead of what the unit-test author thought the CPU should 
perform.
   I have personally already made a mistake on this area when I wrote 
the unit-test to verify KVM handling of INIT signal while vCPU is in VMX 
root-mode. Nadav run the test on top of a Bare-Metal CPU and showed that
   the unit-test fails and therefore should written otherwise.
* It allows, hopefully, for multiple hypervisor vendors to collaborate 
on the same set of unit-tests. Sharing regression tests and helping each 
other enforce correct vCPU behavior.
    As Nadav have pointed out, I have used this technique already to run 
the unit-tests on top of Hyper-V using WHPX. Which indeed revealed bugs 
in Hyper-V.

Of course it was best if Intel would have shared their unit-tests for 
CPU functionality (Sean? I'm looking at you :P), but I am not aware that 
they did.
So cpu-unit-tests (Previously named kvm-unit-tests) should be our best 
replacement for now.

This is also why I would wish that we will make sure that all unit-tests 
enforcing CPU functionality will be written in cpu-unit-tests and not on 
KVM selftests.
Only tests involving using KVM userspace API should be written in KVM 
selftests suite.

-Liran

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Sean Christopherson]

On Thu, Mar 12, 2020 at 12:54:05AM +0200, Liran Alon wrote:
> Of course it was best if Intel would have shared their unit-tests for CPU
> functionality (Sean? I'm looking at you :P), but I am not aware that they
> did.

Only in my dreams :-)  I would love to open source some of Intel's
validation tools so that they could be adapted to hammer KVM, but it'll
never happen for a variety of reasons.

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Nadav Amit]

> On Mar 11, 2020, at 4:12 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> On Thu, Mar 12, 2020 at 12:54:05AM +0200, Liran Alon wrote:
>> Of course it was best if Intel would have shared their unit-tests for CPU
>> functionality (Sean? I'm looking at you :P), but I am not aware that they
>> did.
> 
> Only in my dreams :-)  I would love to open source some of Intel's
> validation tools so that they could be adapted to hammer KVM, but it'll
> never happen for a variety of reasons.

FYI: In 2014 I ran Intel’s fuzzing-tool (Cafe) to test KVM and found (and
fixed) ~100 bugs. And I did not even test nested virtualization…

http://www.cs.technion.ac.il/people/namit/online-publications/161-amit.pdf

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Liran Alon]

On 12/03/2020 1:12, Sean Christopherson wrote:
> On Thu, Mar 12, 2020 at 12:54:05AM +0200, Liran Alon wrote:
>> Of course it was best if Intel would have shared their unit-tests for CPU
>> functionality (Sean? I'm looking at you :P), but I am not aware that they
>> did.
> Only in my dreams :-)  I would love to open source some of Intel's
> validation tools so that they could be adapted to hammer KVM, but it'll
> never happen for a variety of reasons.

I hope then you at least built a setup internally at Intel that runs 
these test suites on top of KVM to find bugs. :)
It would also be nice of Intel to even setup this internally on top of 
other common hypervisors (e.g. Hyper-V, VMware) and report bugs to vendors.

-Liran

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Sean Christopherson]

On Wed, Mar 11, 2020 at 04:21:08PM -0700, Nadav Amit wrote:
> > On Mar 11, 2020, at 4:12 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > 
> > On Thu, Mar 12, 2020 at 12:54:05AM +0200, Liran Alon wrote:
> >> Of course it was best if Intel would have shared their unit-tests for CPU
> >> functionality (Sean? I'm looking at you :P), but I am not aware that they
> >> did.
> > 
> > Only in my dreams :-)  I would love to open source some of Intel's
> > validation tools so that they could be adapted to hammer KVM, but it'll
> > never happen for a variety of reasons.
> 
> FYI: In 2014 I ran Intel’s fuzzing-tool (Cafe) to test KVM and found (and
> fixed) ~100 bugs. And I did not even test nested virtualization…

Heh, I worked on Cafe for 6+ years :-)

> http://www.cs.technion.ac.il/people/namit/online-publications/161-amit.pdf

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Sean Christopherson]

On Wed, Mar 11, 2020 at 11:22:13PM +0000, Liran Alon wrote:
> 
> On 12/03/2020 1:12, Sean Christopherson wrote:
> >On Thu, Mar 12, 2020 at 12:54:05AM +0200, Liran Alon wrote:
> >>Of course it was best if Intel would have shared their unit-tests for CPU
> >>functionality (Sean? I'm looking at you :P), but I am not aware that they
> >>did.
> >Only in my dreams :-)  I would love to open source some of Intel's
> >validation tools so that they could be adapted to hammer KVM, but it'll
> >never happen for a variety of reasons.
> 
> I hope then you at least built a setup internally at Intel that runs these
> test suites on top of KVM to find bugs. :)

It's on my wish list.

> It would also be nice of Intel to even setup this internally on top of other
> common hypervisors (e.g. Hyper-V, VMware) and report bugs to vendors.
> 
> -Liran
> 
> 

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment Registers on vmentry of nested guests

[Nadav Amit]

> On Mar 11, 2020, at 4:25 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> On Wed, Mar 11, 2020 at 04:21:08PM -0700, Nadav Amit wrote:
>>> On Mar 11, 2020, at 4:12 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>>> 
>>> On Thu, Mar 12, 2020 at 12:54:05AM +0200, Liran Alon wrote:
>>>> Of course it was best if Intel would have shared their unit-tests for CPU
>>>> functionality (Sean? I'm looking at you :P), but I am not aware that they
>>>> did.
>>> 
>>> Only in my dreams :-)  I would love to open source some of Intel's
>>> validation tools so that they could be adapted to hammer KVM, but it'll
>>> never happen for a variety of reasons.
>> 
>> FYI: In 2014 I ran Intel’s fuzzing-tool (Cafe) to test KVM and found (and
>> fixed) ~100 bugs. And I did not even test nested virtualization…
> 
> Heh, I worked on Cafe for 6+ years :-)

Well, I was working on its predecessor, Janus, for 5 years… 

Re: [PATCH] kvm-unit-test: nVMX: Test Selector and Base Address fields of Guest Segment registers

[Krish Sadhukhan]

On 3/11/20 8:24 AM, Sean Christopherson wrote:
> On Tue, Mar 10, 2020 at 04:51:40PM -0700, Jim Mattson wrote:
>> On Tue, Mar 10, 2020 at 4:29 PM Krish Sadhukhan
>> <krish.sadhukhan@oracle.com> wrote:
>>> Even thought today's x86 hardware uses paging and not segmentation for memory
>>> management, it is still good to have some tests that can verify the sanity of
>>> the segment register fields on vmentry of nested guests.
>>>
>>> The test on SS Selector field is failing because the hardware (I am using
>>> Intel Xeon Platinum 8167M 2.00GHz) doesn't raise any error even if the
>>> prescribed bit pattern is not set and as a result vmentry succeeds.
>> Are you sure this isn't just an L0 bug? For instance, does your L0 set
>> "unrestricted guest" in vmcs02, even when L1 doesn't set it in vmcs12?
>
> I assume this is the check being discussed?  The code is flawed, if CS=3
> and SS=3, "sel = sel_saved | (~cs_rpl_bits & 0x3)" will yield SS=3 and pass.
>
> I think you wanted something like:
>
>    sel = (sel_saved & ~0x3) | (~cs_rpl_bits & 0x3);

Yes, my bit-setting was wrong and I have fixed it. But that's not the 
cause of the failure.

It appears that Jim's suspicion is correct. L0 is not checking in 
prepare_vmcs02_early(), whether vmcs12 has "unrestricted guest" turned 
off. After I put the relevant setting in that function, the test now 
passes (meaning vmentry fails).

I will add this fix in v2.


>
>
>> +	if (!(vmcs_read(GUEST_RFLAGS) & X86_EFLAGS_VM) &&
>> +	    !(vmcs_read(CPU_SECONDARY) & CPU_URG)) {
>> +		u16 cs_rpl_bits = vmcs_read(GUEST_SEL_CS) & 0x3;
>> +		sel_saved = vmcs_read(GUEST_SEL_SS);
>> +		sel = sel_saved | (~cs_rpl_bits & 0x3);
>> +		TEST_SEGMENT_SEL(GUEST_SEL_SS, "GUEST_SEL_SS", sel, sel_saved);
>> +	}
>> +}