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@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>
Reviewed-by: Tom Lendacky <thomas.lendacky@xxxxxxx>
> ---
> 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);
