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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxxxxxx>
---
  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




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux