On Thu, 2021-08-26 at 16:01 +0000, Sean Christopherson wrote: > On Thu, Aug 26, 2021, Maxim Levitsky wrote: > > If we are emulating an invalid guest state, we don't have a correct > > exit reason, and thus we shouldn't do anything in this function. > > > > Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx> > > This should have Cc: stable. I believe userspace could fairly easily trick KVM > into "handling" a spurious IRQ, e.g. trigger SIGALRM and stuff invalid state. > For all those evil folks running CPUs that are almost old enough to drive :-) > > > --- > > arch/x86/kvm/vmx/vmx.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > > index fada1055f325..0c2c0d5ae873 100644 > > --- a/arch/x86/kvm/vmx/vmx.c > > +++ b/arch/x86/kvm/vmx/vmx.c > > @@ -6382,6 +6382,9 @@ static void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu) > > { > > struct vcpu_vmx *vmx = to_vmx(vcpu); > > > > + if (vmx->emulation_required) > > + return; > > Rather than play whack-a-mole with flows consuming stale state, I'd much prefer > to synthesize a VM-Exit(INVALID_GUEST_STATE). Alternatively, just skip ->run() > entirely by adding hooks in vcpu_enter_guest(), but that's a much larger change > and probably not worth the risk at this juncture. > > --- > arch/x86/kvm/vmx/vmx.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 32e3a8b35b13..12fe63800889 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -6618,10 +6618,21 @@ static fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu) > vmx->loaded_vmcs->soft_vnmi_blocked)) > vmx->loaded_vmcs->entry_time = ktime_get(); > > - /* Don't enter VMX if guest state is invalid, let the exit handler > - start emulation until we arrive back to a valid state */ > - if (vmx->emulation_required) > + /* > + * Don't enter VMX if guest state is invalid, let the exit handler > + * start emulation until we arrive back to a valid state. Synthesize a > + * consistency check VM-Exit due to invalid guest state and bail. > + */ > + if (unlikely(vmx->emulation_required)) { > + vmx->fail = 0; > + vmx->exit_reason.full = EXIT_REASON_INVALID_STATE; > + vmx->exit_reason.failed_vmentry = 1; > + kvm_register_mark_available(vcpu, VCPU_EXREG_EXIT_INFO_1); > + vmx->exit_qualification = ENTRY_FAIL_DEFAULT; > + kvm_register_mark_available(vcpu, VCPU_EXREG_EXIT_INFO_2); > + vmx->exit_intr_info = 0; > return EXIT_FASTPATH_NONE; > + } I was thinking exactly about this when I wrote the patch, and in fact first version of it did roughly what you suggest. But I was afraid that this will also introduce a whack-a-mole as now it "appears" as if VM entry failed and we should thus kill the guest. But I'll try that. Thanks a lot for the review! Best regards, Maxim Levitsky > > trace_kvm_entry(vcpu); > > -- > > or the beginnings of an aggressive refactor... > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index cf8fb6eb676a..a4fe0f78898a 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -9509,6 +9509,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > goto cancel_injection; > } > > + if (unlikely(static_call(kvm_x86_emulation_required)(vcpu))) > + return static_call(kvm_x86_emulate_invalid_guest_state)(vcpu); > + > preempt_disable(); > > static_call(kvm_x86_prepare_guest_switch)(vcpu); > > > + > > if (vmx->exit_reason.basic == EXIT_REASON_EXTERNAL_INTERRUPT) > > handle_external_interrupt_irqoff(vcpu); > > else if (vmx->exit_reason.basic == EXIT_REASON_EXCEPTION_NMI) > > -- > > 2.26.3 > >