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

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

[Amirmohammad Eftekhar]

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)

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] KVM: nSVM: guest-triggerable WARN_ON_ONCE in nested_vmcb02_recalc_intercepts()
--

commit f74a55775787865d4cf964f0af9f8bea8ecd1acb
Author: Amirmohammad Eftekhar <amirmohammad.eftekhar@cispa.de>

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

This commit clears SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE in
vmcb02->control.misc_ctl2 before conditionally setting it in
nested_vmcb02_prepare_control(), and adds a WARN_ON_ONCE() in
nested_vmcb02_recalc_intercepts() if both INTERCEPT_VMLOAD and
SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE are set in vmcb02.

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index f9090b601efaa..67985c676db35 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));
>  }

[Severity: High]
Can an L1 guest or host userspace trigger this WARN_ON_ONCE() at will, and is
the hardware precedence between INTERCEPT_VMLOAD and
SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE reversed in the comment and commit message?

In nested_vmcb02_prepare_control(), SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE is set
in vmcb02->control.misc_ctl2 whenever !nested_vmcb_needs_vls_intercept(svm)
is true:

arch/x86/kvm/svm/nested.c:nested_vmcb_needs_vls_intercept() {
	if (!guest_cpu_cap_has(&svm->vcpu, X86_FEATURE_V_VMSAVE_VMLOAD))
		return true;

	if (!nested_npt_enabled(svm))
		return true;

	if (!(svm->nested.ctl.misc_ctl2 & SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE))
		return true;

	return false;
}

nested_vmcb_needs_vls_intercept() does not check whether INTERCEPT_VMLOAD is
set in vmcb12 or vmcb01. Immediately after setting
SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE, nested_vmcb02_prepare_control() calls
nested_vmcb02_recalc_intercepts(), which copies vmcb01->control.intercepts and
unconditionally ORs vmcb12_ctrl->intercepts into vmcb02->control.intercepts:

arch/x86/kvm/svm/nested.c:nested_vmcb02_recalc_intercepts() {
	...
	for (i = 0; i < MAX_INTERCEPT; i++)
		vmcb02->control.intercepts[i] = vmcb01->control.intercepts[i];
	...
	for (i = 0; i < MAX_INTERCEPT; i++)
		vmcb02->control.intercepts[i] |= vmcb12_ctrl->intercepts[i];
}

If an L1 guest sets both INTERCEPT_VMLOAD in vmcb12->control.intercepts and
SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE in vmcb12->control.misc_ctl2 (with nested NPT
and X86_FEATURE_V_VMSAVE_VMLOAD enabled), vmcb02 will have both bits set and
hit the new WARN_ON_ONCE() on VMRUN.

This occurs in normal software as well as malicious guests:

1. KVM's own selftest in
tools/testing/selftests/kvm/x86/nested_vmsave_vmload_test.c:l1_guest_code()
explicitly sets both INTERCEPT_VMLOAD | INTERCEPT_VMSAVE and
SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE in vmcb12 and runs L2 to verify that
SVM_EXIT_VMSAVE is still delivered:

	/* ..and SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE set */
	svm->vmcb->control.misc_ctl2 |= SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE;

	svm->vmcb->save.rip = (u64)l2_guest_code_vmsave;
	run_guest(svm->vmcb, svm->vmcb_gpa);
	GUEST_ASSERT_EQ(svm->vmcb->control.exit_code, SVM_EXIT_VMSAVE);

2. KVM running as an L1 hypervisor sets SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE in
init_vmcb() and never clears it, while svm_recalc_instruction_intercepts()
sets INTERCEPT_VMLOAD whenever L2 has EFER.SVME=0 or an Intel-compatible
CPUID:

arch/x86/kvm/svm/svm.c:svm_recalc_instruction_intercepts() {
	/*
	 * No need to toggle any of the vgif/vls/etc. enable bits here, as they
	 * are set when the VMCB is initialized and never cleared (if the
	 * relevant intercepts are set, the enablements are meaningless anyway).
	 */
	...
}

3. Host userspace can also trigger this warning via KVM_SET_NESTED_STATE
(svm_set_nested_state() -> nested_vmcb02_prepare_control() ->
nested_vmcb02_recalc_intercepts()), either by supplying a vmcb12 with both
bits set, or by calling KVM_SET_NESTED_STATE before the first KVM_RUN while
vmcb01->control.intercepts still has INTERCEPT_VMLOAD set from init_vmcb() (as
KVM_REQ_RECALC_INTERCEPTS is not serviced until vcpu_enter_guest()), or by
calling KVM_SET_CPUID2 to clear X86_FEATURE_V_VMSAVE_VMLOAD while in guest
mode.

As noted in the svm_recalc_instruction_intercepts() comment and verified by
nested_vmsave_vmload_test.c, AMD SVM hardware gives INTERCEPT_VMLOAD and
INTERCEPT_VMSAVE precedence over SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE. Should the
WARN_ON_ONCE() be dropped (or should SVM_MISC2_ENABLE_V_VMLOAD_VMSAVE not be
set when INTERCEPT_VMLOAD/VMSAVE is set), and the hardware behavior
description in the commit message updated?

> @@ -986,6 +994,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;
>  

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016235221.33726-2-amirmohammad.eftekhar@cispa.de?part=1