linux-kernel - Re: [PATCH 02/10] KVM: SVM: Don't rely on DebugSwap to restore host DR0..DR3

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z7zzgtS-iu0YHwia@google.com>
Date: Mon, 24 Feb 2025 14:32:34 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Tom Lendacky <thomas.lendacky@....com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Naveen N Rao <naveen@...nel.org>, Kim Phillips <kim.phillips@....com>, 
	Alexey Kardashevskiy <aik@....com>
Subject: Re: [PATCH 02/10] KVM: SVM: Don't rely on DebugSwap to restore host DR0..DR3

On Mon, Feb 24, 2025, Tom Lendacky wrote:
> On 2/18/25 19:26, Sean Christopherson wrote:
> > Never rely on the CPU to restore/load host DR0..DR3 values, even if the
> > CPU supports DebugSwap, as there are no guarantees that SNP guests will
> > actually enable DebugSwap on APs.  E.g. if KVM were to rely on the CPU to
> > load DR0..DR3 and skipped them during hw_breakpoint_restore(), KVM would
> > run with clobbered-to-zero DRs if an SNP guest created APs without
> > DebugSwap enabled.
> > 
> > Update the comment to explain the dangers, and hopefully prevent breaking
> > KVM in the future.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> 
> See comment below about the Type-A vs Type-B thing, but functionally:
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@....com>
> 
> > ---
> >  arch/x86/kvm/svm/sev.c | 21 ++++++++++++---------
> >  1 file changed, 12 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index e3606d072735..6c6d45e13858 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -4594,18 +4594,21 @@ 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).  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.
> > +	 * saves and loads debug registers (Type-A).  Sadly, KVM can't prevent
> 
> This mention of Type-A was bothering me, so I did some investigation on
> this. If DebugSwap (DebugVirtualization in the latest APM) is
> disabled/unsupported, DR0-3 and DR0-3 Mask registers are left alone and
> the guest sees the host values, they are not fully restored and fully
> saved. When DebugVirtualization is enabled, at that point the registers
> become Type-B.

Good catch.  I completely glossed over that; I'm pretty sure my subconcious simply
rejected that statement as wrong.

> I'm not sure whether it is best to update the comment here or in the
> first patch.

Probably first patch.