Re: [PATCH 3/4] KVM: x86/mmu: Bug the VM if KVM calcs a CPU role with EFER.LMA=1 && CR4.PAE=0

[Yosry Ahmed]

On Thu, Aug 27, 2026 at 7:57 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Thu, Aug 27, 2026, Yosry Ahmed wrote:
> > On Wed, Aug 26, 2026 at 2:18 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > >
> > > Bug the VM if KVM attempts to construct a CPU role with the should-be-
> > > impossible combination of long mode being active without PAE paging being
> > > enabled.  KVM's MMU construction assumes that EFER.LMA can be set if and
> > > only CR4.PAE is set, and will create a completely invalid MMU if that
> > > assumption fails.  FNAME(walk_addr_generic) already has sanity checks to
> > > try and mitigate the fallout, but attempt to catch such bugs earlier, as
> > > this is (at least) the second time KVM has had bugs that escaped into
> > > FNAME(walk_addr_generic), and it's entirely possible the bad state could
> > > cause problems elsewhere.
> > >
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > ---
> > >  arch/x86/kvm/mmu/mmu.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > > index 064ecc33b926..81c30e2c74f3 100644
> > > --- a/arch/x86/kvm/mmu/mmu.c
> > > +++ b/arch/x86/kvm/mmu/mmu.c
> > > @@ -5910,6 +5910,9 @@ static union kvm_cpu_role kvm_calc_cpu_role(struct kvm_vcpu *vcpu,
> > >                 return role;
> > >         }
> > >
> > > +       if (KVM_BUG_ON(____is_efer_lma(regs) && !____is_cr4_pae(regs), vcpu->kvm))
> >
> > Can we shove this into the existing if (____is_efer_lma(regs)) below?
>
> No, because there are three more checks on EFER.LMA:
>
>         role.ext.cr4_pke = ____is_efer_lma(regs) && ____is_cr4_pke(regs);
>         role.ext.cr4_la57 = ____is_efer_lma(regs) && ____is_cr4_la57(regs);
>         role.ext.efer_lma = ____is_efer_lma(regs);
>
> and I don't want to have to condition them all on something that shouldn't happen.

Yeah I assumed that we don't care about the state anymore if we'll
KVM_BUG_ON(), but apparently that's not the case based on your comment
below.

>
> OMG, I hate SVM.  I resurrected the selftest hack I used to verify this bug, to
> demonstrate that Sashiko's "technically that's undefined behavior and this is
> useless" complaint is wrong, because even though it's undefined behavior and the
> compiler *could* ignore the change, in practice the compiler probably won't ignore
> the change.  And since this is defense-in-depth, it's "fine" if the paranoid
> hardening only isn't guaranteed to kick in.
>
> And in doing so managed to trip this KVM_BUG_ON() in *L0* when running the test
> in L1, because as you kinda sorta noted in patch 1, KVM doesn't ignore EFER.LMA
> when loading L2 state.
>
> I had actually tried to do exactly that, by having nested_vmcb_check_save() clear
> EFER.LMA if EFER.LME=0, but that doesn't work because svm_set_nested_state() uses
> the "cache" only for the checks, not for the actual loading of state.  *sigh*
>
> So in addition to patch 1, we also need this to guard against configuring L2's
> walk_mmu with bad state.

Hmm wouldn't it be simpler at this point to let KVM_SET_NESTED_STATE
and nested VMRUN have the invalid LMA/LME combination and just ignore
EFER.LMA if EFER.LME (or just always check EFER.LMA && EFER.LME)?

>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 49fb10ad1f9f..23d29597d6bf 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -789,6 +789,10 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm)
>
>         kvm_set_rflags(vcpu, save->rflags | X86_EFLAGS_FIXED);
>
> +       /* 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);
>
> Anyways, back to Sashiko's "technically this is wrong" statement, I confirmed
> that tweaking the code to do this does NOT trigger the KVM_BUG_ON() with at least
> clang-21.  I.e. my assertion that clearing regs->efer.LMA could be useful holds
> true.
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index b515a49c5e86..cab8690d0fa0 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5932,8 +5932,10 @@ static union kvm_cpu_role kvm_calc_cpu_role(struct kvm_vcpu *vcpu,
>                 return role;
>         }
>
> -       if (KVM_BUG_ON(____is_efer_lma(regs) && !____is_cr4_pae(regs), vcpu->kvm))
> +       if (____is_efer_lma(regs) && !____is_cr4_pae(regs)) {
> +               pr_warn("Forcing EFER.LMA=0 in calc CPU role\n");
>                 *(u64 *)&regs->efer &= ~EFER_LMA;
> +       }
>
>         role.base.efer_nx = ____is_efer_nx(regs);
>         role.base.cr0_wp = ____is_cr0_wp(regs);
> @@ -5941,6 +5943,8 @@ static union kvm_cpu_role kvm_calc_cpu_role(struct kvm_vcpu *vcpu,
>         role.base.smap_andnot_wp = ____is_cr4_smap(regs) && !____is_cr0_wp(regs);
>         role.base.has_4_byte_gpte = !____is_cr4_pae(regs);
>
> +       KVM_BUG_ON(____is_efer_lma(regs) && !____is_cr4_pae(regs), vcpu->kvm);
> +
>         if (____is_efer_lma(regs))
>                 role.base.level = ____is_cr4_la57(regs) ? PT64_ROOT_5LEVEL
>                                                         : PT64_ROOT_4LEVEL;
>
> Side topic, I also (inadvertantly) somewhat justified keeping the
>
>         if (KVM_BUG_ON(is_long_mode(vcpu) && !is_pae(vcpu), vcpu->kvm) ||
>
> check in FNAME(walk_addr_generic) when testing the above.  If KVM manages to
> configure a sane MMU, but still has a vCPU with the above state, then we still
> want to WARN and bail.

Good point.

>
> > > +               *(u64 *)&regs->efer &= ~EFER_LMA;
> >
> > Why do this if we will crash the VM anyway (and Sashiko doesn't like it)?
>
> Because there's a lot of code between here and checking KVM_VM_DEAD in
> vcpu_enter_guest().  And has been proven far too many times this year, detecting
> a flaw doesn't automagically mitigate true badness.

Interesting, I always assumed we can do whatever we want after KVM_BUG_ON() :P

>
> > > +
> > >         role.base.efer_nx = ____is_efer_nx(regs);
> > >         role.base.cr0_wp = ____is_cr0_wp(regs);
> > >         role.base.cr4_smep = ____is_cr4_smep(regs);
> > > --
> > > 2.55.0.887.g758fc8c411-goog
> > >