| Message ID | 20190319014624.31399-4-krish.sadhukhan@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3,nVMX] : Check "load IA32_PAT" VM-exit control on vmentry of L2 guests | expand |
On Mon, Mar 18, 2019 at 09:46:24PM -0400, Krish Sadhukhan wrote: > 1. According to section "Checks on Host Control Registers and MSRs" in Intel > SDM vol 3C, the following check is performed on vmentry of L2 guests: Heh, "of L2 guests" again. > > If the “load IA32_PAT” VM-exit control is 1, the value of the field > for the IA32_PAT MSR must be one that could be written by WRMSR > without fault at CPL 0. Specifically, each of the 8 bytes in the > field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), > 6 (WB), or 7 (UC-). > > 2. According to section "CHECKING AND LOADING GUEST STATE" in Intel ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Why use all caps here? > SDM vol 3C, the following check is performed on vmentry of L2 guests: > > If the "load IA32_PAT" VM-entry control is 1, the value of the field > for the IA32_PAT MSR must be one that could be written by WRMSR > without fault at CPL 0. Specifically, each of the 8 bytes in the > field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), > 6 (WB), or 7 (UC-). IMO this is a bit gratuitous. The paragraphs are identical except for "exit" vs. "entry". A little consolidation: According to sections "Checks on Host Control Registers and MSRs" and "Checking and Loading Guest State" in Intel SDM vol 3c, the following checks are performed on VM-Entry: If the "load IA32_PAT" VM-{entry,exit} control is 1, the value of the field for the IA32_PAT MSR must be one that could be written by WRMSR without fault at CPL 0. Specifically, each of the 8 bytes in the field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), 6 (WB), or 7 (UC-). > > Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> > Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> > --- > x86/vmx_tests.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 87 insertions(+) > > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > index 66a87f6..7dae0f0 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -4995,6 +4995,90 @@ static void test_sysenter_field(u32 field, const char *name) > vmcs_write(field, addr_saved); > } > > +static void test_pat_fields(u64 vmcs_fld, const char * vmcs_fld_name, > + u64 ctrl, u64 ctrl_fld) > +{ > + u32 ctrl_saved = vmcs_read(ctrl); > + u64 vmcs_fld_saved = vmcs_read(vmcs_fld); > + int i, j, error = 0; > + u64 val; > + > + vmcs_write(ctrl, ctrl_saved & ~ctrl_fld); > + for (i = 0; i < 256; i++) { > + /* Test PAT0..PAT7 fields */ > + for (j = 0; j < 8; j++) { > + val = i << j * 8; This shifts an 'int' by more than 32-bits and likely breaks a 32-bit build. > + vmcs_write(vmcs_fld, val); > + report_prefix_pushf("%s %lx", vmcs_fld_name, val); > + test_vmx_vmlaunch(error, false); I'd prefer to just pass '0' for error, it took me a second to find where error was set. > + report_prefix_pop(); > + } > + } > + > + vmcs_write(ctrl, ctrl_saved | ctrl_fld); > + for (i = 0; i < 256; i++) { > + /* Test PAT0..PAT7 fields */ > + for (j = 0; j < 8; j++) { This is the third or fourth test we have that iterates over every possible combination of illegal value, the vast majority of which are redundant to some degree. E.g. 'i > 8' is basically just 'i==8', and is particularly uninteresting when checking every value in every byte. And on the flip side, a shortcoming is that the value of the bytes we're not explicitly targeting will always be '0'. I wonder if it makes sense to split these types of exhaustive tests into two separate tests, one to be run as a standard VMX test, and the other to be binned into an "exhuastive" test case. That way people can get 99% of coverage using the standard test without having to wait for thousands of VM-Entries. I.e. pare this down to i=0..8 and j=0..8 (64 vs. 2048 entries), and put the i>8 portion into the more exhaustive test case and make it even more exhaustive by loading the other fields with non-zero legal values. > + val = i << j * 8; Again, an int is being shifted by more than 32-bits. > + vmcs_write(vmcs_fld, val); > + report_prefix_pushf("%s %lx", vmcs_fld_name, val); > + if (i == 0x2 || i == 0x3 || i > 0x7) { > + if (vmcs_fld == HOST_PAT) > + error = > + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD; > + else > + error = > + VMXERR_ENTRY_INVALID_CONTROL_FIELD; Invalid GUEST_PAT is a VM-Exit consistency check. > + } else { > + error = 0; > + } > + test_vmx_vmlaunch(error, false); > + report_prefix_pop(); > + } > + } > + > + vmcs_write(ctrl, ctrl_saved); > + vmcs_write(vmcs_fld, vmcs_fld_saved); > +} > + > +/* > + * 1. If the "load IA32_PAT" VM-exit control is 1, the value of the field > + * for the IA32_PAT MSR must be one that could be written by WRMSR > + * without fault at CPL 0. Specifically, each of the 8 bytes in the > + * field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), > + * 6 (WB), or 7 (UC-). > + * > + * 2. If the "load IA32_PAT" VM-entry control is 1, the value of the field > + * for the IA32_PAT MSR must be one that could be written by WRMSR > + * without fault at CPL 0. Specifically, each of the 8 bytes in the > + * field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), > + * 6 (WB), or 7 (UC-). > + * > + * [Intel SDM] > + */ > +static void test_load_pat(void) > +{ > + /* > + * "load IA32_PAT" VM-exit control > + */ > + if (!(ctrl_exit_rev.clr & EXI_LOAD_PAT)) { > + printf("\"Load-IA32-PAT\" exit control not supported\n"); > + return; > + } > + > + test_pat_fields(HOST_PAT, "HOST_PAT", EXI_CONTROLS, EXI_LOAD_PAT); > + > + /* > + * "load IA32_PAT" VM-entry control > + */ > + if (!(ctrl_enter_rev.clr & ENT_LOAD_PAT)) { > + printf("\"Load-IA32-PAT\" entry control not supported\n"); > + return; > + } > + > + test_pat_fields(GUEST_PAT, "GUEST_PAT", ENT_CONTROLS, ENT_LOAD_PAT); > +} > + > /* > * Check that the virtual CPU checks the VMX Host State Area as > * documented in the Intel SDM. > @@ -5010,6 +5094,9 @@ static void vmx_host_state_area_test(void) > > test_sysenter_field(HOST_SYSENTER_ESP, "HOST_SYSENTER_ESP"); > test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP"); > + > + test_load_pat(); You're testing both guest and host state in vmx_HOST_state_area_test(). > + > } > > static bool valid_vmcs_for_vmentry(void) > -- > 2.17.2 >
On Thu, Mar 21, 2019 at 12:09:46AM -0700, Krish Sadhukhan wrote: > > > On 03/19/2019 08:33 AM, Sean Christopherson wrote: > >On Mon, Mar 18, 2019 at 09:46:24PM -0400, Krish Sadhukhan wrote: > >>1. According to section "Checks on Host Control Registers and MSRs" in Intel > >> SDM vol 3C, the following check is performed on vmentry of L2 guests: > >Heh, "of L2 guests" again. > > We are testing VMLAUNCH of L2 in this test. Right, but you're quoting SDM and the SDM has no concept of L2. You could instead word the changelog as: [PATCH 3/3][kvm-unit-test nVMX]: Check "load IA32_PAT" on vmentry of L2 guests ...to verify KVM performs the appropriate consistency checks for loading IA32_PAT as part of running a nested guest. According to section ... > > >> If the “load IA32_PAT” VM-exit control is 1, the value of the field > >> for the IA32_PAT MSR must be one that could be written by WRMSR > >> without fault at CPL 0. Specifically, each of the 8 bytes in the > >> field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), > >> 6 (WB), or 7 (UC-). > >> > >>2. According to section "CHECKING AND LOADING GUEST STATE" in Intel > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Why use all caps here? > > > >> SDM vol 3C, the following check is performed on vmentry of L2 guests: > >> > >> If the "load IA32_PAT" VM-entry control is 1, the value of the field > >> for the IA32_PAT MSR must be one that could be written by WRMSR > >> without fault at CPL 0. Specifically, each of the 8 bytes in the > >> field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), > >> 6 (WB), or 7 (UC-). > >IMO this is a bit gratuitous. The paragraphs are identical except for > >"exit" vs. "entry". A little consolidation: > > > >According to sections "Checks on Host Control Registers and MSRs" and > >"Checking and Loading Guest State" in Intel SDM vol 3c, the following > >checks are performed on VM-Entry: > > > > If the "load IA32_PAT" VM-{entry,exit} control is 1, the value of > > the field for the IA32_PAT MSR must be one that could be written by > > WRMSR without fault at CPL 0. Specifically, each of the 8 bytes in > > the field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), > > 6 (WB), or 7 (UC-). > > > >>Signed-off-by: Krish Sadhukhan<krish.sadhukhan@oracle.com> > >>Reviewed-by: Karl Heubaum<karl.heubaum@oracle.com> ... > One related issue I notice is that the exit reason and the exit > qualification are the same for both a true VM-exit and a VM-exit with > VM-entry failure during guest state checks. For example, for a valid or an > invalid value in the bytes of HOST_PAT/GUEST_PAT, I see the following in > both cases: > > Exit Reason: 0x80000021 > Exit Qualification: 0 > > We are setting bit 31 in Exit Reason even when all guest state checks have > passed successfully and L2 has returned to L1 via a true VM-exit. What > criterion can I use to differentiate between a true VM-exit vs. a VM-exit > with VM-entry failure ? Because it's not actually hitting a "true" VM-Exit. See the comment above vmlaunch_succeeds(): /* * Test for early VMLAUNCH failure. Returns true if VMLAUNCH makes it * at least as far as the guest-state checks. Returns false if the * VMLAUNCH fails early and execution falls through to the next * instruction. */ static bool vmlaunch_succeeds(void) In other words, vmlaunch_succeeds() considers "success" to be reaching the point where VMLAUNCH starts to load guest state, i.e. it doesn't ensure guest state is valid and so can't be used to test an invalid GUEST_PAT.
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c index 66a87f6..7dae0f0 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -4995,6 +4995,90 @@ static void test_sysenter_field(u32 field, const char *name) vmcs_write(field, addr_saved); } +static void test_pat_fields(u64 vmcs_fld, const char * vmcs_fld_name, + u64 ctrl, u64 ctrl_fld) +{ + u32 ctrl_saved = vmcs_read(ctrl); + u64 vmcs_fld_saved = vmcs_read(vmcs_fld); + int i, j, error = 0; + u64 val; + + vmcs_write(ctrl, ctrl_saved & ~ctrl_fld); + for (i = 0; i < 256; i++) { + /* Test PAT0..PAT7 fields */ + for (j = 0; j < 8; j++) { + val = i << j * 8; + vmcs_write(vmcs_fld, val); + report_prefix_pushf("%s %lx", vmcs_fld_name, val); + test_vmx_vmlaunch(error, false); + report_prefix_pop(); + } + } + + vmcs_write(ctrl, ctrl_saved | ctrl_fld); + for (i = 0; i < 256; i++) { + /* Test PAT0..PAT7 fields */ + for (j = 0; j < 8; j++) { + val = i << j * 8; + vmcs_write(vmcs_fld, val); + report_prefix_pushf("%s %lx", vmcs_fld_name, val); + if (i == 0x2 || i == 0x3 || i > 0x7) { + if (vmcs_fld == HOST_PAT) + error = + VMXERR_ENTRY_INVALID_HOST_STATE_FIELD; + else + error = + VMXERR_ENTRY_INVALID_CONTROL_FIELD; + } else { + error = 0; + } + test_vmx_vmlaunch(error, false); + report_prefix_pop(); + } + } + + vmcs_write(ctrl, ctrl_saved); + vmcs_write(vmcs_fld, vmcs_fld_saved); +} + +/* + * 1. If the "load IA32_PAT" VM-exit control is 1, the value of the field + * for the IA32_PAT MSR must be one that could be written by WRMSR + * without fault at CPL 0. Specifically, each of the 8 bytes in the + * field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), + * 6 (WB), or 7 (UC-). + * + * 2. If the "load IA32_PAT" VM-entry control is 1, the value of the field + * for the IA32_PAT MSR must be one that could be written by WRMSR + * without fault at CPL 0. Specifically, each of the 8 bytes in the + * field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), + * 6 (WB), or 7 (UC-). + * + * [Intel SDM] + */ +static void test_load_pat(void) +{ + /* + * "load IA32_PAT" VM-exit control + */ + if (!(ctrl_exit_rev.clr & EXI_LOAD_PAT)) { + printf("\"Load-IA32-PAT\" exit control not supported\n"); + return; + } + + test_pat_fields(HOST_PAT, "HOST_PAT", EXI_CONTROLS, EXI_LOAD_PAT); + + /* + * "load IA32_PAT" VM-entry control + */ + if (!(ctrl_enter_rev.clr & ENT_LOAD_PAT)) { + printf("\"Load-IA32-PAT\" entry control not supported\n"); + return; + } + + test_pat_fields(GUEST_PAT, "GUEST_PAT", ENT_CONTROLS, ENT_LOAD_PAT); +} + /* * Check that the virtual CPU checks the VMX Host State Area as * documented in the Intel SDM. @@ -5010,6 +5094,9 @@ static void vmx_host_state_area_test(void) test_sysenter_field(HOST_SYSENTER_ESP, "HOST_SYSENTER_ESP"); test_sysenter_field(HOST_SYSENTER_EIP, "HOST_SYSENTER_EIP"); + + test_load_pat(); + } static bool valid_vmcs_for_vmentry(void)
1. According to section "Checks on Host Control Registers and MSRs" in Intel SDM vol 3C, the following check is performed on vmentry of L2 guests: If the “load IA32_PAT” VM-exit control is 1, the value of the field for the IA32_PAT MSR must be one that could be written by WRMSR without fault at CPL 0. Specifically, each of the 8 bytes in the field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), 6 (WB), or 7 (UC-). 2. According to section "CHECKING AND LOADING GUEST STATE" in Intel SDM vol 3C, the following check is performed on vmentry of L2 guests: If the "load IA32_PAT" VM-entry control is 1, the value of the field for the IA32_PAT MSR must be one that could be written by WRMSR without fault at CPL 0. Specifically, each of the 8 bytes in the field must have one of the values 0 (UC), 1 (WC), 4 (WT), 5 (WP), 6 (WB), or 7 (UC-). Signed-off-by: Krish Sadhukhan <krish.sadhukhan@oracle.com> Reviewed-by: Karl Heubaum <karl.heubaum@oracle.com> --- x86/vmx_tests.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+)