Re: [PATCH 1/4 v3] KVM: nSVM: Do not advance RIP following VMRUN completion if the latter is single-stepped

From: Sean Christopherson <seanjc@google.com>
To: Krish Sadhukhan <krish.sadhukhan@oracle.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, jmattson@google.com
Subject: Re: [PATCH 1/4 v3] KVM: nSVM: Do not advance RIP following VMRUN completion if the latter is single-stepped
Date: Wed, 24 Feb 2021 13:59:23 -0800	[thread overview]
Message-ID: <YDbMOxqQLw5Q2Iy1@google.com> (raw)
In-Reply-To: <0e553de2-2797-9811-b2a4-8d1467ab64e8@oracle.com>

On Wed, Feb 24, 2021, Krish Sadhukhan wrote:
> 
> On 2/23/21 2:42 PM, Sean Christopherson wrote:
> > On Tue, Feb 23, 2021, Krish Sadhukhan wrote:
> > > Currently, svm_vcpu_run() advances the RIP following VMRUN completion when
> > > control returns to host. This works fine if there is no trap flag set
> > > on the VMRUN instruction i.e., if VMRUN is not single-stepped. But if
> > > VMRUN is single-stepped, this advancement of the RIP leads to an incorrect
> > > RIP in the #DB handler invoked for the single-step trap. Therefore, check

Whose #DB handler?  L1's?

> > > if the VMRUN instruction is single-stepped and if so, do not advance the RIP
> > > when the #DB intercept #VMEXIT happens.
> > This really needs to clarify which VMRUN, i.e. L0 vs. L1.  AFAICT, you're
> > talking about both at separate times.  Is this an issue with L1 single-stepping
> > its VMRUN, L0 single-stepping its VMRUN, L0 single-stepping L1's VMRUN, ???
> 
> 
> The issue is seen when L1 single-steps its own VMRUN. I will fix the
> wording.

...

> > > @@ -3827,7 +3833,11 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu)
> > >   		vcpu->arch.cr2 = svm->vmcb->save.cr2;
> > >   		vcpu->arch.regs[VCPU_REGS_RAX] = svm->vmcb->save.rax;
> > >   		vcpu->arch.regs[VCPU_REGS_RSP] = svm->vmcb->save.rsp;
> > > -		vcpu->arch.regs[VCPU_REGS_RIP] = svm->vmcb->save.rip;
> > > +		if (single_step_vmrun && svm->vmcb->control.exit_code ==
> > > +		    SVM_EXIT_EXCP_BASE + DB_VECTOR)
> > > +			single_step_vmrun = false;
> > Even if you fix the global flag issue, this can't possibly work if userspace
> > changes state, if VMRUN fails and leaves a timebomb, and probably any number of
> > other conditions.
> 
> 
>  Are you saying that I need to adjust the RIP in cases where VMRUN fails ?

If VMRUN fails, the #DB exit will never occur and single_step_vmrun will be left
set.  Ditto if a higher priority exit occurs, though I'm not sure that can cause
problems in practice.  Anyways, my point is that setting a flag that must be
consumed on an exact instruction is almost always fragile, there are just too
many corner cases that pop up.

Can you elaborate more on who/what incorrectly advances RIP?  The changelog says
"svm_vcpu_run() advances the RIP", but it's not advancing anything it's just
grabbing RIP from the VMCB, which IIUC is L2's RIP.