linux-kernel - Re: [PATCH v2 2/2] KVM: SVM: Don't set GIF when clearing EFER.SVME

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] [day] [month] [year] [list]

Message-ID: <aPK7qvIeSdzxdzMZ@google.com>
Date: Fri, 17 Oct 2025 14:56:58 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Jim Mattson <jmattson@...gle.com>
Cc: Yosry Ahmed <yosry.ahmed@...ux.dev>, Paolo Bonzini <pbonzini@...hat.com>, 
	Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, 
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H. Peter Anvin" <hpa@...or.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] KVM: SVM: Don't set GIF when clearing EFER.SVME

On Tue, Oct 14, 2025, Jim Mattson wrote:
> On Tue, Oct 14, 2025 at 3:07 PM Sean Christopherson <seanjc@...gle.com> wrote:
> >
> > On Tue, Oct 14, 2025, Jim Mattson wrote:
> > > On Tue, Oct 14, 2025 at 2:18 PM Sean Christopherson <seanjc@...gle.com> wrote:
> > > >
> > > > On Tue, Oct 14, 2025, Jim Mattson wrote:
> > > > > On Mon, Oct 13, 2025 at 3:33 PM Sean Christopherson <seanjc@...gle.com> wrote:
> > > > > >
> > > > > > On Thu, Oct 09, 2025, Jim Mattson wrote:
> > > > > > > Clearing EFER.SVME is not architected to set GIF.
> > > > > >
> > > > > > But it's also not architected to leave GIF set when the guest is running, which
> > > > > > was the basic gist of the Fixes commit.  I suspect that forcing GIF=1 was
> > > > > > intentional, e.g. so that the guest doesn't end up with GIF=0 after stuffing the
> > > > > > vCPU into SMM mode, which might actually be invalid.
> > > > > >
> > > > > > I think what we actually want is to to set GIF when force-leaving nested.  The
> > > > > > only path where it's not obvious that's "safe" is toggling SMM in
> > > > > > kvm_vcpu_ioctl_x86_set_vcpu_events().  In every other path, setting GIF is either
> > > > > > correct/desirable, or irrelevant because the caller immediately and unconditionally
> > > > > > sets/clears GIF.
> > > > > >
> > > > > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > > > > > index a6443feab252..3392c7e22cae 100644
> > > > > > --- a/arch/x86/kvm/svm/nested.c
> > > > > > +++ b/arch/x86/kvm/svm/nested.c
> > > > > > @@ -1367,6 +1367,8 @@ void svm_leave_nested(struct kvm_vcpu *vcpu)
> > > > > >                 nested_svm_uninit_mmu_context(vcpu);
> > > > > >                 vmcb_mark_all_dirty(svm->vmcb);
> > > > > >
> > > > > > +               svm_set_gif(svm, true);
> > > > > > +
> > > > > >                 if (kvm_apicv_activated(vcpu->kvm))
> > > > > >                         kvm_make_request(KVM_REQ_APICV_UPDATE, vcpu);
> > > > > >         }
> > > > > >
> > > > >
> > > > > This seems dangerously close to KVM making up "hardware" behavior, but
> > > > > I'm okay with that if you are.
> > > >
> > > > Regardless of what KVM does, we're defining hardware behavior, i.e. keeping GIF
> > > > unchanged defines behavior just as much as setting GIF.  The only way to truly
> > > > avoid defining behavior would be to terminate the VM and completely prevent
> > > > userspace from accessing its state.
> > >
> > > This can't be the only instance of "undefined behavior" that KVM deals
> > > with.
> >
> > Oh, for sure.  But unsurprisingly, people only care about cases that actually
> > matter in practice.  E.g. the other one that comes to mind is SHUTDOWN on AMD:
> >
> >         /*
> >          * VMCB is undefined after a SHUTDOWN intercept.  INIT the vCPU to put
> >          * the VMCB in a known good state.  Unfortuately, KVM doesn't have
> >          * KVM_MP_STATE_SHUTDOWN and can't add it without potentially breaking
> >          * userspace.  At a platform view, INIT is acceptable behavior as
> >          * there exist bare metal platforms that automatically INIT the CPU
> >          * in response to shutdown.
> >          *
> 
> The behavior of SHUTDOWN while GIF==0 is clearly architected:
> 
> "If the processor enters the shutdown state (due to a triple fault for
> instance) while GIF is clear, it can only be restarted by means of a
> RESET."
> 
> Doesn't setting GIF in svm_leave_nested() violate this specification?

Probably?  But SHUTDOWN also makes the VMCB undefined, so KVM is caught between
a rock and a hard place.  And when using vGIF, I don't see how KVM can do the
right thing, because the state of GIF at the time of SHUTDOWN is unknown.

And FWIW, if userspace does RESET the guest (which KVM can't detect with 100%
accuracy), GIF=1 on RESET, so it's kinda sorta right :-)