Re: [PATCH 01/45] KVM: nVMX/nSVM: check unconditionally whether CR3 is valid

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: nSVM: missing L1 host CR3 check in nested_svm_vmexit() after RSM
- [Medium] KVM: nSVM: unvalidated L2 CR3 and dead check in svm_set_nested_state()
--

commit 111aed734bf3c907c07b30f4fd256e2dd365f8e8
Author: Paolo Bonzini <pbonzini@redhat.com>

KVM: nVMX/nSVM: check unconditionally whether CR3 is valid

This commit moves the CR3 validity check out of nested_svm_load_cr3() and
nested_vmx_load_cr3() into nested_vmcb_check_save() and
nested_vmx_check_guest_state() so that CR3 is checked unconditionally in all
processor modes during guest state validation.

> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index af0ef678433d7..fedddcd1cc964 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c

[ ... ]

> @@ -717,9 +717,6 @@ static void nested_svm_transition_tlb_flush(struct kvm_vcpu *vcpu)
>  static int nested_svm_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3,
>  			       bool nested_npt, bool reload_pdptrs)
>  {
> -	if (CC(!kvm_vcpu_is_legal_cr3(vcpu, cr3)))
> -		return -EINVAL;
> -

[Severity: High]
Does removing kvm_vcpu_is_legal_cr3() from nested_svm_load_cr3() allow an
unvalidated L1 host CR3 to be loaded on nested #VMEXIT after returning from
SMM?

When an SMI arrives while L2 is running, svm_enter_smm() saves L1's host
state from vmcb01->save into guest memory at MSR_VM_HSAVE_PA + 0x400, where
the SMM handler can modify the saved CR3. On RSM, svm_leave_smm() in
arch/x86/kvm/svm/svm.c copies that HSAVE state back into vmcb01->save, but
only validates vmcb12:

arch/x86/kvm/svm/svm.c:svm_leave_smm() {
	...
	svm_copy_vmrun_state(vcpu, &svm->vmcb01.ptr->save, m_save.map.hva + 0x400);

	/*
	 * Enter the nested guest now
	 */

	vmcb_mark_all_dirty(svm->vmcb01.ptr);

	vmcb12 = m.map.hva;
	nested_copy_vmcb_control_to_cache(svm, &vmcb12->control);
	nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);

	if (nested_svm_check_cached_vmcb12(vcpu) < 0)
		return 1;
	...
}

On the subsequent nested #VMEXIT from L2 to L1, nested_svm_vmexit() in
arch/x86/kvm/svm/nested.c reloads vmcb01->save.cr3 via
nested_svm_load_cr3() and expects an invalid CR3 to trigger a triple fault:

arch/x86/kvm/svm/nested.c:nested_svm_vmexit() {
	...
	if (nested_svm_load_cr3(vcpu, vmcb01->save.cr3, false, true))
		kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);
	...
}

Without kvm_vcpu_is_legal_cr3() in nested_svm_load_cr3(), if L1 is in
64-bit mode (or 32-bit non-PAE mode) so is_pae_paging(vcpu) is false,
nested_svm_load_cr3() returns 0 and installs the illegal CR3 into
vcpu->arch.cr3. When NPT is enabled, svm_load_mmu_pgd() then writes it into
vmcb01->save.cr3, causing hardware VMRUN of L1 to fail with SVM_EXIT_ERR
(KVM_EXIT_FAIL_ENTRY) instead of injecting KVM_REQ_TRIPLE_FAULT.

Should svm_leave_smm() validate the restored host state in vmcb01->save via
nested_vmcb_check_save() (similar to how vmx_leave_smm() calls
nested_vmx_check_restored_vmcs12()), or should nested_svm_vmexit() still
verify kvm_vcpu_is_legal_cr3()?

[Severity: Medium]
Similarly, in svm_set_nested_state() in arch/x86/kvm/svm/nested.c,
nested_vmcb_check_save() only validates L1's saved host state in
user_vmcb->save (copied into save_cached), while L2's current state is held
in vcpu->arch and checked separately at the top of svm_set_nested_state():

arch/x86/kvm/svm/nested.c:svm_set_nested_state() {
	...
	cr0 = kvm_read_cr0(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))
		goto out_free;
	...
	ret = nested_svm_load_cr3(&svm->vcpu, vcpu->arch.cr3,
				  nested_npt_enabled(svm), false);
	if (ret)
		goto out_free;
	...
}

Because kvm_is_valid_sregs() in arch/x86/kvm/regs.c only checks
kvm_vcpu_is_legal_cr3() when long mode is active, and because userspace may
also update CPUID MAXPHYADDR between KVM_SET_SREGS and
KVM_SET_NESTED_STATE, removing kvm_vcpu_is_legal_cr3() from
nested_svm_load_cr3() leaves L2's vcpu->arch.cr3 unvalidated here. In
addition, because svm_set_nested_state() passes reload_pdptrs = false to
nested_svm_load_cr3(), nested_svm_load_cr3() can no longer return a
non-zero value on this path, leaving the if (ret) check dead.

Should svm_set_nested_state() also check kvm_vcpu_is_legal_cr3(vcpu,
vcpu->arch.cr3) alongside the L2 CR0 check?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102174130.8779-1-pbonzini@redhat.com?part=1