Re: [PATCH 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:
- [High] Reading `vcpu->arch.cr3` directly captures a stale CR3 value when EPT is enabled, corrupting L1's CR3 on late VM-Fail.
--

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

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

This commit adds a dedicated field in struct nested_vmx to track L1's
pre-VM-Enter CR3, avoiding the use of vmcs01.GUEST_CR3. It also modifies
nested_vmx_enter_non_root_mode() to save the CR3 value, and
nested_vmx_restore_host_state() to restore it.

> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -3651,19 +3651,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 = vcpu->arch.cr3;

[Severity: High]
Does directly reading vcpu->arch.cr3 here capture a stale value when EPT is
enabled?

Because KVM does not intercept L1 CR3 writes when EPT is enabled, L1 can
update its CR3 without KVM immediately tracking it. On VM-Exit (such as for
VMLAUNCH), KVM invalidates the CR3 register cache by clearing the
VCPU_REG_CR3 availability bit.

If nested_vmx_enter_non_root_mode() directly assigns vmx->nested.pre_vmenter_cr3
from vcpu->arch.cr3 instead of using kvm_read_cr3(vcpu), it seems this would
bypass the cache refresh and stash a stale CR3 value.

>  
>  	vmx_switch_vmcs(vcpu, &vmx->nested.vmcs02);
>  

[ ... ]

> @@ -4975,7 +4970,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;

[Severity: High]
If pre_vmenter_cr3 stashed a stale value, will restoring it here corrupt L1's
CR3 on a late VM-Fail?

If a late VM-Fail occurs (detected by hardware but not KVM), writing this
potentially stale pre_vmenter_cr3 back into KVM's state (vcpu->arch.cr3) and
marking it as available could cause L1 to resume with an outdated CR3, which
might lead to guest memory corruption or a triple fault.

>  	kvm_register_mark_available(vcpu, VCPU_REG_CR3);
>  
>  	/*

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