linux-kernel - Re: [Question] int3 instruction generates a #UD in SEV VM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZMpwiSw9CBZh9xcc@google.com>
Date:   Wed, 2 Aug 2023 08:04:41 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Tom Lendacky <thomas.lendacky@....com>
Cc:     Wu Zongyo <wuzongyo@...l.ustc.edu.cn>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org, x86@...nel.org,
        linux-coco@...ts.linux.dev
Subject: Re: [Question] int3 instruction generates a #UD in SEV VM

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.
> > > > 
> > > > The path I found is:
> > > >      svm_vcpu_run
> > > >          svm_complete_interrupts
> > > >         kvm_requeue_exception // vector = 3
> > > >             kvm_make_request
> > > > 
> > > >      vcpu_enter_guest
> > > >          kvm_check_and_inject_events
> > > >         svm_inject_exception
> > > >             svm_update_soft_interrupt_rip
> > > >             __svm_skip_emulated_instruction
> > > >                 x86_emulate_instruction
> > > >                 svm_can_emulate_instruction
> > > >                     kvm_queue_exception(vcpu, UD_VECTOR)
> > > > 
> > > > Does this mean a #PF intercept occur when the guest try to deliver a
> > > > #BP through the IDT? But why?
> > > 
> > > I doubt it's a #PF.  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.
> > > 
> > > What is the last exit recorded by trace_kvm_exit() before the #UD is
> > > injected?
> > 
> > I'm guessing it was a #NPF, too. Could it be related to the changes that
> > went in around svm_update_soft_interrupt_rip()?
> > 
> > 6ef88d6e36c2 ("KVM: SVM: Re-inject INT3/INTO instead of retrying the
> > instruction")
> 
> Sorry, that should have been:
> 
> 7e5b5ef8dca3 ("KVM: SVM: Re-inject INTn instead of retrying the insn on "failure"")
> 
> > 
> > Before this the !nrips check would prevent the call into
> > svm_skip_emulated_instruction(). But now, there is a call to:
> > 
> >    svm_update_soft_interrupt_rip()
> >      __svm_skip_emulated_instruction()
> >        kvm_emulate_instruction()
> >          x86_emulate_instruction() (passed a NULL insn pointer)
> >            kvm_can_emulate_insn() (passed a NULL insn pointer)
> >              svm_can_emulate_instruction() (passed NULL insn pointer)
> > 
> > Because it is an SEV guest, it ends up in the "if (unlikely(!insn))" path
> > 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_vcpu *vcpu,
        }
 
        if (!svm->next_rip) {
+               if (sev_guest(vcpu->kvm))
+                       return 0;
+
                if (unlikely(!commit_side_effects))
                        old_rflags = svm->vmcb->save.rflags;
 
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 SEV 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)
 
        svm_adjust_mmio_mask();
 
+       nrips = nrips && boot_cpu_has(X86_FEATURE_NRIPS);
+
        /*
         * Note, SEV setup consumes npt_enabled and enable_mmio_caching (which
-        * may be modified by svm_adjust_mmio_mask()).
+        * may be modified by svm_adjust_mmio_mask()), as well as nrips.
         */
        sev_hardware_setup();
 
@@ -5125,11 +5127,6 @@ static __init int svm_hardware_setup(void)
                        goto err;
        }
 
-       if (nrips) {
-               if (!boot_cpu_has(X86_FEATURE_NRIPS))
-                       nrips = false;
-       }
-
        enable_apicv = avic = avic && avic_hardware_setup();
 
        if (!enable_apicv) {