linux-kernel - Re: [PATCH V2 1/5] KVM: SVM: Move STGI and CLGI intercept handling

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: <xndoethnkd2djh5zkemvgmuj6gc4hsnxur2uo5frl57ugxa2ql@c3k7cadxmr4u>
Date: Thu, 15 Jan 2026 16:41:09 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Kevin Cheng <chengkev@...gle.com>, pbonzini@...hat.com, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V2 1/5] KVM: SVM: Move STGI and CLGI intercept handling

On Wed, Jan 14, 2026 at 05:39:13PM -0800, Sean Christopherson wrote:
> On Mon, Jan 12, 2026, Yosry Ahmed wrote:
> > On Mon, Jan 12, 2026 at 05:45:31PM +0000, Kevin Cheng wrote:
> > > Similar to VMLOAD/VMSAVE intercept handling, move the STGI/CLGI
> > > intercept handling to svm_recalc_instruction_intercepts().
> > > ---
> > >  arch/x86/kvm/svm/svm.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > > index 24d59ccfa40d9..6373a25d85479 100644
> > > --- a/arch/x86/kvm/svm/svm.c
> > > +++ b/arch/x86/kvm/svm/svm.c
> > > @@ -1010,6 +1010,11 @@ static void svm_recalc_instruction_intercepts(struct kvm_vcpu *vcpu)
> > >  			svm_clr_intercept(svm, INTERCEPT_VMSAVE);
> > >  			svm->vmcb->control.virt_ext |= VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK;
> > >  		}
> > > +
> > > +		if (vgif) {
> > > +			svm_clr_intercept(svm, INTERCEPT_STGI);
> > 
> > Could this cause a problem with NMI window tracking?
> 
> Yes.
> 
> > svm_enable_nmi_window() sets INTERCEPT_STGI to detect when NMIs are
> > enabled, and it's later cleared by svm_set_gif(). If we recalc
> > intercepts in between we will clear the intercept here and miss NMI
> > enablement.
> > 
> > We could move the logic to set/clear INTERCEPT_STGI for NMI window
> > tracking here as well, but then we'll need to recalc intercepts in
> > svm_enable_nmi_window() and svm_set_gif(), which could be expensive.
> > 
> > The alternative is perhaps setting a flag when INTERCEPT_STGI is set in
> > svm_enable_nmi_window() and avoid clearing the intercept here if the
> > flag is set.
> > 
> > Not sure what's the best way forward here.
> 
> First things first, the changelog needs to state _why_ the code is being moved.
> "To be like VMLOAD/VMSAVE" doesn't suffice, because my initial answer was going
> to be "well just don't move the code" (I already forgot the context of v1).
> 
> But moving the code is needed to fix the missing #UD in "Recalc instructions
> intercepts when EFER.SVME is toggled".
> 
> As for how to fix this, a few ideas:
> 
>  1. Set KVM_REQ_EVENT to force KVM to re-evulate all events.  kvm_check_and_inject_events()
>     will see the pending NMI and/or SMI, that the NMI/SMI is not allowed, and
>     re-call enable_{nmi,smi}_window().
> 
>  2. Manually check for pending+blocked NMI/SMIs.
> 
>  3. Combine parts of #1 and #2.  Set KVM_REQ_EVENT, but only if there's a pending
>     NMI or SMI.
> 
>  4. Add flags to vcpu_svm to explicitly track if a vCPU has an NMI/SMI window,
>     similar to what we're planning on doing for IRQs[*], and use that to more
>     confidently do the right thing when recomputing intercepts.
> 
> I don't love any of those ideas.  Ah, at least not until I poke around KVM.  In
> svm_set_gif() there's already this:
> 
> 		if (svm->vcpu.arch.smi_pending ||
> 		    svm->vcpu.arch.nmi_pending ||
> 		    kvm_cpu_has_injectable_intr(&svm->vcpu) ||
> 		    kvm_apic_has_pending_init_or_sipi(&svm->vcpu))
> 			kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
> 
> So I think it makes sense to bundle that into a helper, e.g. (no idea what to
> call it)
> 
> static bool svm_think_of_a_good_name(struct kvm_vcpu *vcpu)
> {
> 	if (svm->vcpu.arch.smi_pending ||
> 	    svm->vcpu.arch.nmi_pending ||
> 	    kvm_cpu_has_injectable_intr(&svm->vcpu) ||
> 	    kvm_apic_has_pending_init_or_sipi(&svm->vcpu))
> 		kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);
> }

Maybe svm_check_gif_events() or svm_check_gif_interrupts()?

Or maybe it's clearer if we just put the checks in a helper like
svm_waiting_for_gif() or svm_pending_gif_interrupt().


Then in svm_recalc_instruction_intercepts() we do:

	/*
	 * If there is a pending interrupt controlled by GIF, set
	 * KVM_REQ_EVENT to re-evaluate if the intercept needs to be set
	 * again to track when GIF is re-enabled (e.g. for NMI
	 * injection).
	 */
	svm_clr_intercept(svm, INTERCEPT_STGI);	
	if (svm_pending_gif_interrupt())
		kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);

and in svm_set_gif() it reads well semantically:
	
	enable_gif(svm);
	if (svm_pending_gif_interrupt())
		kvm_make_request(KVM_REQ_EVENT, &svm->vcpu);

> 
> And then call that from svm_recalc_instruction_intercepts().  That implements #3
> in a fairly maintainable way (we'll hopefully notice sooner than later if we break
> svm_set_gif()).
> 
> https://lore.kernel.org/all/26732815475bf1c5ba672bc3b1785265f1a994e6.1752819570.git.naveen@kernel.org
>