| Message ID | 1482180521-71290-1-git-send-email-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Dec 19, 2016 at 12:48 PM, David Matlack <dmatlack@google.com> wrote: > kvm_skip_emulated_instruction() should not be called after emulating > a VM-entry failure during or after loading guest state > (nested_vmx_entry_failure()). Otherwise the L1 hypervisor is resumed > some number of bytes past vmcs->host_rip. Ah, I see. Sorry for that regression. These paths are supposed to trigger TF-induced singlestep exceptions though. Quoting from the Intel SDM (Vol 3, Chapter 26) "EFLAGS.TF = 1 causes a VM-entry instruction to generate a single-step debug exception only if failure of one of the checks in Section 26.1 and Section 26.2 causes control to pass to the following instruction. A VM-entry does not generate a single-step debug exception in any of the following cases: (1) the instruction generates a fault; (2) failure of one of the checks in Section 26.3 or in loading MSRs causes processor state to be loaded from the hoststate area of the VMCS; or (3) the instruction passes all checks in Section 26.1, Section 26.2, and Section 26.3 and there is no failure in loading MSRs" - Kyle > Fixes: eb2775621701e6ee3ea2a474437d04e93ccdcb2f > Signed-off-by: David Matlack <dmatlack@google.com> > --- > arch/x86/kvm/vmx.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > index c41d7ff..0e7ad72 100644 > --- a/arch/x86/kvm/vmx.c > +++ b/arch/x86/kvm/vmx.c > @@ -10474,12 +10474,12 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) > !nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4)) { > nested_vmx_entry_failure(vcpu, vmcs12, > EXIT_REASON_INVALID_STATE, ENTRY_FAIL_DEFAULT); > - goto out; > + return 1; > } > if (vmcs12->vmcs_link_pointer != -1ull) { > nested_vmx_entry_failure(vcpu, vmcs12, > EXIT_REASON_INVALID_STATE, ENTRY_FAIL_VMCS_LINK_PTR); > - goto out; > + return 1; > } > > /* > @@ -10499,7 +10499,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) > ia32e != !!(vmcs12->guest_ia32_efer & EFER_LME))) { > nested_vmx_entry_failure(vcpu, vmcs12, > EXIT_REASON_INVALID_STATE, ENTRY_FAIL_DEFAULT); > - goto out; > + return 1; > } > } > > @@ -10517,7 +10517,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) > ia32e != !!(vmcs12->host_ia32_efer & EFER_LME)) { > nested_vmx_entry_failure(vcpu, vmcs12, > EXIT_REASON_INVALID_STATE, ENTRY_FAIL_DEFAULT); > - goto out; > + return 1; > } > } > > -- > 2.8.0.rc3.226.g39d4020 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-12-19 19:15-0800, Kyle Huey: > On Mon, Dec 19, 2016 at 12:48 PM, David Matlack <dmatlack@google.com> wrote: >> kvm_skip_emulated_instruction() should not be called after emulating >> a VM-entry failure during or after loading guest state >> (nested_vmx_entry_failure()). Otherwise the L1 hypervisor is resumed >> some number of bytes past vmcs->host_rip. > > Ah, I see. Sorry for that regression. > > These paths are supposed to trigger TF-induced singlestep exceptions > though. Quoting from the Intel SDM (Vol 3, Chapter 26) > > "EFLAGS.TF = 1 causes a VM-entry instruction to generate a single-step > debug exception only if failure of one of the checks in Section 26.1 > and Section 26.2 causes control to pass to the following instruction. > A VM-entry does not generate a single-step debug exception in any of > the following cases: (1) the instruction generates a fault; (2) > failure of one of the checks in Section 26.3 or in loading MSRs causes > processor state to be loaded from the hoststate area of the VMCS; or > (3) the instruction passes all checks in Section 26.1, Section 26.2, > and Section 26.3 and there is no failure in loading MSRs" Changed cases are in section 26.3 => not generating #DB is correct, Reviewed-by: Radim Krčmář <rkrcmar@redhat.com> Sorry for missing that while applying, I wonder if there is a reason why we didn't check them after enter_guest_mode() ... >> Fixes: eb2775621701e6ee3ea2a474437d04e93ccdcb2f >> Signed-off-by: David Matlack <dmatlack@google.com> >> --- >> arch/x86/kvm/vmx.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >> index c41d7ff..0e7ad72 100644 >> --- a/arch/x86/kvm/vmx.c >> +++ b/arch/x86/kvm/vmx.c >> @@ -10474,12 +10474,12 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) >> !nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4)) { >> nested_vmx_entry_failure(vcpu, vmcs12, >> EXIT_REASON_INVALID_STATE, ENTRY_FAIL_DEFAULT); >> - goto out; >> + return 1; >> } >> if (vmcs12->vmcs_link_pointer != -1ull) { >> nested_vmx_entry_failure(vcpu, vmcs12, >> EXIT_REASON_INVALID_STATE, ENTRY_FAIL_VMCS_LINK_PTR); >> - goto out; >> + return 1; >> } >> >> /* >> @@ -10499,7 +10499,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) >> ia32e != !!(vmcs12->guest_ia32_efer & EFER_LME))) { >> nested_vmx_entry_failure(vcpu, vmcs12, >> EXIT_REASON_INVALID_STATE, ENTRY_FAIL_DEFAULT); >> - goto out; >> + return 1; >> } >> } >> >> @@ -10517,7 +10517,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) >> ia32e != !!(vmcs12->host_ia32_efer & EFER_LME)) { >> nested_vmx_entry_failure(vcpu, vmcs12, >> EXIT_REASON_INVALID_STATE, ENTRY_FAIL_DEFAULT); >> - goto out; >> + return 1; >> } >> } >> >> -- >> 2.8.0.rc3.226.g39d4020 >> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-12-20 16:24+0100, Radim Krčmář: > 2016-12-19 19:15-0800, Kyle Huey: > > On Mon, Dec 19, 2016 at 12:48 PM, David Matlack <dmatlack@google.com> wrote: > >> kvm_skip_emulated_instruction() should not be called after emulating > >> a VM-entry failure during or after loading guest state > >> (nested_vmx_entry_failure()). Otherwise the L1 hypervisor is resumed > >> some number of bytes past vmcs->host_rip. > > > > Ah, I see. Sorry for that regression. > > > > These paths are supposed to trigger TF-induced singlestep exceptions > > though. Quoting from the Intel SDM (Vol 3, Chapter 26) > > > > "EFLAGS.TF = 1 causes a VM-entry instruction to generate a single-step > > debug exception only if failure of one of the checks in Section 26.1 > > and Section 26.2 causes control to pass to the following instruction. > > A VM-entry does not generate a single-step debug exception in any of > > the following cases: (1) the instruction generates a fault; (2) > > failure of one of the checks in Section 26.3 or in loading MSRs causes > > processor state to be loaded from the hoststate area of the VMCS; or > > (3) the instruction passes all checks in Section 26.1, Section 26.2, > > and Section 26.3 and there is no failure in loading MSRs" > > Changed cases are in section 26.3 => not generating #DB is correct, > > Reviewed-by: Radim Krčmář <rkrcmar@redhat.com> > > Sorry for missing that while applying, I wonder if there is a reason why > we didn't check them after enter_guest_mode() ... > > >> @@ -10517,7 +10517,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) > >> ia32e != !!(vmcs12->host_ia32_efer & EFER_LME)) { > >> nested_vmx_entry_failure(vcpu, vmcs12, > >> EXIT_REASON_INVALID_STATE, ENTRY_FAIL_DEFAULT); > >> - goto out; > >> + return 1; My attention is in shambles these days ... this one looks like it is in section 26.2.2 and should therefore be nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); goto out; Not a problem of this patch though, I'll go for a minor refactoring after a beer. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 20, 2016 at 9:47 AM, Radim Krčmář <rkrcmar@redhat.com> wrote: > 2016-12-20 16:24+0100, Radim Krčmář: >> 2016-12-19 19:15-0800, Kyle Huey: >> > On Mon, Dec 19, 2016 at 12:48 PM, David Matlack <dmatlack@google.com> wrote: >> >> kvm_skip_emulated_instruction() should not be called after emulating >> >> a VM-entry failure during or after loading guest state >> >> (nested_vmx_entry_failure()). Otherwise the L1 hypervisor is resumed >> >> some number of bytes past vmcs->host_rip. >> > >> > Ah, I see. Sorry for that regression. >> > >> > These paths are supposed to trigger TF-induced singlestep exceptions >> > though. Quoting from the Intel SDM (Vol 3, Chapter 26) >> > >> > "EFLAGS.TF = 1 causes a VM-entry instruction to generate a single-step >> > debug exception only if failure of one of the checks in Section 26.1 >> > and Section 26.2 causes control to pass to the following instruction. >> > A VM-entry does not generate a single-step debug exception in any of >> > the following cases: (1) the instruction generates a fault; (2) >> > failure of one of the checks in Section 26.3 or in loading MSRs causes >> > processor state to be loaded from the hoststate area of the VMCS; or >> > (3) the instruction passes all checks in Section 26.1, Section 26.2, >> > and Section 26.3 and there is no failure in loading MSRs" >> >> Changed cases are in section 26.3 => not generating #DB is correct, >> >> Reviewed-by: Radim Krčmář <rkrcmar@redhat.com> >> >> Sorry for missing that while applying, I wonder if there is a reason why >> we didn't check them after enter_guest_mode() ... I think this would be a nice cleanup. >> >> >> @@ -10517,7 +10517,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) >> >> ia32e != !!(vmcs12->host_ia32_efer & EFER_LME)) { >> >> nested_vmx_entry_failure(vcpu, vmcs12, >> >> EXIT_REASON_INVALID_STATE, ENTRY_FAIL_DEFAULT); >> >> - goto out; >> >> + return 1; > > My attention is in shambles these days ... this one looks like it is in > section 26.2.2 and should therefore be > > nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_HOST_STATE_FIELD); > goto out; I agree. Good catch. > > Not a problem of this patch though, I'll go for a minor refactoring > after a beer. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index c41d7ff..0e7ad72 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -10474,12 +10474,12 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) !nested_guest_cr4_valid(vcpu, vmcs12->guest_cr4)) { nested_vmx_entry_failure(vcpu, vmcs12, EXIT_REASON_INVALID_STATE, ENTRY_FAIL_DEFAULT); - goto out; + return 1; } if (vmcs12->vmcs_link_pointer != -1ull) { nested_vmx_entry_failure(vcpu, vmcs12, EXIT_REASON_INVALID_STATE, ENTRY_FAIL_VMCS_LINK_PTR); - goto out; + return 1; } /* @@ -10499,7 +10499,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) ia32e != !!(vmcs12->guest_ia32_efer & EFER_LME))) { nested_vmx_entry_failure(vcpu, vmcs12, EXIT_REASON_INVALID_STATE, ENTRY_FAIL_DEFAULT); - goto out; + return 1; } } @@ -10517,7 +10517,7 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch) ia32e != !!(vmcs12->host_ia32_efer & EFER_LME)) { nested_vmx_entry_failure(vcpu, vmcs12, EXIT_REASON_INVALID_STATE, ENTRY_FAIL_DEFAULT); - goto out; + return 1; } }
kvm_skip_emulated_instruction() should not be called after emulating a VM-entry failure during or after loading guest state (nested_vmx_entry_failure()). Otherwise the L1 hypervisor is resumed some number of bytes past vmcs->host_rip. Fixes: eb2775621701e6ee3ea2a474437d04e93ccdcb2f Signed-off-by: David Matlack <dmatlack@google.com> --- arch/x86/kvm/vmx.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)