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

[Sean Christopherson <seanjc@google.com>]

On Tue, Oct 27, 2026, Yosry Ahmed wrote:
> On Tue, Oct 27, 2026 at 3:49 PM Sean Christopherson <seanjc@google.com> 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).

> 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@kernel.org
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  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.

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

> I suppose it makes sense for the sync to only fail if it's actually
> changing something. Mostly just thinking out loud.