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@xxxxxxxxxx> Reviewed-by: Karl Heubaum <karl.heubaum@xxxxxxxxxx> --- 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 valueComments 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
