Re: [PATCH v2 2/2] KVM: nVMX: Don't use vmcs01.GUEST_CR3 to snapshot L1's CR3 when EPT is disabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `pre_vmenter_cr3` field is not serialized during live migration, causing L1's CR3 to be permanently lost and corrupted if a late VM-Fail occurs on the destination.
--

commit 2c3bbd56634c27ee75bdd6af8c48278025e7dcb4
Author: Sean Christopherson <seanjc@google.com>

KVM: nVMX: Don't use vmcs01.GUEST_CR3 to snapshot L1's CR3 when EPT is disabled

Add a dedicated field in "struct nested_vmx" to track L1's pre-VM-Enter CR3
instead of using vmcs01.GUEST_CR3.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 199b866072c0f..7a2251061bfa7 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
[ ... ]
> @@ -3669,19 +3669,14 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
>  				    &vmx->nested.pre_vmenter_ssp_tbl);
>  
>  	/*
> -	 * Overwrite vmcs01.GUEST_CR3 with L1's CR3 if EPT is disabled.  In the
> -	 * event of a "late" VM-Fail, i.e. a VM-Fail detected by hardware but
> -	 * not KVM, KVM must unwind its software model to the pre-VM-Entry host
> -	 * state.  When EPT is disabled, GUEST_CR3 holds KVM's shadow CR3, not
> -	 * L1's "real" CR3, which causes nested_vmx_restore_host_state() to
> -	 * corrupt vcpu->arch.cr3.  Stuffing vmcs01.GUEST_CR3 results in the
> -	 * unwind naturally setting arch.cr3 to the correct value.  Smashing
> -	 * vmcs01.GUEST_CR3 is safe because nested VM-Exits, and the unwind,
> -	 * reset KVM's MMU, i.e. vmcs01.GUEST_CR3 is guaranteed to be
> -	 * overwritten with a shadow CR3 prior to re-entering L1.
> +	 * Stash L1's CR3, so that in the event of a "late" VM-Fail, i.e. a
> +	 * VM-Fail detected by hardware but not KVM, KVM can unwind its
> +	 * software model to the pre-VM-Entry host state.  When EPT is
> +	 * disabled, GUEST_CR3 holds KVM's shadow CR3, not L1's "real" CR3,
> +	 * and so simply restoring from vmcs01.GUEST_CR3 would corrupt
> +	 * vcpu->arch.cr3.
>  	 */
> -	if (!enable_ept)
> -		vmcs_writel(GUEST_CR3, vcpu->arch.cr3);
> +	vmx->nested.pre_vmenter_cr3 = kvm_read_cr3(vcpu);

[Severity: High]
This is a pre-existing issue, but could this corrupt L1's CR3 during live
migration if a late VM-Fail occurs on the destination?

When migrating a nested VM that is currently in L2 (or has a pending nested
run), KVM_SET_NESTED_STATE calls nested_vmx_enter_non_root_mode() with
from_vmentry=false.

At this point, kvm_read_cr3() either holds L2's CR3 (if KVM_SET_SREGS
was already called by userspace) or the default initial CR3, but not L1's CR3.

[ ... ]
> @@ -4993,7 +4988,7 @@ static void nested_vmx_restore_host_state(struct kvm_vcpu *vcpu)
>  	vmx_set_cr4(vcpu, vmcs_readl(CR4_READ_SHADOW));
>  
>  	nested_ept_uninit_mmu_context(vcpu);
> -	vcpu->arch.cr3 = vmcs_readl(GUEST_CR3);
> +	vcpu->arch.cr3 = vmx->nested.pre_vmenter_cr3;
>  	kvm_register_mark_available(vcpu, VCPU_REG_CR3);

If a late hardware VM-Fail then occurs when KVM attempts to enter the guest
on the destination, nested_vmx_restore_host_state() will unwind L1's state
by setting vcpu->arch.cr3 to the value stashed above.

Since pre_vmenter_cr3 is not serialized as part of the nested state, doesn't
this clobber L1's CR3 with L2's CR3 or zero, potentially leading to memory
corruption or a kernel panic in L1?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260612145642.452392-1-seanjc@google.com?part=2