Re: [PATCH 5/8] KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction

From: Sean Christopherson <seanjc@google.com>
To: "Maciej S. Szmigiero" <mail@maciej.szmigiero.name>
Cc: Maxim Levitsky <mlevitsk@redhat.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 5/8] KVM: SVM: Re-inject INT3/INTO instead of retrying the instruction
Date: Mon, 4 Apr 2022 19:54:29 +0000	[thread overview]
Message-ID: <YktM9bnq5HaTMKkV@google.com> (raw)
In-Reply-To: <YktIGHM86jHkzGdF@google.com>

On Mon, Apr 04, 2022, Sean Christopherson wrote:
> On Mon, Apr 04, 2022, Maciej S. Szmigiero wrote:
> > > > > index 47e7427d0395..a770a1c7ddd2 100644
> > > > > --- a/arch/x86/kvm/svm/svm.h
> > > > > +++ b/arch/x86/kvm/svm/svm.h
> > > > > @@ -230,8 +230,8 @@ struct vcpu_svm {
> > > > >   	bool nmi_singlestep;
> > > > >   	u64 nmi_singlestep_guest_rflags;
> > > > > -	unsigned int3_injected;
> > > > > -	unsigned long int3_rip;
> > > > > +	unsigned soft_int_injected;
> > > > > +	unsigned long soft_int_linear_rip;
> > > > >   	/* optional nested SVM features that are enabled for this guest  */
> > > > >   	bool nrips_enabled                : 1;
> > > > 
> > > > 
> > > > I mostly agree with this patch, but think that it doesn't address the
> > > > original issue that Maciej wanted to address:
> > > > 
> > > > Suppose that there is *no* instruction in L2 code which caused the software
> > > > exception, but rather L1 set arbitrary next_rip, and set EVENTINJ to software
> > > > exception with some vector, and that injection got interrupted.
> > > > 
> > > > I don't think that this code will support this.
> > > 
> > > Argh, you're right.  Maciej's selftest injects without an instruction, but it doesn't
> > > configure the scenario where that injection fails due to an exception+VM-Exit that
> > > isn't intercepted by L1 and is handled by L0.  The event_inj test gets the coverage
> > > for the latter, but always has a backing instruction.
> > 
> > Still reviewing the whole patch set, but want to clear this point quickly:
> > The selftest does have an implicit intervening NPF (handled by L0) while
> > injecting the first L1 -> L2 event.
> 
> I'll do some debug to figure out why the test passes for me.  I'm guessing I either
> got lucky, e.g. IDT was faulted in already, or I screwed up and the test doesn't
> actually pass.

Well that was easy.  My code is indeed flawed and skips the wrong instruction,
the skipped instruction just so happens to be a (spurious?) adjustment of RSP.  The
L2 guest function never runs to completion and so the "bad" RSP is never consumed.
 
   KVM: incomplete injection for L2, vector 32 @ 401c70.  next_rip = 0
   KVM: injecting for L2, vector 0 @ 401c70.  next_rip = 401c74

0000000000401c70 <l2_guest_code>:
  401c70:       48 83 ec 08             sub    $0x8,%rsp
  401c74:       83 3d 75 a7 0e 00 01    cmpl   $0x1,0xea775(%rip)        # 4ec3f0 <int_fired>
  401c7b:       74 1e                   je     401c9b <l2_guest_code+0x2b>
  401c7d:       45 31 c0                xor    %r8d,%r8d
  401c80:       b9 32 00 00 00          mov    $0x32,%ecx
  401c85:       ba 90 40 4b 00          mov    $0x4b4090,%edx
  401c8a:       31 c0                   xor    %eax,%eax
  401c8c:       be 02 00 00 00          mov    $0x2,%esi
  401c91:       bf 02 00 00 00          mov    $0x2,%edi
  401c96:       e8 05 ae 00 00          call   40caa0 <ucall>
  401c9b:       0f 01 d9                vmmcall 
  401c9e:       0f 0b                   ud2    
  401ca0:       83 3d 4d a7 0e 00 01    cmpl   $0x1,0xea74d(%rip)        # 4ec3f4 <bp_fired>
  401ca7:       74 1e                   je     401cc7 <l2_guest_code+0x57>
  401ca9:       45 31 c0                xor    %r8d,%r8d
  401cac:       b9 36 00 00 00          mov    $0x36,%ecx
  401cb1:       ba b8 40 4b 00          mov    $0x4b40b8,%edx
  401cb6:       31 c0                   xor    %eax,%eax
  401cb8:       be 02 00 00 00          mov    $0x2,%esi
  401cbd:       bf 02 00 00 00          mov    $0x2,%edi
  401cc2:       e8 d9 ad 00 00          call   40caa0 <ucall>
  401cc7:       f4                      hlt    
  401cc8:       48 83 c4 08             add    $0x8,%rsp
  401ccc:       c3                      ret    
  401ccd:       0f 1f 00                nopl   (%rax)

I don't see why the compiler is creating room for a single variable, but it doesn't
really matter, the easiest way to detect this bug is to assert that the return RIP
in the INT 0x20 handler points at l2_guest_code, e.g. this fails:

diff --git a/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
index d39be5d885c1..257aa2280b5c 100644
--- a/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
+++ b/tools/testing/selftests/kvm/x86_64/svm_nested_soft_inject_test.c
@@ -40,9 +40,13 @@ static void guest_bp_handler(struct ex_regs *regs)
 }

 static unsigned int int_fired;
+static void l2_guest_code(void);
+
 static void guest_int_handler(struct ex_regs *regs)
 {
        int_fired++;
+       GUEST_ASSERT_2(regs->rip == (unsigned long)l2_guest_code,
+                      regs->rip, (unsigned long)l2_guest_code);
 }

 static void l2_guest_code(void)
