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

[sashiko-bot@kernel.org]

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