linux-kernel - Re: [PATCH 1/3] KVM: nSVM: Use intuitive local variables in recalc

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: <gmdou4cp47vpx72tw3mwklwixpd3ujcdcomoplosv2u2tzfub2@wtqgzkhguoap>
Date: Wed, 4 Feb 2026 17:43:00 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Kevin Cheng <chengkev@...gle.com>, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] KVM: nSVM: Use intuitive local variables in
 recalc_intercepts()

On Wed, Feb 04, 2026 at 09:29:36AM -0800, Sean Christopherson wrote:
> On Mon, Jan 12, 2026, Yosry Ahmed wrote:
> > recalc_intercepts() currently uses c, h, g as local variables for the
> > control area of the current VMCB, vmcb01, and (cached) vmcb12.
> > 
> > The current VMCB should always be vmcb02 when recalc_intercepts() is
> > executed in guest mode. Use vmcb01/vmcb02 local variables instead to
> > make it clear the function is updating intercepts in vmcb02 based on the
> > intercepts in vmcb01 and (cached) vmcb12.
> > 
> > Add a WARNING() if the current VMCB is not in fact vmcb02.
> 
> This belongs in a separate patch.
> 
> > No functional change intended.
> > 
> > Signed-off-by: Yosry Ahmed <yosry.ahmed@...ux.dev>
> > ---
> >  arch/x86/kvm/svm/nested.c | 31 +++++++++++++++----------------
> >  1 file changed, 15 insertions(+), 16 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index f295a41ec659..2dda52221fd8 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -125,8 +125,7 @@ static bool nested_vmcb_needs_vls_intercept(struct vcpu_svm *svm)
> >  
> >  void recalc_intercepts(struct vcpu_svm *svm)
> >  {
> > -	struct vmcb_control_area *c, *h;
> > -	struct vmcb_ctrl_area_cached *g;
> > +	struct vmcb *vmcb01, *vmcb02;
> >  	unsigned int i;
> >  
> >  	vmcb_mark_dirty(svm->vmcb, VMCB_INTERCEPTS);
> > @@ -134,14 +133,14 @@ void recalc_intercepts(struct vcpu_svm *svm)
> >  	if (!is_guest_mode(&svm->vcpu))
> >  		return;
> >  
> > -	c = &svm->vmcb->control;
> > -	h = &svm->vmcb01.ptr->control;
> > -	g = &svm->nested.ctl;
> > +	vmcb01 = svm->vmcb01.ptr;
> > +	vmcb02 = svm->nested.vmcb02.ptr;
> > +	WARN_ON_ONCE(svm->vmcb != vmcb02);
> 
> If we're going to bother with a WARN, then this code should definitely bail,
> because configuring vmcb01 using the nested logic is all but guaranteed to break
> L1 in weird ways.

I can put the WARN + bail in a separate patch.

> 
> >  	for (i = 0; i < MAX_INTERCEPT; i++)
> > -		c->intercepts[i] = h->intercepts[i];
> > +		vmcb02->control.intercepts[i] = vmcb01->control.intercepts[i];
> >  
> > -	if (g->int_ctl & V_INTR_MASKING_MASK) {
> > +	if (svm->nested.ctl.int_ctl & V_INTR_MASKING_MASK) {
> 
> I vote to keep a pointer to the cached control as vmcb12_ctrl.  Coming from a
> nVMX-focused background, I can never remember what svm->nested.ctl holds.  For
> me, this is waaaay more intuivite:

I agree it reads better, but honestly all of nSVM code uses
svm->nested.ctl, and changing its name here just makes things
inconsistent imo.

> 
> 	if (vmcb12_ctrl->int_ctl & V_INTR_MASKING_MASK) {
> 
> >  	for (i = 0; i < MAX_INTERCEPT; i++)
> > -		c->intercepts[i] |= g->intercepts[i];
> > +		vmcb02->control.intercepts[i] |= svm->nested.ctl.intercepts[i];
> 
> And even more so here:
> 
> 	for (i = 0; i < MAX_INTERCEPT; i++)
> 		vmcb02->control.intercepts[i] |= vmcb12_ctrl->intercepts[i];
> 
> >  
> >  	/* If SMI is not intercepted, ignore guest SMI intercept as well  */
> >  	if (!intercept_smi)
> > -		vmcb_clr_intercept(c, INTERCEPT_SMI);
> > +		vmcb_clr_intercept(&vmcb02->control, INTERCEPT_SMI);
> >  
> >  	if (nested_vmcb_needs_vls_intercept(svm)) {
> >  		/*
> > @@ -177,10 +176,10 @@ void recalc_intercepts(struct vcpu_svm *svm)
> >  		 * we must intercept these instructions to correctly
> >  		 * emulate them in case L1 doesn't intercept them.
> >  		 */
> > -		vmcb_set_intercept(c, INTERCEPT_VMLOAD);
> > -		vmcb_set_intercept(c, INTERCEPT_VMSAVE);
> > +		vmcb_set_intercept(&vmcb02->control, INTERCEPT_VMLOAD);
> > +		vmcb_set_intercept(&vmcb02->control, INTERCEPT_VMSAVE);
> >  	} else {
> > -		WARN_ON(!(c->virt_ext & VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK));
> > +		WARN_ON(!(vmcb02->control.virt_ext & VIRTUAL_VMLOAD_VMSAVE_ENABLE_MASK));
> 
> Opportunistically switch this to WARN_ON_ONCE.  Any "unguarded" WARN in KVM
> (outside of e.g. __init code) is just asking for a self-DoS.

Will do.

> 
> >  	}
> >  }
> >  
> > -- 
> > 2.52.0.457.g6b5491de43-goog
> >