Re: [PATCH v5 1/3] KVM: X86: Save&restore the triple fault request

[Sean Christopherson]

On Tue, Apr 05, 2022, Sean Christopherson wrote:
> On Tue, Apr 05, 2022, Sean Christopherson wrote:
> > On Fri, Mar 18, 2022, Chenyi Qiang wrote:
> > > @@ -4976,6 +4980,9 @@ static int kvm_vcpu_ioctl_x86_set_vcpu_events(struct kvm_vcpu *vcpu,
> > >  		}
> > >  	}
> > >  
> > > +	if (events->flags & KVM_VCPUEVENT_TRIPLE_FAULT)
> > > +		kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
> > > +
> > >  	kvm_make_request(KVM_REQ_EVENT, vcpu);
> > 
> > Looks correct, but this really needs a selftest, at least for the SET path since
> > the intent is to use that for the NOTIFY handling.  Doesn't need to be super fancy,
> > e.g. do port I/O from L2, inject a triple fault, and verify L1 sees the appropriate
> > exit.
> > 
> > Aha!  And for the GET path, abuse KVM_X86_SET_MCE with CR4.MCE=0 to coerce KVM into
> > making a KVM_REQ_TRIPLE_FAULT, that way there's no need to try and hit a timing
> > window to intercept the request.
> 
> Drat, I bet that MCE path means the WARN in nested_vmx_vmexit() can be triggered
> by userspace.  If so, this patch makes it really, really easy to hit, e.g. queue the
> request while L2 is active, then do KVM_SET_NESTED_STATE to force an "exit" without
> bouncing through kvm_check_nested_events().
> 
>   WARN_ON_ONCE(kvm_check_request(KVM_REQ_TRIPLE_FAULT, vcpu))
> 
> I don't think SVM has a user-triggerable WARN, but the request should still be
> dropped on forced exit from L2, e.g. I believe this is the correct fix:
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 9a6dc2b38fcf..18c5e96b12a5 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -1128,6 +1128,7 @@ void svm_leave_nested(struct kvm_vcpu *vcpu)
>         if (is_guest_mode(vcpu)) {
>                 svm->nested.nested_run_pending = 0;
>                 svm->nested.vmcb12_gpa = INVALID_GPA;
> +               kvm_clear_request(KVM_REQ_TRIPLE_FAULT, vcpu);
>  
>                 leave_guest_mode(vcpu);
>  
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index f18744f7ff82..0587ef647553 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -6276,6 +6276,7 @@ void vmx_leave_nested(struct kvm_vcpu *vcpu)
>  {
>         if (is_guest_mode(vcpu)) {
>                 to_vmx(vcpu)->nested.nested_run_pending = 0;
> +               kvm_clear_request(KVM_REQ_TRIPLE_FAULT, vcpu);
>                 nested_vmx_vmexit(vcpu, -1, 0, 0);
>         }
>         free_nested(vcpu);
> 

Wrong again.  Once your patch to save/restore via KVM_VCPUEVENT_TRIPLE_FAULT
lands, clearing KVM_REQ_TRIPLE_FAULT is wrong because that will result in KVM
dropping the request when forcing an exit from L2 in order to reload L2 state,
e.g. if userspace restores events before nested state.  So I think the only thing
that can be done is to delete the WARN :-(