Re: [PATCH] KVM: nSVM: Reset the virtual VMLOAD/VMSAVE bit in vmcb02 on every nested VMRUN

[Yosry Ahmed <yosry@kernel.org>]

On Thu, Oct 22, 2026 at 10:21:28AM -0700, Sean Christopherson wrote:
> +Yosry and Jim
> 
> On Sat, Oct 17, 2026, Amirmohammad Eftekhar wrote:
> > The SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE (VLS) bit in
> > vmcb02->control.misc_ctl2 is OR-set in nested_vmcb02_prepare_control()
> > when L2 is allowed to use virtual VMLOAD/VMSAVE, but it is never reset.
> > vmcb02 is a persistent per-vCPU structure reused across nested VMRUNs,
> > so once one nested entry enables VLS the bit leaks into every later
> > entry -- including entries where vmcb12 has VLS cleared and L1 has asked
> > KVM to intercept VMLOAD/VMSAVE.  Per AMD's APM, virtual VMLOAD/VMSAVE is
> > honored over the VMLOAD/VMSAVE intercepts, so with the stale bit set
> > L2's VMLOAD/VMSAVE are virtualized and run without a #VMEXIT, and the
> > intercept L1 explicitly requested is silently dropped.

Does the APM say this? My version doesn't (Revision 4.09, March 2026):

	15.33.1 VMSAVE and VMLOAD Virtualization

	This feature allows the VMSAVE and VMLOAD instructions to
	execute in guest mode without causing a #VMEXIT. The VMCB
	address in RAX is treated as a guest physical address and is
	translated to a host physical address. Any page fault in
	attempting that translation will result in a normal #VMEXIT with
	a nested page fault exit code. If the translation is successful,
	the register state transfer to or from the VMCB will then be
	performed.

	Support for virtualized VMSAVE and VMLOAD is indicated by CPUID
	Fn8000_000A_EDX[15]=1.  When this feature is available, it must
	be explicitly enabled by setting bit 1 of VMCB offset 0B8h to 1.
	This enable bit is only recognized when the hypervisor is in 64
	bit mode, nested paging is enabled and Secure Encrypted
	Virtualization is disabled, otherwise attempted execution of a
	VMLOAD or VMSAVE in the guest will result in a #VMEXIT with a
	VMSAVE/VMLOAD exit code.

In fact, this is opposite to the conclusion I reached in 55780d8a1dcc9
("KVM: SVM: Stop toggling virtual VMSAVE/VMLOAD on intercept recalc"),
and I like to think that I tested on HW back then (tho I honestly don't
remember).

Actually, looking at nested_vmsave_vmload_test.c, I make sure that the
intercepts are honored (in L1) *even* if VLS is enabled. I also *think*
that in that test, KVM will end up setting both the intercepts and VLS:
1. nested_vmcb_needs_vls_intercept() will return false because the guest
   supports VLS, has NPT, and enables VLS.
2. KVM still copies the intercepts from vmcb12 anyway, including VMSAVE
   and VMLOAD, see nested_vmcb02_recalc_intercepts(). KVM (correctly)
   doesn't clear VMSAVE/VMLOAD intercepts if VLS is enabled by L1 (but
   it does the opposite).

So this test case passing should be confirming my allegation that
intercepts take precedence over VLS.

I think the test also goes on to clear VLS and test the behavior with it
cleared, which should be further evidence that KVM leaving the bit set
is harmless, because KVM will intercept VMSAVE/VMLOAD if L1 clears VLS,
even tho it will leave VLS set in vmcb12.

> > 
> > misc_ctl is reset from vmcb01 a few lines earlier; misc_ctl2's VLS bit
> > has no equivalent reset.  The only writes to misc_ctl2 are this OR-set,
> > the vmcb01 init OR-set, and the independent V_LBR (bit 0) set/clear, so
> > nothing clears bit 1 between entries.  nested_vmcb02_recalc_intercepts()
> > sets INTERCEPT_VMLOAD/VMSAVE when nested_vmcb_needs_vls_intercept() is
> > true and otherwise only WARNs on the reverse condition; it never catches
> > the real hazard of VLS on while the intercept is also set.
> > 
> > Reset the VLS bit before the conditional set, mirroring misc_ctl, and
> > add a forward WARN so a future regression trips immediately.
> > 
> > This is distinct from CVE-2021-3656 (single-VMRUN under-sanitization of
> > a VLS bit L1 set in vmcb12 without being granted the feature); here L1
> > is granted VLS and uses it legitimately, and the bit goes stale across
> > VMRUNs.
> > 
> > Signed-off-by: Amirmohammad Eftekhar <amirmohammad.eftekhar@cispa.de>
> > ---
> >  arch/x86/kvm/svm/nested.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> > index af0ef6784..65e61b796 100644
> > --- a/arch/x86/kvm/svm/nested.c
> > +++ b/arch/x86/kvm/svm/nested.c
> > @@ -197,6 +197,14 @@ void nested_vmcb02_recalc_intercepts(struct vcpu_svm *svm)
> >  	} else {
> >  		WARN_ON_ONCE(!(vmcb02->control.misc_ctl2 & SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE));
> >  	}
> > +
> > +	/*
> > +	 * If KVM is intercepting VMLOAD/VMSAVE, virtual VMLOAD/VMSAVE must be
> > +	 * off, or hardware would honor the virtualization and render the
> > +	 * intercept inert.
> > +	 */
> > +	WARN_ON_ONCE(vmcb_is_intercept(&vmcb02->control, INTERCEPT_VMLOAD) &&
> > +		     (vmcb02->control.misc_ctl2 & SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE));
> >  }
> >  
> >  /*
> > @@ -994,6 +1002,13 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
> >  
> >  	/* SVM_MISC2_ENABLE_V_LBR is controlled by svm_update_lbrv() */
> >  
> > +	/*
> > +	 * vmcb02 is reused across nested VMRUNs and the VLS bit below is only
> > +	 * OR-set, so clear it first; a stale bit would let L2 run VMLOAD/VMSAVE
> > +	 * unintercepted even on a VMRUN that asks KVM to intercept them.
> > +	 */
> 
> This becomes self-explanatory if written as:
> 
> 	if (nested_vmcb_needs_vls_intercept(svm))
> 		vmcb02->control.misc_ctl2 &= ~SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE;
> 	else
> 		vmcb02->control.misc_ctl2 |= SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE;

All that being said, I am not opposed to doing this in terms of hygiene.

> 
> and I definitely think we should un-invert the check to match nested_vmcb02_recalc_intercepts().
> 
> That said, not explicitly initializing all of misc_ctl2 is crazy.  AFAICT, LBR
> virtualization is the only other bit in misc_ctl2, and it to is always computed
> dynamically so I'd rather do this:
> 
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 1503fee15baa..6ea35a3dc361 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -884,8 +884,6 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
>  	}
>  
>  	/*
> -	 * Copied from vmcb01.  msrpm_base can be overwritten later.
> -	 *
>  	 * SVM_MISC_ENABLE_NP in vmcb12 is only used for consistency checks.  If
>  	 * L1 enables NPTs, KVM shadows L1's NPTs and uses those to run L2. If
>  	 * L1 disables NPT, KVM runs L2 with the same NPTs used to run L1. For
> @@ -893,6 +891,7 @@ static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
>  	 * to L1 GPAs, so the same NPTs can be used for L1 and L2.
>  	 */
>  	vmcb02->control.misc_ctl = vmcb01->control.misc_ctl & (SVM_MISC_ENABLE_NP | SVM_MISC_ENABLE_GMET);
> +	vmcb02->control.misc_ctl2 = 0;
>  	vmcb02->control.iopm_base_pa = vmcb01->control.iopm_base_pa;
>  	vmcb02->control.msrpm_base_pa = vmcb01->control.msrpm_base_pa;
>  	vmcb_mark_dirty(vmcb02, VMCB_PERM_MAP);

Also not opposed to this.

> 
> Argh!  But nested_vmcb02_prepare_save() isn't called in the svm_set_nested_state()
> case, so AFAICT svm_update_lbrv() is never called.  Though that seems like a
> pre-existing?

Yeah this seems like a pre-existing issue. I suppose we can move
svm_update_lbrv() outside of nested_vmcb02_prepare_control():

1. In enter_svm_guest_mode(), call it after
nested_vmcb02_prepare_save().
2. In svm_set_nested_state(), call it after
nested_vmcb02_prepare_control(), as svm_copy_vmrun_state() should have
already updated the LBR and dbgctl regs.