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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3a1b6e1c-5a74-4c9d-81d2-7f9a34f58042@amd.com>
Date: Mon, 24 Feb 2025 20:22:19 -0600
From: Kim Phillips <kim.phillips@....com>
To: Sean Christopherson <seanjc@...gle.com>,
 Paolo Bonzini <pbonzini@...hat.com>
Cc: kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 Naveen N Rao <naveen@...nel.org>, Tom Lendacky <thomas.lendacky@....com>,
 Alexey Kardashevskiy <aik@....com>
Subject: Re: [PATCH 01/10] KVM: SVM: Save host DR masks but NOT DRs on CPUs
 with DebugSwap

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

Thanks,

Kim