Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 2 Aug 2023 08:04:41 -0700
In-Reply-To: <7a4f3f59-1482-49c4-92b2-aa621e9b06b3@amd.com>
Mime-Version: 1.0
References: <8eb933fd-2cf3-d7a9-32fe-2a1d82eac42a@mail.ustc.edu.cn>
 <ZMfFaF2M6Vrh/QdW@google.com> <4ebb3e20-a043-8ad3-ef6c-f64c2443412c@amd.com>
 <544b7f95-4b34-654d-a57b-3791a6f4fd5f@mail.ustc.edu.cn> <ZMpEUVsv5hSmrcH8@iZuf6hx7901barev1c282cZ>
 <ZMphvF+0H9wHQr5B@google.com> <bbc52f40-2661-3fa2-8e09-bec772728812@amd.com> <7a4f3f59-1482-49c4-92b2-aa621e9b06b3@amd.com>
Message-ID: <ZMpwiSw9CBZh9xcc@google.com>
Subject: Re: [Question] int3 instruction generates a #UD in SEV VM
From:   Sean Christopherson <seanjc@google.com>
To:     Tom Lendacky <thomas.lendacky@amd.com>
Cc:     Wu Zongyo <wuzongyo@mail.ustc.edu.cn>,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org, x86@kernel.org,
        linux-coco@lists.linux.dev
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Aug 02, 2023, Tom Lendacky wrote:
> On 8/2/23 09:25, Tom Lendacky wrote:
> > On 8/2/23 09:01, Sean Christopherson wrote:
> > > > You're right. The #UD is injected by KVM.
> > > >=20
> > > > The path I found is:
> > > > =C2=A0=C2=A0=C2=A0=C2=A0 svm_vcpu_run
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 svm_complete_inter=
rupts
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 kvm_requeue_exception //=
 vector =3D 3
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
kvm_make_request
> > > >=20
> > > > =C2=A0=C2=A0=C2=A0=C2=A0 vcpu_enter_guest
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 kvm_check_and_inje=
ct_events
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 svm_inject_exception
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
svm_update_soft_interrupt_rip
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 =
__svm_skip_emulated_instruction
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 x86_emulate_instruction
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0 svm_can_emulate_instruction
> > > > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 kvm_queue_exception(vcpu, =
UD_VECTOR)
> > > >=20
> > > > Does this mean a #PF intercept occur when the guest try to deliver =
a
> > > > #BP through the IDT? But why?
> > >=20
> > > I doubt it's a #PF.=C2=A0 A #NPF is much more likely, though it could=
 be
> > > something
> > > else entirely, but I'm pretty sure that would require bugs in both
> > > the host and
> > > guest.
> > >=20
> > > What is the last exit recorded by trace_kvm_exit() before the #UD is
> > > injected?
> >=20
> > I'm guessing it was a #NPF, too. Could it be related to the changes tha=
t
> > went in around svm_update_soft_interrupt_rip()?
> >=20
> > 6ef88d6e36c2 ("KVM: SVM: Re-inject INT3/INTO instead of retrying the
> > instruction")
>=20
> Sorry, that should have been:
>=20
> 7e5b5ef8dca3 ("KVM: SVM: Re-inject INTn instead of retrying the insn on "=
failure"")
>=20
> >=20
> > Before this the !nrips check would prevent the call into
> > svm_skip_emulated_instruction(). But now, there is a call to:
> >=20
> >  =C2=A0 svm_update_soft_interrupt_rip()
> >  =C2=A0=C2=A0=C2=A0 __svm_skip_emulated_instruction()
> >  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 kvm_emulate_instruction()
> >  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 x86_emulate_instruction() (=
passed a NULL insn pointer)
> >  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 kvm_can_emulate=
_insn() (passed a NULL insn pointer)
> >  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 svm=
_can_emulate_instruction() (passed NULL insn pointer)
> >=20
> > Because it is an SEV guest, it ends up in the "if (unlikely(!insn))" pa=
th
> > and injects the #UD.

Yeah, my money is on that too.  I believe this is the least awful solution:

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index d381ad424554..2eace114a934 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -385,6 +385,9 @@ static int __svm_skip_emulated_instruction(struct kvm_v=
cpu *vcpu,
        }
=20
        if (!svm->next_rip) {
+               if (sev_guest(vcpu->kvm))
+                       return 0;
+
                if (unlikely(!commit_side_effects))
                        old_rflags =3D svm->vmcb->save.rflags;
=20
I'll send a formal patch (with a comment) if that solves the problem.


Side topic, KVM should require nrips for SEV and beyond, I don't see how SE=
V can
possibly work if KVM doesn't utilize nrips.  E.g. this

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 2eace114a934..43e500503d48 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -5111,9 +5111,11 @@ static __init int svm_hardware_setup(void)
=20
        svm_adjust_mmio_mask();
=20
+       nrips =3D nrips && boot_cpu_has(X86_FEATURE_NRIPS);
+
        /*
         * Note, SEV setup consumes npt_enabled and enable_mmio_caching (wh=
ich
-        * may be modified by svm_adjust_mmio_mask()).
+        * may be modified by svm_adjust_mmio_mask()), as well as nrips.
         */
        sev_hardware_setup();
=20
@@ -5125,11 +5127,6 @@ static __init int svm_hardware_setup(void)
                        goto err;
        }
=20
-       if (nrips) {
-               if (!boot_cpu_has(X86_FEATURE_NRIPS))
-                       nrips =3D false;
-       }
-
        enable_apicv =3D avic =3D avic && avic_hardware_setup();
=20
        if (!enable_apicv) {