Re: [PATCH 01/10] KVM: SVM: Save host DR masks but NOT DRs on CPUs with DebugSwap

[Tom Lendacky <thomas.lendacky@amd.com>]

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@vger.kernel.org
>> Cc: Naveen N Rao <naveen@kernel.org>
>> Cc: Kim Phillips <kim.phillips@amd.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Alexey Kardashevskiy <aik@amd.com>
>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>> ---
>>   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