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

[sashiko-bot@kernel.org]

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