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

[Kim Phillips <kim.phillips@amd.com>]

On 2/13/25 6:55 PM, Sean Christopherson wrote:
> On Thu, Feb 13, 2025, Kim Phillips wrote:
>> On 2/11/25 3:46 PM, Sean Christopherson wrote:
>>> On Mon, Feb 10, 2025, Tom Lendacky wrote:
>>>> On 2/7/25 17:34, Kim Phillips wrote:
>>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>>> index a2a794c32050..a9e16792cac0 100644
>>>>> --- a/arch/x86/kvm/svm/sev.c
>>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>>> @@ -894,9 +894,19 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>>>>>    	return 0;
>>>>>    }
>>>>> +static u64 allowed_sev_features(struct kvm_sev_info *sev)
>>>>> +{
>>>>> +	if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES) &&
>>>>
>>>> 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.

Thanks,

Kim

> 
>> If KVM enforces ALLOWED_SEV_FEATURES, it can break existing VMs, thus
>> the explicit userspace allowed-sev-features=on opt-in [2].
>>
>> Thanks,
>>
>> Kim
>>
>> [1] https://lore.kernel.org/kvm/ZsfKYHFkWA-Rh23C@google.com/
>> [2] https://lore.kernel.org/kvm/20250207233327.130770-1-kim.phillips@amd.com/