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

[Sean Christopherson <seanjc@google.com>]

On Thu, Aug 27, 2026, Yosry Ahmed wrote:
> On Thu, Aug 27, 2026 at 6:37 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Thu, Aug 27, 2026, Yosry Ahmed wrote:
> > > On Wed, Aug 26, 2026 at 2:18 PM Sean Christopherson <seanjc@google.com> 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.

But again, this *does* match VMRUN behavior, because it's impossible for L1 to
have EFER.LMA=1, EFER.LME=0, and EFER.PAE=0 at the time of VMRUN.

> > > > 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@vger.kernel.org
> > > > Cc: Yosry Ahmed <yosry@kernel.org>
> > > > Reported-by: Stefan Teodorescu <fane@google.com>
> > > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > > ---
> > > >  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.

Presumably it's covered by the !PROTECTED_MODE clause.

  IF ((MSR_EFER.SVME == 0) || (!PROTECTED_MODE))  // This instruction can only be executed in protected
    EXCEPTION [#UD]                               // mode with SVM enabled

Section "1.3.4 Legacy Modes" describes "Protected Mode" and "Virtual-8086 Mode"
as separate submodes.  And the tables for most instructions differentiate between
Real, Virtual 8086, and Protected modes when enumerating exceptions.

The APM weasels around 64-bit mode by also saying "Before enabling and activating
long mode, system software must first enable protected mode".  E.g. the table for
SYSENTER describes the #UD due to "This instruction is not recognized in long mode"
as scenario that's unique to Protected Mode.

Stating the obvious, it would be lovely if the APM explicitly stated what the
exact checks are, though that's about as likely as AMD gifting me a pony.

FWIW, if that reading is wrong (though I'm pretty sure it's not), then the only
issue is that KVM is synthesizing #UD instead of #GP, because Virtual 8086 always
runs at CPL=3.

  IF (CPL != 0)                                   // This instruction is only allowed at CPL 0
    EXCEPTION [#GP]


> 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?