[PATCH] KVM: nSVM: Validate L1 host save state restored on RSM from SMM

[PATCH] KVM: nSVM: Validate L1 host save state restored on RSM from SMM

[Abdelali Maddane]

When an SMI is taken while L2 is running, svm_enter_smm() saves L1's host
state into the HSAVE area (ordinary guest memory pointed to by
MSR_VM_HSAVE_PA).  On RSM, svm_leave_smm() copies that state back into
vmcb01 and builds the nested MMU from it, but unlike KVM_SET_NESTED_STATE
it does not validate the restored state.

Because the HSAVE area is guest memory, L1 can modify it while the vCPU is
in SMM so that RSM loads an impossible EFER.LMA=1 && EFER.LME=0 combination
(e.g. together with CR4.PAE=0).  kvm_calc_cpu_role() then computes a 4- or
5-level role while shadow_mmu_init_context() wires up the 2-level paging32
template, and FNAME(walk_addr_generic) indexes out of bounds of the
fixed-size guest_walker arrays.  On kernels carrying the KVM_BUG_ON()
backstop in kvm_calc_cpu_role() this is a guest-triggerable KVM_BUG_ON()
(dead VM, or a host panic with panic_on_warn); on older kernels it is an
out-of-bounds access in the page-table walker.

Validate the restored L1 host state in svm_leave_smm() with the same checks
KVM_SET_NESTED_STATE already applies to L1 host state, factored out into
nested_svm_check_l1_save().  A failed check fails RSM, which is the correct
response to an impossible saved state.

The patch, reproducer, and this changelog were developed with substantial
help from an AI assistant (Anthropic's Claude Code), directed and reviewed
by the submitter.  Tested by the submitter on an AMD EPYC host: without the
patch a reproducer triggers the guest-reachable KVM_BUG_ON() in
kvm_calc_cpu_role(); with the patch the reproducer is rejected, and the
smm_test and svm_nested_shutdown_test selftests still pass.

Fixes: 37be407b2ce8 ("KVM: nSVM: Fix L1 state corruption upon return from SMM")
Signed-off-by: Abdelali Maddane <ma.08@web.de>
---
Reproducer (a KVM selftest) available on request.

 arch/x86/kvm/svm/nested.c | 36 ++++++++++++++++++++++--------------
 arch/x86/kvm/svm/svm.c    |  7 +++++++
 arch/x86/kvm/svm/svm.h    |  1 +
 3 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
index af0ef6784..2295bd90a 100644
--- a/arch/x86/kvm/svm/nested.c
+++ b/arch/x86/kvm/svm/nested.c
@@ -616,6 +616,26 @@ void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm,
 	__nested_copy_vmcb_save_to_cache(&svm->nested.save, save);
 }
 
+/*
+ * Validate L1 host save state loaded on return to L1 (RSM from SMM, or via
+ * KVM_SET_NESTED_STATE).  g_pat is not checked: it may be clobbered by SMM and
+ * isn't loaded at emulated #VMEXIT.
+ */
+int nested_svm_check_l1_save(struct kvm_vcpu *vcpu, struct vmcb_save_area *save)
+{
+	struct vmcb_save_area_cached save_cached;
+
+	__nested_copy_vmcb_save_to_cache(&save_cached, save);
+	if (!(save->cr0 & X86_CR0_PG) ||
+	    !(save->cr0 & X86_CR0_PE) ||
+	    (save->rflags & X86_EFLAGS_VM) ||
+	    ((save->efer & EFER_LMA) && !(save->efer & EFER_LME)) ||
+	    !nested_vmcb_check_save(vcpu, &save_cached, false))
+		return -EINVAL;
+
+	return 0;
+}
+
 /*
  * Synchronize fields that are written by the processor, so that
  * they can be copied back into the vmcb12.
@@ -1968,7 +1988,6 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
 		&user_kvm_nested_state->data.svm[0];
 	struct vmcb_control_area *ctl;
 	struct vmcb_save_area *save;
-	struct vmcb_save_area_cached save_cached;
 	struct vmcb_ctrl_area_cached ctl_cached;
 	bool use_separate_l2_pat;
 	unsigned long cr0;
@@ -2033,19 +2052,8 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
         if (((cr0 & X86_CR0_CD) == 0) && (cr0 & X86_CR0_NW))
 		goto out_free;
 
-	/*
-	 * Validate host state saved from before VMRUN (see
-	 * nested_svm_check_permissions). Note that the g_pat field is not
-	 * validated, because (a) it may have been clobbered by SMM before
-	 * KVM_GET_NESTED_STATE, and (b) it is not loaded at emulated
-	 * #VMEXIT.
-	 */
-	__nested_copy_vmcb_save_to_cache(&save_cached, save);
-	if (!(save->cr0 & X86_CR0_PG) ||
-	    !(save->cr0 & X86_CR0_PE) ||
-	    (save->rflags & X86_EFLAGS_VM) ||
-	    ((save->efer & EFER_LMA) && !(save->efer & EFER_LME)) ||
-	    !nested_vmcb_check_save(vcpu, &save_cached, false))
+	/* Validate host state saved from before VMRUN. */
+	if (nested_svm_check_l1_save(vcpu, save))
 		goto out_free;
 
 	/*
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 136069dfc..9de22fa92 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -5224,6 +5224,13 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram)
 
 	svm_copy_vmrun_state(vcpu, &svm->vmcb01.ptr->save, m_save.map.hva + 0x400);
 
+	/*
+	 * The HSAVE area is guest memory; validate the restored L1 state before
+	 * it is used to build the nested MMU, same as KVM_SET_NESTED_STATE.
+	 */
+	if (nested_svm_check_l1_save(vcpu, &svm->vmcb01.ptr->save))
+		return 1;
+
 	/*
 	 * Enter the nested guest now
 	 */
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 6d0242966..f7f0f3071 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -901,6 +901,7 @@ static inline void nested_svm_simple_vmexit(struct vcpu_svm *svm, u32 exit_code)
 int nested_svm_exit_handled(struct vcpu_svm *svm);
 int nested_svm_check_permissions(struct kvm_vcpu *vcpu);
 int nested_svm_check_cached_vmcb12(struct kvm_vcpu *vcpu);
+int nested_svm_check_l1_save(struct kvm_vcpu *vcpu, struct vmcb_save_area *save);
 int nested_svm_check_exception(struct vcpu_svm *svm, unsigned nr,
 			       bool has_error_code, u32 error_code);
 int nested_svm_exit_special(struct vcpu_svm *svm);

base-commit: dea4f6380f4ec96e8f3b0aaf2343764bd0d25ef4
-- 
2.56.0

Re: [PATCH] KVM: nSVM: Validate L1 host save state restored on RSM from SMM

[Abdelali Maddane]

Please disregard this patch, the same issue is already fixed by Sean's
"[PATCH 1/7] KVM: nSVM: Check validity of vmcs01 save area copied from
SMRAM on RSM" [1],
which I missed before sending. Sorry for the noise.

[1] https://lore.kernel.org/kvm/20261102231428.13523-1-pbonzini@redhat.com/

Thanks,
Abdelali

Re: [PATCH] KVM: nSVM: Validate L1 host save state restored on RSM from SMM

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: nSVM: svm_leave_smm() clobbers vmcb01 save area before validation
--

commit 01dc0fd5f011d2d987b3ef7b232d855ee26c4055
Author: Abdelali Maddane <ma.08@web.de>

KVM: nSVM: Validate L1 host save state restored on RSM from SMM

This commit factors out the L1 host save state validation from
svm_set_nested_state() into nested_svm_check_l1_save() and calls it in
svm_leave_smm() to reject invalid L1 state restored from the guest HSAVE
area on RSM.

> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 136069dfcd44f..9de22fa928efa 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -5224,6 +5224,13 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const union kvm_smram *smram)
>  
>  	svm_copy_vmrun_state(vcpu, &svm->vmcb01.ptr->save, m_save.map.hva + 0x400);
>  
> +	/*
> +	 * The HSAVE area is guest memory; validate the restored L1 state before
> +	 * it is used to build the nested MMU, same as KVM_SET_NESTED_STATE.
> +	 */
> +	if (nested_svm_check_l1_save(vcpu, &svm->vmcb01.ptr->save))
> +		return 1;

[Severity: Medium]
In arch/x86/kvm/svm/svm.c, svm_leave_smm() calls svm_copy_vmrun_state() to
copy the untrusted guest HSAVE state into svm->vmcb01.ptr->save before
validating it with nested_svm_check_l1_save() (and before
nested_svm_check_cached_vmcb12()).  Unlike svm_set_nested_state() in
arch/x86/kvm/svm/nested.c, which validates the candidate save area before
calling svm_copy_vmrun_state(), doesn't returning early here leave the
rejected HSAVE state in the active vmcb01 save area out of sync with
vcpu->arch?

Before calling svm_leave_smm(), rsm_interception() ->
kvm_emulate_instruction_from_buffer() -> em_rsm() -> emulator_leave_smm()
in arch/x86/kvm/smm.c transitions vcpu->arch and vmcb01 to 32-bit real mode:

	cr0 = kvm_read_cr0(vcpu);
	if (cr0 & X86_CR0_PE)
		kvm_set_cr0(vcpu, cr0 & ~(X86_CR0_PG | X86_CR0_PE));

When nested_svm_check_l1_save() fails in svm_leave_smm(), em_rsm() queues
KVM_REQ_TRIPLE_FAULT and exits to userspace with KVM_EXIT_SHUTDOWN, leaving
svm->vmcb01.ptr->save populated with the unvalidated HSAVE fields while
vcpu->arch reflects 32-bit real mode.

If userspace then calls KVM_GET_SREGS, __get_sregs_common() reads segment
registers, GDT, and IDT from svm->vmcb01.ptr->save (such as cs.l = 1) while
reading cr0, cr4, and efer from vcpu->arch (efer = 0, cr0.PG = 0),
producing an inconsistent kvm_sregs structure that KVM_SET_SREGS rejects
with -EINVAL.  Furthermore, if userspace invokes KVM_RUN again without a
full KVM_SET_SREGS reset, svm_vcpu_run() in arch/x86/kvm/svm/svm.c will
sync the rejected vmcb01 cr0 and cr3 back into vcpu->arch without updating
vcpu->arch.root_mmu:

		if (!svm_is_intercept(svm, INTERCEPT_CR0_WRITE))
			vcpu->arch.cr0 = svm->vmcb->save.cr0;
		if (npt_enabled)
			vcpu->arch.cr3 = svm->vmcb->save.cr3;

And when npt_enabled is false, svm_copy_vmrun_state() will have overwritten
the mandatory shadow-paging bits (X86_CR0_PG | X86_CR0_WP and X86_CR4_PAE)
in svm->vmcb01.ptr->save.cr0 and cr4 with raw guest HSAVE values.

Should nested_svm_check_l1_save() validate m_save.map.hva + 0x400 (and
should nested_svm_check_cached_vmcb12() also run) before
svm_copy_vmrun_state() modifies svm->vmcb01.ptr->save?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103230750.1843-1-ma.08@web.de?part=1