On 2/24/25 20:22, Kim Phillips wrote:
> On 2/18/25 7:26 PM, Sean Christopherson wrote:
>> When running SEV-SNP guests on a CPU that supports DebugSwap, always save
>> the host's DR0..DR3 mask MSR values irrespective of whether or not
>> DebugSwap is enabled, to ensure the host values aren't clobbered by the
>> CPU.
>>
>> SVM_VMGEXIT_AP_CREATE is deeply flawed in that it allows the *guest* to
>> create a VMSA with guest-controlled SEV_FEATURES. A well behaved guest
>> can inform the hypervisor, i.e. KVM, of its "requested" features, but on
>> CPUs without ALLOWED_SEV_FEATURES support, nothing prevents the guest
>> from
>> lying about which SEV features are being enabled (or not!).
>>
>> If a misbehaving guest enables DebugSwap in a secondary vCPU's VMSA, the
>> CPU will load the DR0..DR3 mask MSRs on #VMEXIT, i.e. will clobber the
>> MSRs with '0' if KVM doesn't save its desired value.
>>
>> Note, DR0..DR3 themselves are "ok", as DR7 is reset on #VMEXIT, and KVM
>> restores all DRs in common x86 code as needed via
>> hw_breakpoint_restore().
>> I.e. there is no risk of host DR0..DR3 being clobbered (when it matters).
>> However, there is a flaw in the opposite direction; because the guest can
>> lie about enabling DebugSwap, i.e. can *disable* DebugSwap without KVM's
>> knowledge, KVM must not rely on the CPU to restore DRs. Defer fixing
>> that wart, as it's more of a documentation issue than a bug in the code.
>>
>> Note, KVM added support for DebugSwap on commit d1f85fbe836e ("KVM: SEV:
>> Enable data breakpoints in SEV-ES"), but that is not an appropriate
>> Fixes,
>> as the underlying flaw exists in hardware, not in KVM. I.e. all kernels
>> that support SEV-SNP need to be patched, not just kernels with KVM's full
>> support for DebugSwap (ignoring that DebugSwap support landed first).
>>
>> Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
>> Cc: stable@xxxxxxxxxxxxxxx
>> Cc: Naveen N Rao <naveen@xxxxxxxxxx>
>> Cc: Kim Phillips <kim.phillips@xxxxxxx>
>> Cc: Tom Lendacky <thomas.lendacky@xxxxxxx>
>> Cc: Alexey Kardashevskiy <aik@xxxxxxx>
>> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
>> ---
>> arch/x86/kvm/svm/sev.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 74525651770a..e3606d072735 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -4568,6 +4568,8 @@ void sev_es_vcpu_reset(struct vcpu_svm *svm)
>> void sev_es_prepare_switch_to_guest(struct vcpu_svm *svm, struct
>> sev_es_save_area *hostsa)
>> {
>> + struct kvm *kvm = svm->vcpu.kvm;
>> +
>> /*
>> * All host state for SEV-ES guests is categorized into three
>> swap types
>> * based on how it is handled by hardware during a world switch:
>> @@ -4592,9 +4594,14 @@ void sev_es_prepare_switch_to_guest(struct
>> vcpu_svm *svm, struct sev_es_save_are
>> /*
>> * If DebugSwap is enabled, debug registers are loaded but NOT
>> saved by
>> * the CPU (Type-B). If DebugSwap is disabled/unsupported, the
>> CPU both
>> - * saves and loads debug registers (Type-A).
>> + * saves and loads debug registers (Type-A). Sadly, on CPUs without
>> + * ALLOWED_SEV_FEATURES, KVM can't prevent SNP guests from enabling
>> + * DebugSwap on secondary vCPUs without KVM's knowledge via "AP
>> Create",
>> + * and so KVM must save DRs if DebugSwap is supported to prevent DRs
>> + * from being clobbered by a misbehaving guest.
>> */
>> - if (sev_vcpu_has_debug_swap(svm)) {
>> + if (sev_vcpu_has_debug_swap(svm) ||
>> + (sev_snp_guest(kvm) &&
>> cpu_feature_enabled(X86_FEATURE_DEBUG_SWAP))) {
>
> Both ALLOWED_SEV_FEATURES and DEBUG_SWAP are also SEV-ES (not only SNP)
> features, so s/sev_snp_guest/sev_es_guest/?
Only SNP can supply a VMSA that may have a different SEV_FEATURES. For
SEV-ES, SEV_FEATURES will have been set by KVM and only KVM.
Thanks,
Tom
>
> Thanks,
>
> Kim
