Re: [PATCH v2 2/6] KVM: x86: Reject clearing EFER.SVME via KVM_SYNC_X86_SREGS if L2 is active

[Yosry Ahmed]

On Wed, Oct 28, 2026 at 6:36 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Tue, Oct 27, 2026, Yosry Ahmed wrote:
> > On Tue, Oct 27, 2026 at 3:49 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > >
> > > Disallow clearing EFER.SVME via KVM_SYNC_X86_SREGS if L2 is active so that
> > > svm_set_efer() can treat yanking the vCPU out of L2 during KVM_RUN as a KVM
> > > bug.  Ideally, kvm_is_valid_sregs() itself would reject attempts to clear
> > > SVME while L2 is active, but the risk of breaking userspace, e.g. across
> > > save/restore, is too high.
> >
> > I am confused. This says we shouldn't disallow clearing SVME while L2
> > is active because we may break userspace..
> >
> > > On the other hand, yanking the vCPU out of L2
> > > during save/restore is already "broken" as doing so would queue a triple
> > > fault for L1.
> >
> > ..and this says it's already broken.
>
> Gah, the "save/restore" part is a goof.  KVM_SYNC_X86_SREGS is already broken.
> That should read:
>
>   On the other hand, yanking the vCPU out of L2 via KVM_SYNC_X86_SREGS is
>   already "broken" as doing so would queue a triple fault for L1 (because
>   "wants_to_run" will be true).

Much clearer :)

>
> > I suspect you in the first paragraph were referring to userspace
> > restoring a vCPU not in guest mode on top of a vCPU in guest mode or
> > similar? Might be worth clarifying what exactly we don't wanna break.
> >
> > >
> > > Link: https://lore.kernel.org/all/sashiko-outbox-182127@xxxxxxxxxx
> > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > ---
> > >  arch/x86/kvm/regs.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/arch/x86/kvm/regs.c b/arch/x86/kvm/regs.c
> > > index 3621f1a47d37..e53496fdeb82 100644
> > > --- a/arch/x86/kvm/regs.c
> > > +++ b/arch/x86/kvm/regs.c
> > > @@ -738,6 +738,10 @@ int kvm_run_sync_regs_from_user(struct kvm_vcpu *vcpu)
> > >         if (vcpu->run->kvm_dirty_regs & KVM_SYNC_X86_SREGS) {
> > >                 struct kvm_sregs sregs = vcpu->run->s.regs.sregs;
> > >
> > > +               if (is_guest_mode(vcpu) &&
> > > +                   (vcpu->arch.efer & EFER_SVME) && !(sregs.efer & EFER_SVME))
> > > +                       return -EINVAL;
> >
> > Do we need to check (vcpu->arch.efer & EFER_SVME)?
> >
> > If that's not set, we already lost, right?
>
> This is common code, i.e. runs for both VMX and SVM.  The vCPU could be in L2
> via nVMX.  This could check guest_cpu_cap_has(vcpu, X86_FEATURE_SVM), but I worry
> that would be more confusing overall.

Duh, I missed this, sorry for the noise.

> > IIUC, we can't be in the middle of save/restore here. Is this intended
> > to check for the situation where userspace cleared SVME while L2 is
> > running? Or maybe L1 cleared EFER_SVME from under L2 but the shutdown
> > didn't happen yet?
>
> It's intended to prevent syzkaller from abusing KVM_SYNC_X86_SREGS to trip the
> KVM_BUG_ON() I want to add in the next patch.

lol