mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH] KVM: nSVM: Reset the virtual VMLOAD/VMSAVE bit in vmcb02 on every nested VMRUN
@ 2026-10-16 23:52 Amirmohammad Eftekhar
  2026-10-22 17:21 ` Sean Christopherson
  0 siblings, 1 reply; 4+ messages in thread
From: Amirmohammad Eftekhar @ 2026-10-16 23:52 UTC (permalink / raw)
  To: seanjc, pbonzini; +Cc: kvm, linux-kernel

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.

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.
+	 */
+	vmcb02->control.misc_ctl2 &= ~SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE;
+
 	if (!nested_vmcb_needs_vls_intercept(svm))
 		vmcb02->control.misc_ctl2 |= SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE;
 
-- 
2.54.0 (Apple Git-157)


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: nSVM: Reset the virtual VMLOAD/VMSAVE bit in vmcb02 on every nested VMRUN
  2026-10-16 23:52 [PATCH] KVM: nSVM: Reset the virtual VMLOAD/VMSAVE bit in vmcb02 on every nested VMRUN Amirmohammad Eftekhar
@ 2026-10-22 17:21 ` Sean Christopherson
  2026-10-22 23:45   ` Yosry Ahmed
  0 siblings, 1 reply; 4+ messages in thread
From: Sean Christopherson @ 2026-10-22 17:21 UTC (permalink / raw)
  To: Amirmohammad Eftekhar
  Cc: pbonzini, kvm, linux-kernel, Yosry Ahmed, Jim Mattson

+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.
> 
> 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;

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);

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?

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: nSVM: Reset the virtual VMLOAD/VMSAVE bit in vmcb02 on every nested VMRUN
  2026-10-22 17:21 ` Sean Christopherson
@ 2026-10-22 23:45   ` Yosry Ahmed
  2026-10-23  9:09     ` Amirmohammad Eftekhar
  0 siblings, 1 reply; 4+ messages in thread
From: Yosry Ahmed @ 2026-10-22 23:45 UTC (permalink / raw)
  To: Sean Christopherson
  Cc: Amirmohammad Eftekhar, pbonzini, kvm, linux-kernel, Jim Mattson

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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] KVM: nSVM: Reset the virtual VMLOAD/VMSAVE bit in vmcb02 on every nested VMRUN
  2026-10-22 23:45   ` Yosry Ahmed
@ 2026-10-23  9:09     ` Amirmohammad Eftekhar
  0 siblings, 0 replies; 4+ messages in thread
From: Amirmohammad Eftekhar @ 2026-10-23  9:09 UTC (permalink / raw)
  To: yosry, seanjc
  Cc: pbonzini, kvm, linux-kernel, jmattson, amirmohammad.eftekhar

On Wed, Oct 22, 2026, Yosry Ahmed wrote:
> So this test case passing should be confirming my allegation that
> intercepts take precedence over VLS.

I could be missing something, but a hardware repro points the other way,
at least on this part (Zen 4, Ryzen 5 PRO 8500GE, mainline 7.3-rc3). The
L2 runs "vmload; cpuid", so the exit code says whether VMLOAD was
intercepted (SVM_EXIT_VMLOAD, 0x82) or virtualized and run on to the
cpuid (SVM_EXIT_CPUID, 0x72):

  - Single VMRUN, vmcb12 VLS off, L1 intercepting VMLOAD: 0x82. The
    intercept is honored, as expected.
  - VMRUN #1 enables VLS legitimately (0x72), then VMRUN #2 with vmcb12
    VLS=0 and L1 intercepting VMLOAD: still 0x72. 3/3.

The difference from nested_vmsave_vmload_test.c is that the test only
looks at a single VMRUN. On VMRUN #2 vmcb12 has VLS off, so
nested_vmcb_needs_vls_intercept() returns true and
nested_vmcb02_recalc_intercepts() sets INTERCEPT_VMLOAD/VMSAVE in vmcb02.
But vmcb02->control.misc_ctl2 still has the VLS enable bit from VMRUN #1,
since it is only ever OR-set and never cleared, so vmcb02 enters VMRUN #2
with both INTERCEPT_VMLOAD and VLS set and the CPU virtualizes VMLOAD
anyway. That is the dropped intercept.

So with both bits live in the same vmcb02, VLS wins over the intercept on
this hardware, which is why I think the stale bit is an actual
intercept-drop (an L1 that intercepts VMLOAD/VMSAVE for its own L2 loses
that intercept after it has used VLS even once) rather than only untidy
state.

I haven't managed to square that with your reading of the test, so it is
entirely possible I am wrong about the precedence. If it helps I can send
the reproducer, or dump vmcb02->control (the intercept bitmap plus
misc_ctl2) right before the VMRUN #2 that virtualizes VMLOAD, so we can
confirm both bits are set going in.

Thanks,
Amirmohammad

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-10-23  9:09 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-16 23:52 [PATCH] KVM: nSVM: Reset the virtual VMLOAD/VMSAVE bit in vmcb02 on every nested VMRUN Amirmohammad Eftekhar
2026-10-22 17:21 ` Sean Christopherson
2026-10-22 23:45   ` Yosry Ahmed
2026-10-23  9:09     ` Amirmohammad Eftekhar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®