| Message ID | 20190212230402.26851-4-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nVMX: Fix unrestricted guest toggling | expand |
On Tue, Feb 12, 2019 at 3:04 PM Sean Christopherson <sean.j.christopherson@intel.com> wrote: > > The hardware exception injection test toggles unrestricted guest so that > it can test the case where an event is injected into real mode with and > without an error code (exception error codes don't exist in real mode). > Unrestricted guest has its own requirements, specifically that EPT is > also enabled (since IA32 paging could be disabled). > > Unfortunately, the enable_unrestricted_guest() helper fails to ensure > EPT is enabled, which causes all subsequent VMLAUNCH instructions to > fail with "invalid control field". Use the new added enable_ept() to > configure unrestricted guest. In addition, assert that unrestricted > guest is disabled at the beginning of the relevant section as things > will likely go sideways if unrestricted guest is already enabled, e.g. > odds are good it was enabled in order to muck with CR0. This allows > for the removal of disable_unrestricted_guest() entirely. And finally, > clean up the control fields after finishing the unrestricted guest > section (instead of invoking the defunct disable_unrestricted_guest()). > > Note that it's not the unrestricted guest tests that fail, since there > is no "vmlaunch succeeds" variant, rather its the following tests that > expect success that end up failing (because the shoddy enabling of URG > isn't undone). > > Fixes: 8d2cdb3 ("x86: Add test for nested VM entry prereqs") > Cc: Marc Orr <marcorr@google.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > x86/vmx.h | 18 ------------------ > x86/vmx_tests.c | 32 +++++++++++++++++++++++++++++--- > 2 files changed, 29 insertions(+), 21 deletions(-) > > diff --git a/x86/vmx.h b/x86/vmx.h > index 8a00f73..bcfaa79 100644 > --- a/x86/vmx.h > +++ b/x86/vmx.h > @@ -779,24 +779,6 @@ static inline bool invvpid(unsigned long type, u64 vpid, u64 gla) > return ret; > } > > -static inline int enable_unrestricted_guest(void) > -{ > - if (!(ctrl_cpu_rev[0].clr & CPU_SECONDARY)) > - return -1; > - > - if (!(ctrl_cpu_rev[1].clr & CPU_URG)) > - return -1; > - > - vmcs_write(CPU_EXEC_CTRL0, vmcs_read(CPU_EXEC_CTRL0) | CPU_SECONDARY); > - vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) | CPU_URG); > - return 0; > -} > - > -static inline void disable_unrestricted_guest(void) > -{ > - vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) & ~CPU_URG); > -} > - > const char *exit_reason_description(u64 reason); > void print_vmexit_info(void); > void print_vmentry_failure_info(struct vmentry_failure *failure); > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > index 4cfb55f..ff51185 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -1067,6 +1067,21 @@ static int enable_ept(void) > return setup_eptp(0, false); > } > > +static int enable_unrestricted_guest(void) > +{ > + if (!(ctrl_cpu_rev[0].clr & CPU_SECONDARY) || > + !(ctrl_cpu_rev[1].clr & CPU_URG)) > + return 1; > + > + if (enable_ept()) > + return 1; > + > + vmcs_write(CPU_EXEC_CTRL0, vmcs_read(CPU_EXEC_CTRL0) | CPU_SECONDARY); > + vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) | CPU_URG); > + > + return 0; > +} > + > static void ept_enable_ad_bits(void) > { > eptp |= EPTP_AD_FLAG; > @@ -4081,12 +4096,16 @@ static void test_invalid_event_injection(void) > * (a) the "unrestricted guest" VM-execution control is 0 > * (b) CR0.PE is set. > */ > + > + /* Assert that unrestricted guest is disabled or unsupported */ > + assert(!(ctrl_cpu_rev[0].clr & CPU_SECONDARY) || > + !(secondary_save & CPU_URG)); > + > ent_intr_info = ent_intr_info_base | INTR_TYPE_HARD_EXCEPTION | > GP_VECTOR; > report_prefix_pushf("%s, VM-entry intr info=0x%x", > "error code <-> (!URG || prot_mode) [-]", > ent_intr_info); > - disable_unrestricted_guest(); > vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG); > vmcs_write(ENT_INTR_INFO, ent_intr_info); > test_vmx_controls(false, false); > @@ -4097,18 +4116,19 @@ static void test_invalid_event_injection(void) > report_prefix_pushf("%s, VM-entry intr info=0x%x", > "error code <-> (!URG || prot_mode) [+]", > ent_intr_info); > - disable_unrestricted_guest(); > vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG); > vmcs_write(ENT_INTR_INFO, ent_intr_info); > test_vmx_controls(true, false); > report_prefix_pop(); > > + if (enable_unrestricted_guest()) > + goto skip_unrestricted_guest; > + > ent_intr_info = ent_intr_info_base | INTR_INFO_DELIVER_CODE_MASK | > INTR_TYPE_HARD_EXCEPTION | GP_VECTOR; > report_prefix_pushf("%s, VM-entry intr info=0x%x", > "error code <-> (!URG || prot_mode) [-]", > ent_intr_info); > - enable_unrestricted_guest(); > vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG); > vmcs_write(ENT_INTR_INFO, ent_intr_info); > test_vmx_controls(false, false); > @@ -4124,6 +4144,12 @@ static void test_invalid_event_injection(void) > test_vmx_controls(false, false); > report_prefix_pop(); > > + vmcs_write(CPU_EXEC_CTRL1, secondary_save); > + vmcs_write(CPU_EXEC_CTRL0, primary_save); Writing these inside of a helper, enable_unrestricted_guest(), and restoring them explicitly here, seems oddly asymmetric to me. I guess I'd prefer to see these vmcs_write() operations in a helper that corresponds to enable_unrestricted_guest(). At a minimum, I think we should put a comment here, to say that these vmcs_write() operations are to restore these fields to their prior state, before enable_unrestricted_guest(). > + > +skip_unrestricted_guest: > + vmcs_write(GUEST_CR0, guest_cr0_save); > + > /* deliver-error-code is 1 iff the interruption type is HW exception */ > report_prefix_push("error code <-> HW exception"); > for (cnt = 0; cnt < 8; cnt++) { > -- > 2.20.1 > Reviewed-by: Marc Orr <marcorr@google.com>
On 02/12/2019 03:04 PM, Sean Christopherson wrote: > The hardware exception injection test toggles unrestricted guest so that > it can test the case where an event is injected into real mode with and > without an error code (exception error codes don't exist in real mode). > Unrestricted guest has its own requirements, specifically that EPT is > also enabled (since IA32 paging could be disabled). > > Unfortunately, the enable_unrestricted_guest() helper fails to ensure > EPT is enabled, which causes all subsequent VMLAUNCH instructions to > fail with "invalid control field". Use the new added enable_ept() to > configure unrestricted guest. In addition, assert that unrestricted > guest is disabled at the beginning of the relevant section as things > will likely go sideways if unrestricted guest is already enabled, e.g. > odds are good it was enabled in order to muck with CR0. This allows > for the removal of disable_unrestricted_guest() entirely. And finally, > clean up the control fields after finishing the unrestricted guest > section (instead of invoking the defunct disable_unrestricted_guest()). > > Note that it's not the unrestricted guest tests that fail, since there > is no "vmlaunch succeeds" variant, rather its the following tests that > expect success that end up failing (because the shoddy enabling of URG > isn't undone). > > Fixes: 8d2cdb3 ("x86: Add test for nested VM entry prereqs") > Cc: Marc Orr <marcorr@google.com> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > x86/vmx.h | 18 ------------------ > x86/vmx_tests.c | 32 +++++++++++++++++++++++++++++--- > 2 files changed, 29 insertions(+), 21 deletions(-) > > diff --git a/x86/vmx.h b/x86/vmx.h > index 8a00f73..bcfaa79 100644 > --- a/x86/vmx.h > +++ b/x86/vmx.h > @@ -779,24 +779,6 @@ static inline bool invvpid(unsigned long type, u64 vpid, u64 gla) > return ret; > } > > -static inline int enable_unrestricted_guest(void) > -{ > - if (!(ctrl_cpu_rev[0].clr & CPU_SECONDARY)) > - return -1; > - > - if (!(ctrl_cpu_rev[1].clr & CPU_URG)) > - return -1; > - > - vmcs_write(CPU_EXEC_CTRL0, vmcs_read(CPU_EXEC_CTRL0) | CPU_SECONDARY); > - vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) | CPU_URG); > - return 0; > -} > - > -static inline void disable_unrestricted_guest(void) > -{ > - vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) & ~CPU_URG); > -} > - > const char *exit_reason_description(u64 reason); > void print_vmexit_info(void); > void print_vmentry_failure_info(struct vmentry_failure *failure); > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c > index 4cfb55f..ff51185 100644 > --- a/x86/vmx_tests.c > +++ b/x86/vmx_tests.c > @@ -1067,6 +1067,21 @@ static int enable_ept(void) > return setup_eptp(0, false); > } > > +static int enable_unrestricted_guest(void) > +{ > + if (!(ctrl_cpu_rev[0].clr & CPU_SECONDARY) || > + !(ctrl_cpu_rev[1].clr & CPU_URG)) > + return 1; > + > + if (enable_ept()) > + return 1; > + > + vmcs_write(CPU_EXEC_CTRL0, vmcs_read(CPU_EXEC_CTRL0) | CPU_SECONDARY); > + vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) | CPU_URG); > + > + return 0; > +} > + > static void ept_enable_ad_bits(void) > { > eptp |= EPTP_AD_FLAG; > @@ -4081,12 +4096,16 @@ static void test_invalid_event_injection(void) > * (a) the "unrestricted guest" VM-execution control is 0 > * (b) CR0.PE is set. > */ > + > + /* Assert that unrestricted guest is disabled or unsupported */ > + assert(!(ctrl_cpu_rev[0].clr & CPU_SECONDARY) || Instead of this being an assert(), shouldn't this be a check ? If CPU_SECONDARY is unsupported, other existing functions don't run any test and just return. > + !(secondary_save & CPU_URG)); > + > ent_intr_info = ent_intr_info_base | INTR_TYPE_HARD_EXCEPTION | > GP_VECTOR; > report_prefix_pushf("%s, VM-entry intr info=0x%x", > "error code <-> (!URG || prot_mode) [-]", > ent_intr_info); > - disable_unrestricted_guest(); > vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG); > vmcs_write(ENT_INTR_INFO, ent_intr_info); > test_vmx_controls(false, false); > @@ -4097,18 +4116,19 @@ static void test_invalid_event_injection(void) > report_prefix_pushf("%s, VM-entry intr info=0x%x", > "error code <-> (!URG || prot_mode) [+]", > ent_intr_info); > - disable_unrestricted_guest(); > vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG); > vmcs_write(ENT_INTR_INFO, ent_intr_info); > test_vmx_controls(true, false); > report_prefix_pop(); > > + if (enable_unrestricted_guest()) > + goto skip_unrestricted_guest; > + > ent_intr_info = ent_intr_info_base | INTR_INFO_DELIVER_CODE_MASK | > INTR_TYPE_HARD_EXCEPTION | GP_VECTOR; > report_prefix_pushf("%s, VM-entry intr info=0x%x", > "error code <-> (!URG || prot_mode) [-]", > ent_intr_info); > - enable_unrestricted_guest(); > vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG); > vmcs_write(ENT_INTR_INFO, ent_intr_info); > test_vmx_controls(false, false); > @@ -4124,6 +4144,12 @@ static void test_invalid_event_injection(void) > test_vmx_controls(false, false); > report_prefix_pop(); > > + vmcs_write(CPU_EXEC_CTRL1, secondary_save); > + vmcs_write(CPU_EXEC_CTRL0, primary_save); Restoring these controls outside of a function, makes the code less readable. We are calling enable_unrestricted_guest() just once, so there is really no need for a separate function. You can just do these where you are calling the function: + if (enable_ept()) + return 1; + + vmcs_write(CPU_EXEC_CTRL0, vmcs_read(CPU_EXEC_CTRL0) | CPU_SECONDARY); + vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) | CPU_URG); > + > +skip_unrestricted_guest: > + vmcs_write(GUEST_CR0, guest_cr0_save); > + > /* deliver-error-code is 1 iff the interruption type is HW exception */ > report_prefix_push("error code <-> HW exception"); > for (cnt = 0; cnt < 8; cnt++) {
diff --git a/x86/vmx.h b/x86/vmx.h index 8a00f73..bcfaa79 100644 --- a/x86/vmx.h +++ b/x86/vmx.h @@ -779,24 +779,6 @@ static inline bool invvpid(unsigned long type, u64 vpid, u64 gla) return ret; } -static inline int enable_unrestricted_guest(void) -{ - if (!(ctrl_cpu_rev[0].clr & CPU_SECONDARY)) - return -1; - - if (!(ctrl_cpu_rev[1].clr & CPU_URG)) - return -1; - - vmcs_write(CPU_EXEC_CTRL0, vmcs_read(CPU_EXEC_CTRL0) | CPU_SECONDARY); - vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) | CPU_URG); - return 0; -} - -static inline void disable_unrestricted_guest(void) -{ - vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) & ~CPU_URG); -} - const char *exit_reason_description(u64 reason); void print_vmexit_info(void); void print_vmentry_failure_info(struct vmentry_failure *failure); diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c index 4cfb55f..ff51185 100644 --- a/x86/vmx_tests.c +++ b/x86/vmx_tests.c @@ -1067,6 +1067,21 @@ static int enable_ept(void) return setup_eptp(0, false); } +static int enable_unrestricted_guest(void) +{ + if (!(ctrl_cpu_rev[0].clr & CPU_SECONDARY) || + !(ctrl_cpu_rev[1].clr & CPU_URG)) + return 1; + + if (enable_ept()) + return 1; + + vmcs_write(CPU_EXEC_CTRL0, vmcs_read(CPU_EXEC_CTRL0) | CPU_SECONDARY); + vmcs_write(CPU_EXEC_CTRL1, vmcs_read(CPU_EXEC_CTRL1) | CPU_URG); + + return 0; +} + static void ept_enable_ad_bits(void) { eptp |= EPTP_AD_FLAG; @@ -4081,12 +4096,16 @@ static void test_invalid_event_injection(void) * (a) the "unrestricted guest" VM-execution control is 0 * (b) CR0.PE is set. */ + + /* Assert that unrestricted guest is disabled or unsupported */ + assert(!(ctrl_cpu_rev[0].clr & CPU_SECONDARY) || + !(secondary_save & CPU_URG)); + ent_intr_info = ent_intr_info_base | INTR_TYPE_HARD_EXCEPTION | GP_VECTOR; report_prefix_pushf("%s, VM-entry intr info=0x%x", "error code <-> (!URG || prot_mode) [-]", ent_intr_info); - disable_unrestricted_guest(); vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG); vmcs_write(ENT_INTR_INFO, ent_intr_info); test_vmx_controls(false, false); @@ -4097,18 +4116,19 @@ static void test_invalid_event_injection(void) report_prefix_pushf("%s, VM-entry intr info=0x%x", "error code <-> (!URG || prot_mode) [+]", ent_intr_info); - disable_unrestricted_guest(); vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG); vmcs_write(ENT_INTR_INFO, ent_intr_info); test_vmx_controls(true, false); report_prefix_pop(); + if (enable_unrestricted_guest()) + goto skip_unrestricted_guest; + ent_intr_info = ent_intr_info_base | INTR_INFO_DELIVER_CODE_MASK | INTR_TYPE_HARD_EXCEPTION | GP_VECTOR; report_prefix_pushf("%s, VM-entry intr info=0x%x", "error code <-> (!URG || prot_mode) [-]", ent_intr_info); - enable_unrestricted_guest(); vmcs_write(GUEST_CR0, guest_cr0_save & ~X86_CR0_PE & ~X86_CR0_PG); vmcs_write(ENT_INTR_INFO, ent_intr_info); test_vmx_controls(false, false); @@ -4124,6 +4144,12 @@ static void test_invalid_event_injection(void) test_vmx_controls(false, false); report_prefix_pop(); + vmcs_write(CPU_EXEC_CTRL1, secondary_save); + vmcs_write(CPU_EXEC_CTRL0, primary_save); + +skip_unrestricted_guest: + vmcs_write(GUEST_CR0, guest_cr0_save); + /* deliver-error-code is 1 iff the interruption type is HW exception */ report_prefix_push("error code <-> HW exception"); for (cnt = 0; cnt < 8; cnt++) {
The hardware exception injection test toggles unrestricted guest so that it can test the case where an event is injected into real mode with and without an error code (exception error codes don't exist in real mode). Unrestricted guest has its own requirements, specifically that EPT is also enabled (since IA32 paging could be disabled). Unfortunately, the enable_unrestricted_guest() helper fails to ensure EPT is enabled, which causes all subsequent VMLAUNCH instructions to fail with "invalid control field". Use the new added enable_ept() to configure unrestricted guest. In addition, assert that unrestricted guest is disabled at the beginning of the relevant section as things will likely go sideways if unrestricted guest is already enabled, e.g. odds are good it was enabled in order to muck with CR0. This allows for the removal of disable_unrestricted_guest() entirely. And finally, clean up the control fields after finishing the unrestricted guest section (instead of invoking the defunct disable_unrestricted_guest()). Note that it's not the unrestricted guest tests that fail, since there is no "vmlaunch succeeds" variant, rather its the following tests that expect success that end up failing (because the shoddy enabling of URG isn't undone). Fixes: 8d2cdb3 ("x86: Add test for nested VM entry prereqs") Cc: Marc Orr <marcorr@google.com> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- x86/vmx.h | 18 ------------------ x86/vmx_tests.c | 32 +++++++++++++++++++++++++++++--- 2 files changed, 29 insertions(+), 21 deletions(-)