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

[Yosry Ahmed]

On Thu, Aug 27, 2026 at 6:37 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:
> > >
> > > 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?
>
> No, because (a) this is KVM uAPI, not emulation of hardware, and (b) it's a check
> on L1 state, not L2 state.  It should be impossible for L1 state to have this
> combination through "natural" means, and so a snapshot provided by KVM should
> never have this combo either, which there's zero reason to allow userspace to
> provide garbage.

Right, I understand that KVM can do whatever it wants here. I was
wondering if we wanted to make the uAPI behavior match the  VMRUN
behavior, but I guess we're free to make it more strict.

>
> > > 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?
>
> Again, this is L1 state when L2 is active (the !KVM_STATE_NESTED_GUEST_MODE path
> has already bailed), and VMRUN "can only be executed in protected mode with SVM
> enabled".  Amusingly, the APM says #VMEXIT "Forces CR0.PE = 1, RFLAGS.VM = 0.",
> so I guess it means business.
>
> If anything is wrong, it's the CR0.PG check.  Presumably that got carried forward
> from commit c0725420cfdc ("KVM: SVM: Add helper functions for nested SVM").  I
> don't see anything in the APM that requires paging to be enabled, and nothing in
> that ancient series points at concrete documentation either.

Oh yeah you're right, for some reason I thought it was paging not
protected mode. Well then, it seems like
nested_svm_check_permissions() is also incorrectly checking paging as
well, seems like both checks are incorrect? Also, I don't see anything
in the APM about checking RFLAGS.VM before VMRUN.

If the goal here is to keep the checks here consistent with
nested_svm_check_permissions(), aside from the new EFER check, then
maybe we should also check CPL here?