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 10:33 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Thu, Aug 27, 2026, Yosry Ahmed wrote:
> > 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.
>
> 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@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.
>
> 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]

I see, thanks for digging this up. Either way I think we want to keep
things consistent between here and nested_svm_check_permissions(). We
probably want a CPL check here, and it would be a superset of
X86_EFLAGS_VM as you mentioned. We probably wanna drop the CR0.PG
check from both places, and add an explicit X86_EFLAGS_VM check in
nested_svm_check_permissions() to #UD instead of #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?