Re: [PATCH v3 1/2] KVM: x86: Check EFER validity on KVM_SET_SREGS*

[Yosry Ahmed]

On Wed, Jul 29, 2026 at 9:52 AM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
>
> On Tue, Jul 28, 2026 at 9:53 PM Jim Mattson <jmattson@xxxxxxxxxx> wrote:
> >
> > On Mon, Jul 13, 2026 at 11:04 AM Yosry Ahmed <yosry@xxxxxxxxxx> wrote:
> > >
> > > When handling userspace SREGS writes, check the validity of EFER (i.e.
> > > allowed bits) before writing the new value of EFER through the
> > > per-vendor set_efer callbacks. This prevents userspace from writing
> > > bogus values (e.g. EFER.SVME=1 with nested=0).
> > >
> > > Note: on KVM_SET_MSRS, KVM only checks EFER validity in terms of KVM
> > > caps, not guest caps, so it is possible to set EFER bits that are
> > > supported by KVM but not by the guest CPUID. Potentially allowing
> > > userspace to set msrs before CPUID.
> > >
> > > However, for KVM_SET_SREGS*, check the validity of the set bits against
> > > both KVM and guest caps. This is consistent with other validity checks
> > > (e.g. for CR4) that check validity against guest caps, which already
> > > imposes the need to set CPUID before SREGS.
> >
> > Where is the requirement to set CPUID before SREGS documented, aside
> > from this commit message?
>
> I don't think so, and also coming back to this again I think the
> commit message is wrong. The whole basis for doing validity checks
> against guest CPUID (other than the convenience of using
> kvm_valid_efer()) is cr4_guest_rsvd_bits, which is initialized based
> on both KVM caps and guest CPUID.
>
> However, cr4_guest_rsvd_bits seems to be initialized *after* CPUID is
> set, so it only checks CR4 against CPUID if userspace already set
> CPUID. It doesn't impose a restriction to set CPUID before SREGS, but
> this patch is.
>
> So I think this may be too restrictive. We should probably only check
> against KVM caps, which was the whole motivation of this patch to
> begin with (disallowing EFER.SVME if nested=0). Maybe we should just
> drop the Cc:stable as it won't apply to any of the stable trees any
> way, and do this on top of the kvm_caps.supported_efer_bits changes?
>
> diff --git a/arch/x86/kvm/regs.c b/arch/x86/kvm/regs.c
> index 8f66438989e47..412a37ca86ecc 100644
> --- a/arch/x86/kvm/regs.c
> +++ b/arch/x86/kvm/regs.c
> @@ -565,7 +565,7 @@ static bool kvm_is_valid_sregs(struct kvm_vcpu
> *vcpu, struct kvm_sregs *sregs)
>
>         return kvm_is_valid_cr4(vcpu, sregs->cr4) &&
>                kvm_is_valid_cr0(vcpu, sregs->cr0) &&
> -              kvm_valid_efer(vcpu, sregs->efer);
> +              !(sregs->efer & ~kvm_caps.supported_efer_bits);
>  }
>
>  static int __set_sregs_common(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs,
>
> ---
>
> diff --git a/tools/testing/selftests/kvm/x86/set_sregs_test.c
> b/tools/testing/selftests/kvm/x86/set_sregs_test.c
> index 603226ffe4374..c07ccf0bbe85a 100644
> --- a/tools/testing/selftests/kvm/x86/set_sregs_test.c
> +++ b/tools/testing/selftests/kvm/x86/set_sregs_test.c
> @@ -171,7 +171,6 @@ int main(int argc, char *argv[])
>          */
>         vm = vm_create_barebones();
>         vcpu = __vm_vcpu_add(vm, 0);
> -       test_efer_bits(vcpu, KVM_ALWAYS_ALLOWED_EFER);
>         test_cr_bits(vcpu, KVM_ALWAYS_ALLOWED_CR4);
>         kvm_vm_free(vm);
>
> ---
>
> Sean, WDYT?
>
> If we will change this, would you rather do it in place, a followup
> fix, or a new version?

I take this back, I think I was right the first time.
kvm_vcpu_after_set_cpuid() is called on vCPU creation (confusing?),
and looking closely at set_sregs_test seems like it specifically
verifies that CR4 bits guarded by CPUID bits cannot be set before
CPUID is set.

I think the main difference here is probably that CR4 bits that are
guarded by CPUID are more "advanced" than EFER bits?