On 2/24/21 1:59 PM, Sean Christopherson wrote:
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?
Yes
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 RIP advances because KVM is not taking action for the pending
RFLAGS.TF when it executes L1 for the first time after L2 exits. #DB
intercept is triggered only after the instruction next to VMRUN is
executed in svm_vcpu_run and hence the #DB handler for L1 shows the
wrong RIP.
I have just sent out v4 which fixes the problem in a better way.
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.
