Re: [PATCH v3 2/2] KVM: SEV: Configure "ALLOWED_SEV_FEATURES" VMCB Field

Sean Christopherson
2025-02-18T17:07:50Z

On Fri, Feb 14, 2025, Kim Phillips wrote:
> On 2/13/25 6:55 PM, Sean Christopherson wrote:
> > On Thu, Feb 13, 2025, Kim Phillips wrote:
> > > > > Not sure if the cpu_feature_enabled() check is necessary, as init should
> > > > > have failed if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES wasn't set in
> > > > > sev_supported_vmsa_features.
> > > >
> > > > Two things missing from this series:
> > > >
> > > > 1: KVM enforcement. No way is KVM going to rely on userspace to opt-in to
> > > > preventing the guest from enabling features.
> > > > 2: Backwards compatilibity if KVM unconditionally enforces ALLOWED_SEV_FEATURES.
> > > > Although maybe there's nothing to do here? I vaguely recall all of the gated
> > > > features being unsupported, or something...
> > >
> > > This contradicts your review comment from the previous version of the series [1].
> >
> > First off, my comment was anything but decisive. I don't see how anyone can read
> > this and come away thinking "this is exactly what Sean wants".
> >
> > This may need additional uAPI so that userspace can opt-in. Dunno. I hope guests
> > aren't abusing features, but IIUC, flipping this on has the potential to break
> > existing VMs, correct?
> >
> > Second, there's a clear question there that went unanswered. Respond to questions
> > and elaborate as needed until we're all on the same page. Don't just send patches.
> >
> > Third, letting userspace opt-in to something doesn't necessarily mean giving
> > userspace full control. Which is the entire reason I asked the question about
> > whether or not this can break userspace. E.g. we can likely get away with only
> > making select features opt-in, and enforcing everything else by default.
> >
> > I don't think RESTRICTED_INJECTION or ALTERNATE_INJECTION can work without KVM
> > cooperation, so enforcing those shouldn't break anything.
> >
> > It's still not clear to me that we don't have a bug with DEBUG_SWAP. AIUI,
> > DEBUG_SWAP is allowed by default. I.e. if ALLOWED_FEATURES is unsupported, then
> > the guest can use DEBUG_SWAP via SVM_VMGEXIT_AP_CREATE without KVM's knowledge.
> >
> > So _maybe_ we have to let userspace opt-in to enforcing DEBUG_SWAP, but I suspect
> > we can get away with fully enabling ALLOWED_FEATURES without userspace's blessing.
>
> If I hardcode DEBUG_SWAP (bit 5) in the vmsa->sev_features assignment
> in wakeup_cpu_via_vmgexit(), such guest boots successfully with the
> kvm_amd module's debug_swap parameter set.
>
> The guest *doesn't* boot if I also turn on allowed_sev_features=1 with
> qemu and this patchseries.
>
> So, the answer is yes, always enforcing ALLOWED_SEV_FEATURES does break
> existing guests, thus the userspace opt-in for it.

That is not an "existing" guest. That's a deliberately misconfigured guest that
serves as testcase/reproducer. IIUC, the BSP can't enable DEBUG_SWAP through a
backdoor, so I don't think it's at all sane/reasonable for the guest to expect
that enabling DEBUG_SWAP only on APs will function. Ah, and KVM will still set
the DR7 intercepts, i.e. the guest can't read/write DR7, so this is definitely a
nonsensical/unsupported configuration.

So unless I've missed something, KVM can unconditionally enforce ALLOWED_SEV_FEATURES.