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

[Sean Christopherson <seanjc@google.com>]

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.