Re: [PATCH v3 5/7] KVM: x86: Recompute EFER.LMA instead of inheriting the current value on EFER writes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kvm: x86: guest-triggerable WARN_ON_ONCE in set_efer() via SVM state sync
--

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

KVM: x86: Recompute EFER.LMA instead of inheriting the current value on EFER writes

This commit updates set_efer() to recompute EFER_LMA from EFER_LME,
is_paging(vcpu), and is_pae(vcpu) instead of carrying forward the existing
EFER_LMA value from vcpu->arch.efer, and adds a WARN_ON_ONCE() if the
recomputed EFER_LMA differs from the previous value.

> diff --git a/arch/x86/kvm/msrs.c b/arch/x86/kvm/msrs.c
> index c3cb1575fe66e..c55402970a345 100644
> --- a/arch/x86/kvm/msrs.c
> +++ b/arch/x86/kvm/msrs.c
> @@ -638,8 +638,17 @@ static int set_efer(struct kvm_vcpu *vcpu, u64 efer)
>  	    (vcpu->arch.efer & EFER_SVME) && !(efer & EFER_SVME))
>  		return 1;
>  
> -	efer &= ~EFER_LMA;
> -	efer |= vcpu->arch.efer & EFER_LMA;
> +	if ((efer & EFER_LME) && is_paging(vcpu) && is_pae(vcpu))
> +		efer |= EFER_LMA;
> +	else
> +		efer &= ~EFER_LMA;
> +
> +	/*
> +	 * Toggling EFER.LMA on a bare write to EFER should be impossible, as
> +	 * it means KVM allowed architecturally invalid state at some point in
> +	 * the past, i.e. allowed EFER.LMA=1 && (!CR0.PG || !CR4.PAE).
> +	 */
> +	WARN_ON_ONCE((efer & EFER_LMA) != (vcpu->arch.efer & EFER_LMA));

[Severity: High]
As reported by Sashiko on v1 [1], can a guest or host userspace trigger this
WARN_ON_ONCE() via SVM paths that leave EFER_LMA in vcpu->arch.efer out of
sync with EFER_LME, X86_CR0_PG, and X86_CR4_PAE?

In arch/x86/kvm/msrs.c:set_efer(), changing EFER_LME while paging is enabled
is rejected earlier:

	if (is_paging(vcpu) && (vcpu->arch.efer & EFER_LME) != (efer & EFER_LME))
		return 1;

As a result, the recomputed EFER_LMA always equals
((vcpu->arch.efer & EFER_LME) && is_paging(vcpu) && is_pae(vcpu)), and the
WARN_ON_ONCE() fires whenever a vCPU enters set_efer() with vcpu->arch.efer's
EFER_LMA bit out of sync with EFER_LME, X86_CR0_PG, and X86_CR4_PAE. Several
SVM paths appear to still leave EFER_LMA out of sync:

1. In arch/x86/kvm/svm/nested.c:nested_vmcb02_prepare_save() and
arch/x86/kvm/svm/svm.c:svm_set_cr0():

	/* SVM ignores EFER.LMA if EFER.LME=0 (instead of failing VMRUN). */
	if (!(svm->nested.save.efer & EFER_LME))
		svm->nested.save.efer &= ~EFER_LMA;

	svm_set_efer(vcpu, svm->nested.save.efer);

	svm_set_cr0(vcpu, svm->nested.save.cr0);
	svm_set_cr4(vcpu, svm->nested.save.cr4);

where svm_set_cr0() only updates EFER_LMA on a transition of X86_CR0_PG
relative to the previous vcpu->arch.cr0:

	if (vcpu->arch.efer & EFER_LME) {
		if (!is_paging(vcpu) && (cr0 & X86_CR0_PG)) {
			vcpu->arch.efer |= EFER_LMA;
			if (!vcpu->arch.guest_state_protected)
				svm->vmcb->save.efer |= EFER_LMA | EFER_LME;
		}

		if (is_paging(vcpu) && !(cr0 & X86_CR0_PG)) {
			vcpu->arch.efer &= ~EFER_LMA;
			if (!vcpu->arch.guest_state_protected)
				svm->vmcb->save.efer &= ~(EFER_LMA | EFER_LME);
		}
	}

Because L1 has X86_CR0_PG=1 when executing VMRUN, is_paging(vcpu) is already
true when svm_set_cr0() runs. If L1 executes VMRUN with vmcb12->save
containing EFER_SVME=1, EFER_LME=1, EFER_LMA=0, X86_CR0_PG=1, X86_CR0_PE=1,
and X86_CR4_PAE=1 (which nested_vmcb_check_save() accepts because hardware
SVM ignores EFER_LMA in the VMCB save area), L2 runs with EFER_LMA=0 in
vcpu->arch.efer while EFER_LME=1, is_paging(vcpu), and is_pae(vcpu) are all
true. A subsequent WRMSR to MSR_EFER in L2 or KVM_SET_MSRS from userspace
enters set_efer(), computes EFER_LMA=1, and triggers the WARN_ON_ONCE().

2. On RSM from SMM into nested SVM via
emulator_leave_smm() -> svm_leave_smm() -> enter_svm_guest_mode() ->
nested_vmcb02_prepare_save(), emulator_leave_smm() clears X86_CR0_PG=0
before calling svm_leave_smm(). If vmcb12->save in guest memory has
EFER_SVME=1, EFER_LME=1, EFER_LMA=1, and X86_CR0_PG=0,
nested_vmcb02_prepare_save() does not clear EFER_LMA (since EFER_LME=1) and
svm_set_cr0() does not clear EFER_LMA (since is_paging(vcpu) is already
false). Immediately afterward, rsm_load_state_64() calls
__kvm_emulate_msr_write(vcpu, MSR_EFER, smstate->efer & ~EFER_LMA) ->
set_efer(), which computes EFER_LMA=0 against vcpu->arch.efer's EFER_LMA=1
and triggers the WARN_ON_ONCE().

3. In arch/x86/kvm/svm/nested.c:svm_set_nested_state(), the check on L1's
host save state in vmcb01->save only rejects (save->efer & EFER_LMA) &&
!(save->efer & EFER_LME):

	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;

and svm_leave_smm() similarly loads vmcb01->save from guest MSR_VM_HSAVE_PA
memory without checking EFER_LMA, allowing vmcb01->save to hold EFER_LME=1,
EFER_LMA=0, X86_CR0_PG=1, and X86_CR4_PAE=1. On nested_svm_vmexit() from a
paged L2 (where is_paging(vcpu) is already true):

	svm_set_efer(vcpu, vmcb01->save.efer);
	svm_set_cr0(vcpu, vmcb01->save.cr0 | X86_CR0_PE);
	svm_set_cr4(vcpu, vmcb01->save.cr4);

L1 is left with EFER_LMA=0 in vcpu->arch.efer while EFER_LME=1, X86_CR0_PG=1,
and X86_CR4_PAE=1, so the next WRMSR to MSR_EFER or KVM_SET_MSRS triggers
the WARN_ON_ONCE().

4. On SEV-SNP AP creation via sev_snp_init_protected_guest_state() and
kvm_vcpu_reset(), KVM initializes vcpu->arch.cr0 = X86_CR0_ET,
vcpu->arch.cr4 = 0, and vcpu->arch.efer = 0 while the guest VMSA can already
have X86_CR4_PAE=1 in hardware. In arch/x86/kvm/svm/svm.c:cr_trap():

	case 0:
		old_value = kvm_read_cr0(vcpu);
		svm_set_cr0(vcpu, new_value);

		kvm_post_set_cr0(vcpu, old_value, new_value);
		break;

If the AP traps on an MSR_EFER write setting EFER_LME=1 (while vcpu->arch.cr0
has X86_CR0_PG=0) and then traps on a CR0 write setting X86_CR0_PG=1 without
having trapped on CR4, cr_trap() -> svm_set_cr0() sets
vcpu->arch.efer |= EFER_LMA even though vcpu->arch.cr4 is still 0. A
subsequent MSR_EFER write trap before any CR4 write enters
efer_trap() -> set_efer(), sees !is_pae(vcpu), clears EFER_LMA in
vcpu->arch.efer while the guest is running in long mode, and triggers the
WARN_ON_ONCE().

[1] https://lore.kernel.org/r/sashiko-outbox-182134@kernel.org

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