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/18/25 19:26, 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>

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.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))) {
>  		hostsa->dr0 = native_get_debugreg(0);
>  		hostsa->dr1 = native_get_debugreg(1);
>  		hostsa->dr2 = native_get_debugreg(2);