linux-kernel - Re: [PATCH 2/3] KVM: nSVM: Rename recalc_intercepts() to clarify vmcb02 as the target

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: <zqaboykgmtuknbmlvcl3cl7b2i6pbngwqasjgdvh5w2v4ygxhg@o3e2qqxrw7ab>
Date: Wed, 4 Feb 2026 18:26:54 +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 2/3] KVM: nSVM: Rename recalc_intercepts() to clarify
 vmcb02 as the target

On Wed, Feb 04, 2026 at 09:45:52AM -0800, Sean Christopherson wrote:
> On Mon, Jan 12, 2026, Yosry Ahmed wrote:
> > recalc_intercepts() updates the intercept bits in vmcb02 based on vmcb01
> > and (cached) vmcb12.
> 
> Ah, but it does more than that.  More below.
> 
> > However, the name is too generic to make this
> > clear, and is especially confusing while searching through the code as
> > it shares the same name as the recalc_intercepts callback in
> > kvm_x86_ops.
> > 
> > Rename it to nested_vmcb02_recalc_intercepts() (similar to other
> > nested_vmcb02_* scoped functions), to make it clear what it is doing.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Yosry Ahmed <yosry.ahmed@...ux.dev>
> > ---
> >  arch/x86/kvm/svm/nested.c |  4 ++--
> >  arch/x86/kvm/svm/sev.c    |  2 +-
> >  arch/x86/kvm/svm/svm.c    |  4 ++--
> >  arch/x86/kvm/svm/svm.h    | 10 +++++-----
> >  4 files changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index 2dda52221fd8..bacb2ac4c59e 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -123,7 +123,7 @@ static bool nested_vmcb_needs_vls_intercept(struct vcpu_svm *svm)
> >  	return false;
> >  }
> >  
> > -void recalc_intercepts(struct vcpu_svm *svm)
> > +void nested_vmcb02_recalc_intercepts(struct vcpu_svm *svm)
> >  {
> >  	struct vmcb *vmcb01, *vmcb02;
> >  	unsigned int i;
> 
> Drat, I should have responded to the previous patch.  Lurking out of sight is a
> pre-existing bug that effectively invalidates this entire rename.
> 
> The existing code is:
> 
>   void recalc_intercepts(struct vcpu_svm *svm)
>   {
> 	struct vmcb *vmcb01, *vmcb02;
> 	unsigned int i;
> 
> 	vmcb_mark_dirty(svm->vmcb, VMCB_INTERCEPTS);  <======= not vmcb01!!!!!
> 
> 	if (!is_guest_mode(&svm->vcpu))
> 		return;
> 
> When L2 is active, svm->vmcb is vmcb02.  Which, at first glance, _looks_ right,
> but (the *horribly* named) recalc_intercepts() isn't _just_ recalculating
> intercepts for L2, it's also responsible for marking the VMCB_INTERCEPTS dirty
> (obviously).
> 
> But what isn't so obvious is that _all_ callers operate on vmcb01, because the
> pattern is to modify vmcb01 intercepts, and then merge the new vmcb01 intercepts
> with vmcb12, i.e. the "recalc intercepts" aspect is "part 2" of the overall
> function.

I think the 4th law of thermodynamics is that any piece of nSVM code has
a bug if you look at it long enough.

> 
> Lost in all of this is that KVM forgets to mark vmcb01 dirty, and unless there's
> a call buried somewhere deep, nested_svm_vmexit() isn't guaranteed to mark
> VMCB_INTERCEPTS dirty, e.g. if PAUSE interception is disabled.
> 
> It's probably a benign bug in practice, as AMD CPUs don't appear to do anything
> with the clean fields, but easy to fix.
> 
> As a bonus, fixing that bug yields for even better naming and code.  After the
> dust settles, we can end up with this in svm.h:
> 
>   void nested_vmcb02_recalc_intercepts(struct vcpu_svm *svm);
> 
>   static inline void svm_mark_intercepts_dirty(struct vcpu_svm *svm)
>   {
> 	vmcb_mark_dirty(svm->vmcb01.ptr, VMCB_INTERCEPTS);
> 
> 	/*
> 	 * If L2 is active, recalculate the intercepts for vmcb02 to account
> 	 * for the changes made to vmcb01.  All intercept configuration is done
> 	 * for vmcb01 and then propagated to vmcb02 to combine KVM's intercepts
> 	 * with L1's intercepts (from the vmcb12 snapshot).
> 	 */
> 	if (is_guest_mode(&svm->vcpu))
> 		nested_vmcb02_recalc_intercepts(svm);
>   }
> 
> and this for nested_vmcb02_recalc_intercepts():
> 
>   void nested_vmcb02_recalc_intercepts(struct vcpu_svm *svm)
>   {
> 	struct vmcb_ctrl_area_cached *vmcb12_ctrl = &svm->nested.ctl;
> 	struct vmcb *vmcb02 = svm->nested.vmcb02.ptr;
> 	struct vmcb *vmcb01 = svm->vmcb01.ptr;
> 	unsigned int i;
> 
> 	if (WARN_ON_ONCE(svm->vmcb != vmcb02))
> 		return;
> 
> 	...
>   }
> 
> with the only other caller of nested_vmcb02_recalc_intercepts() being
> nested_vmcb02_prepare_control().

I think this looks good. Definitely an improvement over what we
currently have :)