Re: [PATCH 1/4] KVM: nSVM: Reject KVM_SET_NESTED_STATE if L1 has EFER.LMA=1 && EFER.LME=0

[Yosry Ahmed]

On Wed, Aug 26, 2026 at 2:18 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> Reject KVM_SET_NESTED_STATE if the incoming L1 host state has what is
> effectively an impossible EFER combination of LMA=1 but LME=0, i.e. if the
> state says long mode is active but not enabled.  Unlike VMX, SVM doesn't
> have an explicit consistent check for the illegal combination; presumably
> hardware simply ignores EFER.LMA if EFER.LME=0.
>
> Unfortunately, KVM doesn't ignore EFER.LMA in this case and consumes the
> illegal state when constructing the shadow MMU for L2.  E.g. if userspace
> also clears CR4.PAE, then kvm_calc_cpu_role() will compute a role with 4 or
> 5 levels of paging, but shadow_mmu_init_context() will wire up the MMU to
> use the paging32 template, which maxes out its levels at 2.

Isn't the "right" thing to do what hardware (presumably) does and
ignore EFER.LMA if EFER.LME=0?

>
> Note, the "real badness" is effectively the same as what happened with the
> nVMX bug fixed by commit 112e66017bff ("KVM: nVMX: add missing consistency
> checks for CR0 and CR4").  Unfortunately, the sanity check added by commit
> 72e2fb24a0b0 ("KVM: x86/mmu: Bug the VM if a vCPU ends up in long mode
> without PAE enabled") doesn't work for this case, since L2 state is active
> at the time of the page fault, but it's L1 that has the bad state.
>
> Fixes: cc440cdad5b7 ("KVM: nSVM: implement KVM_GET_NESTED_STATE and KVM_SET_NESTED_STATE")
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Yosry Ahmed <yosry@xxxxxxxxxx>
> Reported-by: Stefan Teodorescu <fane@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  arch/x86/kvm/svm/nested.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 73f37b050d0a..49fb10ad1f9f 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -2028,6 +2028,7 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>         if (!(save->cr0 & X86_CR0_PG) ||
>             !(save->cr0 & X86_CR0_PE) ||
>             (save->rflags & X86_EFLAGS_VM) ||
> +           ((save->efer & EFER_LMA) && !(save->efer & EFER_LME)) ||

I just realized I have no idea why we check X86_CR0_PE and
X86_EFLAGS_VM here. Commit 6906e06db9b04 ("KVM: nSVM: Add missing
checks for reserved bits to svm_set_nested_state()") says it's to do
the same checks as VMRUN, but I don't think that's actually the case?
The checks here seem arbitrary to me?

>             !nested_vmcb_check_save(vcpu, &save_cached, false))
>                 goto out_free;
>
> --
> 2.55.0.887.g758fc8c411-goog
>