Re: [PATCH v2 4/6] 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 3 potential issue(s) to consider:
- [High] kvm: x86: guest- and userspace-reachable WARN_ON_ONCE() in set_efer()
- [High] kvm: x86: missing MMU context reset in set_efer() when EFER_LMA changes
- [High] KVM: nSVM: guest-reachable WARN_ON_ONCE in set_efer() due to stale EFER.LMA
--

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

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

This commit recomputes EFER.LMA from EFER.LME, CR0.PG, and CR4.PAE on bare
writes to MSR_EFER instead of inheriting the existing EFER.LMA bit from
vcpu->arch.efer, and adds a WARN_ON_ONCE() if EFER.LMA changes.

> 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]
Can an unprivileged guest or host userspace trigger this WARN_ON_ONCE() in
arch/x86/kvm/msrs.c:set_efer() by leaving vcpu->arch.efer with a stale
EFER_LMA across nested SVM transitions?

On AMD SVM, VMRUN ignores EFER.LMA in the VMCB save area and computes it
from (EFER.LME && CR0.PG), so arch/x86/kvm/svm/nested.c:nested_vmcb_check_save()
does not require save->efer's EFER_LMA to match (EFER.LME && CR0.PG).

In arch/x86/kvm/svm/nested.c:nested_vmcb02_prepare_save(), KVM only clears
EFER_LMA when EFER_LME is 0, and relies on svm_set_cr0() to update EFER_LMA
when EFER_LME is 1:

arch/x86/kvm/svm/nested.c:nested_vmcb02_prepare_save() {
	/* 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);
}

However, arch/x86/kvm/svm/svm.c:svm_set_cr0() only updates EFER_LMA when
CR0.PG changes relative to the previous vcpu->arch.cr0 (is_paging(vcpu)):

arch/x86/kvm/svm/svm.c:svm_set_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);
		}
	}
}

This allows vcpu->arch.efer's EFER_LMA to become out of sync with
(EFER.LME && CR0.PG && CR4.PAE) in several ways:

1. Guest VMRUN (nested_svm_vmrun() -> enter_svm_guest_mode() ->
nested_vmcb02_prepare_save()): L1 must have CR0.PG=1 to execute VMRUN, so
is_paging(vcpu) is already true when nested_vmcb02_prepare_save() runs. If
L1 sets vmcb12->save with EFER_LME=1, EFER_LMA=0, CR0.PG=1, CR0.PE=1, and
CR4.PAE=1, svm_set_efer() loads EFER_LMA=0 into vcpu->arch.efer (and also
clears EFER_LME in vmcb02->save.efer when !npt_enabled), and svm_set_cr0()
sees !is_paging(vcpu) == false and never sets EFER_LMA. L2 is left with
EFER_LME=1, EFER_LMA=0, CR0.PG=1, CR4.PAE=1 (which also truncates L2's
64-bit RSP and RIP to 32 bits and configures the MMU for 32-bit PAE), and
a subsequent WRMSR(MSR_EFER) in L2 (or KVM_SET_MSRS from userspace)
computes EFER_LMA=1 and trips the WARN_ON_ONCE() in set_efer().

2. Guest RSM into L2 (emulator_leave_smm() -> svm_leave_smm() ->
enter_svm_guest_mode() -> nested_vmcb02_prepare_save() ->
rsm_load_state_64()): emulator_leave_smm() clears CR0.PG before calling
svm_leave_smm(), so is_paging(vcpu) is false. If vmcb12->save in guest
memory has EFER_LME=1, EFER_LMA=1, and CR0.PG=0, nested_vmcb02_prepare_save()
does not clear EFER_LMA and svm_set_cr0() sees is_paging(vcpu) == false and
leaves EFER_LMA=1 in vcpu->arch.efer while CR0.PG=0. Immediately after
svm_leave_smm() returns, arch/x86/kvm/smm.c:rsm_load_state_64() calls
__kvm_emulate_msr_write(vcpu, MSR_EFER, smstate->efer & ~EFER_LMA) ->
set_efer(), which computes EFER_LMA=0 and triggers the WARN_ON_ONCE().

3. KVM_SET_NESTED_STATE or SMM HSAVE restoration followed by nested #VMEXIT
(svm_set_nested_state() / svm_leave_smm() -> nested_svm_vmexit()):
arch/x86/kvm/svm/nested.c:svm_set_nested_state() requires CR0.PG=1 in L1's
saved host state (vmcb01->save) and rejects (EFER_LMA && !EFER_LME), but
allows EFER_LME=1 with EFER_LMA=0:

arch/x86/kvm/svm/nested.c:svm_set_nested_state() {
	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;
}

When exiting from a paged L2 (is_paging(vcpu) == true),
arch/x86/kvm/svm/nested.c:nested_svm_vmexit() calls
svm_set_efer(vcpu, vmcb01->save.efer) followed by
svm_set_cr0(vcpu, vmcb01->save.cr0 | X86_CR0_PE), leaving L1 with
EFER_LME=1, EFER_LMA=0, CR0.PG=1, CR4.PAE=1 and triggering the
WARN_ON_ONCE() on the next MSR_EFER write.

4. SEV-SNP AP creation (sev_snp_ap_creation() ->
KVM_REQ_UPDATE_PROTECTED_GUEST_STATE -> kvm_vcpu_reset()): svm_set_cr0()
sets EFER_LMA without checking is_pae(vcpu) while vcpu->arch.cr4 still has
CR4.PAE=0, allowing a subsequent trapped EFER write via efer_trap() to
trigger the WARN_ON_ONCE().

>  
>  	r = kvm_x86_call(set_efer)(vcpu, efer);
>  	if (r) {

[Severity: High]
When set_efer() toggles EFER_LMA to restore architectural consistency,
doesn't it also need to reset the MMU context if EFER_LMA changed?

At the end of arch/x86/kvm/msrs.c:set_efer(), KVM only calls
kvm_mmu_reset_context(vcpu) when bits in KVM_MMU_EFER_ROLE_BITS change:

arch/x86/kvm/msrs.c:set_efer() {
	...
	if ((efer ^ old_efer) & KVM_MMU_EFER_ROLE_BITS)
		kvm_mmu_reset_context(vcpu);
}

However, KVM_MMU_EFER_ROLE_BITS in arch/x86/kvm/mmu.h is defined as
(EFER_LME | EFER_NX) and does not include EFER_LMA, even though
arch/x86/kvm/mmu/mmu.c:kvm_calc_cpu_role() computes role.base.level,
role.ext.efer_lma, role.ext.cr4_pke, and role.ext.cr4_la57 from
____is_efer_lma(regs).

Because set_efer() rejects any write that modifies EFER_LME while
is_paging(vcpu) is true, ((efer ^ old_efer) & EFER_LME) is always 0 when
paging is enabled. If EFER_LMA is toggled by set_efer() while EFER_NX is
unchanged, ((efer ^ old_efer) & KVM_MMU_EFER_ROLE_BITS) evaluates to 0 and
kvm_mmu_reset_context(vcpu) is skipped, leaving the vCPU's MMU role and root
level stale relative to the updated vcpu->arch.efer.

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