| Message ID | 20250207233410.130813-3-kim.phillips@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: SEV: Add support for the ALLOWED_SEV_FEATURES feature | expand |
On 2/7/25 17:34, Kim Phillips wrote: > AMD EPYC 5th generation processors have introduced a feature that allows > the hypervisor to control the SEV_FEATURES that are set for, or by, a > guest [1]. ALLOWED_SEV_FEATURES can be used by the hypervisor to enforce > that SEV-ES and SEV-SNP guests cannot enable features that the > hypervisor does not want to be enabled. > > When ALLOWED_SEV_FEATURES is enabled, a VMRUN will fail if any > non-reserved bits are 1 in SEV_FEATURES but are 0 in > ALLOWED_SEV_FEATURES. > > Some SEV_FEATURES - currently PmcVirtualization and SecureAvic > (see Appendix B, Table B-4) - require an opt-in via ALLOWED_SEV_FEATURES, > i.e. are off-by-default, whereas all other features are effectively > on-by-default, but still honor ALLOWED_SEV_FEATURES. > > [1] Section 15.36.20 "Allowed SEV Features", AMD64 Architecture > Programmer's Manual, Pub. 24593 Rev. 3.42 - March 2024: > https://bugzilla.kernel.org/attachment.cgi?id=306250 > > Co-developed-by: Kishon Vijay Abraham I <kvijayab@amd.com> > Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com> > Signed-off-by: Kim Phillips <kim.phillips@amd.com> > --- > arch/x86/include/asm/svm.h | 5 ++++- > arch/x86/kvm/svm/sev.c | 17 +++++++++++++++++ > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h > index e2fac21471f5..6d94a727cc1a 100644 > --- a/arch/x86/include/asm/svm.h > +++ b/arch/x86/include/asm/svm.h > @@ -158,7 +158,9 @@ struct __attribute__ ((__packed__)) vmcb_control_area { > u64 avic_physical_id; /* Offset 0xf8 */ > u8 reserved_7[8]; > u64 vmsa_pa; /* Used for an SEV-ES guest */ > - u8 reserved_8[720]; > + u8 reserved_8[40]; > + u64 allowed_sev_features; /* Offset 0x138 */ > + u8 reserved_9[672]; > /* > * Offset 0x3e0, 32 bytes reserved > * for use by hypervisor/software. > @@ -289,6 +291,7 @@ static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_ > #define SVM_SEV_FEAT_RESTRICTED_INJECTION BIT(3) > #define SVM_SEV_FEAT_ALTERNATE_INJECTION BIT(4) > #define SVM_SEV_FEAT_DEBUG_SWAP BIT(5) > +#define SVM_SEV_FEAT_ALLOWED_SEV_FEATURES BIT_ULL(63) Hmmm... I believe it is safe to define this bit value, as the Allowed SEV features VMCB field shows bits 61:0 being used for the allowed features mask and we know that the SEV_FEATURES field is used in the SEV Features MSR left-shifted 2 bits, so we only expect bits 61:0 to be used and bits 62 and 63 will always be reserved. But, given that I think we need two functions: - get_allowed_sev_features() keeping it as you have it below, where it returns the sev->vmsa_features bitmap if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES is set or 0 if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES is not set. - get_vmsa_sev_features() which removes the SVM_SEV_FEAT_ALLOWED_SEV_FEATURES bit, since it is not defined in the VMSA SEV_FEATURES definition. > > #define SVM_SEV_FEAT_INT_INJ_MODES \ > (SVM_SEV_FEAT_RESTRICTED_INJECTION | \ > 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. Thanks, Tom > + (sev->vmsa_features & SVM_SEV_FEAT_ALLOWED_SEV_FEATURES)) > + return sev->vmsa_features; > + > + return 0; > +} > + > static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu, > int *error) > { > + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; > struct sev_data_launch_update_vmsa vmsa; > struct vcpu_svm *svm = to_svm(vcpu); > int ret; > @@ -906,6 +916,8 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu, > return -EINVAL; > } > > + svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev); > + > /* Perform some pre-encryption checks against the VMSA */ > ret = sev_es_sync_vmsa(svm); > if (ret) > @@ -2447,6 +2459,8 @@ static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp) > struct vcpu_svm *svm = to_svm(vcpu); > u64 pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT; > > + svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev); > + > ret = sev_es_sync_vmsa(svm); > if (ret) > return ret; > @@ -3069,6 +3083,9 @@ void __init sev_hardware_setup(void) > sev_supported_vmsa_features = 0; > if (sev_es_debug_swap_enabled) > sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP; > + > + if (sev_es_enabled && cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES)) > + sev_supported_vmsa_features |= SVM_SEV_FEAT_ALLOWED_SEV_FEATURES; > } > > void sev_hardware_unsetup(void)
On Mon, Feb 10, 2025, Tom Lendacky wrote: > On 2/7/25 17:34, Kim Phillips wrote: > > @@ -289,6 +291,7 @@ static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_ > > #define SVM_SEV_FEAT_RESTRICTED_INJECTION BIT(3) > > #define SVM_SEV_FEAT_ALTERNATE_INJECTION BIT(4) > > #define SVM_SEV_FEAT_DEBUG_SWAP BIT(5) > > +#define SVM_SEV_FEAT_ALLOWED_SEV_FEATURES BIT_ULL(63) > > Hmmm... I believe it is safe to define this bit value, as the Allowed > SEV features VMCB field shows bits 61:0 being used for the allowed > features mask and we know that the SEV_FEATURES field is used in the SEV > Features MSR left-shifted 2 bits, so we only expect bits 61:0 to be used > and bits 62 and 63 will always be reserved. But, given that I think we > need two functions: > > - get_allowed_sev_features() > keeping it as you have it below, where it returns the > sev->vmsa_features bitmap if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES is set > or 0 if SVM_SEV_FEAT_ALLOWED_SEV_FEATURES is not set. > > - get_vmsa_sev_features() > which removes the SVM_SEV_FEAT_ALLOWED_SEV_FEATURES bit, since it is > not defined in the VMSA SEV_FEATURES definition. Or just don't add wrappers that do more harm than good? diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index a9e16792cac0..4d0b5a020b65 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -894,15 +894,6 @@ 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) && - (sev->vmsa_features & SVM_SEV_FEAT_ALLOWED_SEV_FEATURES)) - return sev->vmsa_features; - - return 0; -} - static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu, int *error) { @@ -916,7 +907,8 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu, return -EINVAL; } - svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev); + if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES)) + svm->vmcb->control.allowed_sev_features = sev->vmsa_features; /* Perform some pre-encryption checks against the VMSA */ ret = sev_es_sync_vmsa(svm); @@ -2459,7 +2451,8 @@ static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp) struct vcpu_svm *svm = to_svm(vcpu); u64 pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT; - svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev); + if (cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES)) + svm->vmcb->control.allowed_sev_features = sev->vmsa_features; ret = sev_es_sync_vmsa(svm); if (ret) > > #define SVM_SEV_FEAT_INT_INJ_MODES \ > > (SVM_SEV_FEAT_RESTRICTED_INJECTION | \ > > 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...
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]. 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/
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 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/
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/
On Thu, Feb 13, 2025 at 04:55:13PM -0800, 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: > > 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. In sev_es_prepare_switch_to_guest(), we save host debug register state (DR0-DR3) only if KVM is aware of DEBUG_SWAP being enabled in the guest (via vmsa_features). So, from what I can tell, it looks like the guest will end up overwriting host state if it enables DEBUG_SWAP without KVM's knowledge? Not sure if that's reason enough to enforce ALLOWED_SEV_FEATURES for DEBUG_SWAP :) If ALLOWED_SEV_FEATURES is not supported, we may still have to unconditionally save the host DR0-DR3 registers. - Naveen
On Mon, Feb 17, 2025, Naveen N Rao wrote: > On Thu, Feb 13, 2025 at 04:55:13PM -0800, 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: > > > > 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. > > In sev_es_prepare_switch_to_guest(), we save host debug register state > (DR0-DR3) only if KVM is aware of DEBUG_SWAP being enabled in the guest > (via vmsa_features). So, from what I can tell, it looks like the guest > will end up overwriting host state if it enables DEBUG_SWAP without > KVM's knowledge? Yes, that's what I'm effectively "asking". > Not sure if that's reason enough to enforce ALLOWED_SEV_FEATURES for > DEBUG_SWAP :) > > If ALLOWED_SEV_FEATURES is not supported, we may still have to > unconditionally save the host DR0-DR3 registers. Yes, that's my understanding of the situation. If the CPU supports DEBUG_SWAP, KVM must assume the guest can enable it without KVM's knowledge.
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.
On Mon, Feb 17, 2025, Naveen N Rao wrote: > On Thu, Feb 13, 2025 at 04:55:13PM -0800, 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: > > > > 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. > > In sev_es_prepare_switch_to_guest(), we save host debug register state > (DR0-DR3) only if KVM is aware of DEBUG_SWAP being enabled in the guest > (via vmsa_features). So, from what I can tell, it looks like the guest > will end up overwriting host state if it enables DEBUG_SWAP without > KVM's knowledge? > > Not sure if that's reason enough to enforce ALLOWED_SEV_FEATURES for > DEBUG_SWAP :) > > If ALLOWED_SEV_FEATURES is not supported, we may still have to > unconditionally save the host DR0-DR3 registers. Aha! We do not. The existing KVM code is actually flawed in the opposite direction, in that saving host DR0..DR3 during sev_es_prepare_switch_to_guest() is superfluous. DR7 is reset on #VMEXIT (swap type "C"), and so KVM only needs to ensure DR0..DR3 are restored before loading DR7 with the host's value. KVM takes care of that in common x86 code via hw_breakpoint_restore(). However, there is still a bug, as the AMD-specific *masks* are not restored. KVM doesn't support MSR_F16H_DRx_ADDR_MASK emulation or passthrough for normal guests, so the guest can't set those values either, i.e. will get a #VC. But the masks do need to be restored, because the CPU will clobber them with '0' when DebugSwap is enabled. And of course the DR0..DR3 loads in sev_es_prepare_switch_to_guest() are a complete waste of cycles. *sigh* Ugh, it gets worse. The awfulness goes in both direction. Unless I've misunderstood how this all works, just because *KVM* enables DebugSwap for the BSP doesn't mean the guest will enable DebugSwap for APs. And so KVM _can't_ rely on DebugSwap to actually swap DR0..DR3, because KVM has no way of knowing whether or not a given vCPU actually has it enabled. And that's true even when SEV_ALLOWED_FEATURES comes along, which means treating DR0..DR3 as swap type B is utterly worthless. What a mess. I'll send a small series to try and clean things up. Also, I told y'all so: https://lore.kernel.org/all/YWnbfCet84Vup6q9@google.com P.S. Can someone please get the APM updated to actually explain WTF enabling Debug Virtualization in SEV_FEATURES does? The APM does not at all match what y'all have told me. A sane reading of the APM would be that DRs are *unconditionally* swap type B when DebugSwap is supported, whereas I've been told from the very beginning[*] that treating them as type B requires software enabling: AMD Milan introduces support for the swapping, as type 'B', of DR[0-3] and DR[0-3]_ADDR_MASK registers. It requires that SEV_FEATURES[5] be set in the VMSA. [*] https://lore.kernel.org/all/20221201021948.9259-3-aik@amd.com
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h index e2fac21471f5..6d94a727cc1a 100644 --- a/arch/x86/include/asm/svm.h +++ b/arch/x86/include/asm/svm.h @@ -158,7 +158,9 @@ struct __attribute__ ((__packed__)) vmcb_control_area { u64 avic_physical_id; /* Offset 0xf8 */ u8 reserved_7[8]; u64 vmsa_pa; /* Used for an SEV-ES guest */ - u8 reserved_8[720]; + u8 reserved_8[40]; + u64 allowed_sev_features; /* Offset 0x138 */ + u8 reserved_9[672]; /* * Offset 0x3e0, 32 bytes reserved * for use by hypervisor/software. @@ -289,6 +291,7 @@ static_assert((X2AVIC_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AVIC_ #define SVM_SEV_FEAT_RESTRICTED_INJECTION BIT(3) #define SVM_SEV_FEAT_ALTERNATE_INJECTION BIT(4) #define SVM_SEV_FEAT_DEBUG_SWAP BIT(5) +#define SVM_SEV_FEAT_ALLOWED_SEV_FEATURES BIT_ULL(63) #define SVM_SEV_FEAT_INT_INJ_MODES \ (SVM_SEV_FEAT_RESTRICTED_INJECTION | \ 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) && + (sev->vmsa_features & SVM_SEV_FEAT_ALLOWED_SEV_FEATURES)) + return sev->vmsa_features; + + return 0; +} + static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu, int *error) { + struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info; struct sev_data_launch_update_vmsa vmsa; struct vcpu_svm *svm = to_svm(vcpu); int ret; @@ -906,6 +916,8 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu, return -EINVAL; } + svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev); + /* Perform some pre-encryption checks against the VMSA */ ret = sev_es_sync_vmsa(svm); if (ret) @@ -2447,6 +2459,8 @@ static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp) struct vcpu_svm *svm = to_svm(vcpu); u64 pfn = __pa(svm->sev_es.vmsa) >> PAGE_SHIFT; + svm->vmcb->control.allowed_sev_features = allowed_sev_features(sev); + ret = sev_es_sync_vmsa(svm); if (ret) return ret; @@ -3069,6 +3083,9 @@ void __init sev_hardware_setup(void) sev_supported_vmsa_features = 0; if (sev_es_debug_swap_enabled) sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP; + + if (sev_es_enabled && cpu_feature_enabled(X86_FEATURE_ALLOWED_SEV_FEATURES)) + sev_supported_vmsa_features |= SVM_SEV_FEAT_ALLOWED_SEV_FEATURES; } void sev_hardware_unsetup(void)
AMD EPYC 5th generation processors have introduced a feature that allows the hypervisor to control the SEV_FEATURES that are set for, or by, a guest [1]. ALLOWED_SEV_FEATURES can be used by the hypervisor to enforce that SEV-ES and SEV-SNP guests cannot enable features that the hypervisor does not want to be enabled. When ALLOWED_SEV_FEATURES is enabled, a VMRUN will fail if any non-reserved bits are 1 in SEV_FEATURES but are 0 in ALLOWED_SEV_FEATURES. Some SEV_FEATURES - currently PmcVirtualization and SecureAvic (see Appendix B, Table B-4) - require an opt-in via ALLOWED_SEV_FEATURES, i.e. are off-by-default, whereas all other features are effectively on-by-default, but still honor ALLOWED_SEV_FEATURES. [1] Section 15.36.20 "Allowed SEV Features", AMD64 Architecture Programmer's Manual, Pub. 24593 Rev. 3.42 - March 2024: https://bugzilla.kernel.org/attachment.cgi?id=306250 Co-developed-by: Kishon Vijay Abraham I <kvijayab@amd.com> Signed-off-by: Kishon Vijay Abraham I <kvijayab@amd.com> Signed-off-by: Kim Phillips <kim.phillips@amd.com> --- arch/x86/include/asm/svm.h | 5 ++++- arch/x86/kvm/svm/sev.c | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-)