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

[Sean Christopherson <sean.j.christopherson@intel.com>]

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
>