linux-kernel - Re: [PATCH v4 2/4] KVM: x86: Drop kvm_x86_ops.set_dr6() in favor of a new KVM

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: <aCc4gyb_hrQZs9Ya@google.com>
Date: Fri, 16 May 2025 06:07:15 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Chao Gao <chao.gao@...el.com>
Cc: Maxim Levitsky <mlevitsk@...hat.com>, kvm@...r.kernel.org, 
	Thomas Gleixner <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>, x86@...nel.org, 
	Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org, 
	"H. Peter Anvin" <hpa@...or.com>, Paolo Bonzini <pbonzini@...hat.com>, 
	Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH v4 2/4] KVM: x86: Drop kvm_x86_ops.set_dr6() in favor of a
 new KVM_RUN flag

On Fri, May 16, 2025, Chao Gao wrote:
> >diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> >index c8b8a9947057..026b28051fff 100644
> >--- a/arch/x86/kvm/svm/svm.c
> >+++ b/arch/x86/kvm/svm/svm.c
> >@@ -4308,10 +4308,13 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
> > 	svm_hv_update_vp_id(svm->vmcb, vcpu);
> > 
> > 	/*
> >-	 * Run with all-zero DR6 unless needed, so that we can get the exact cause
> >-	 * of a #DB.
> >+	 * Run with all-zero DR6 unless the guest can write DR6 freely, so that
> >+	 * KVM can get the exact cause of a #DB.  Note, loading guest DR6 from
> >+	 * KVM's snapshot is only necessary when DR accesses won't exit.
> > 	 */
> >-	if (likely(!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)))
> >+	if (unlikely(run_flags & KVM_RUN_LOAD_GUEST_DR6))
> >+		svm_set_dr6(vcpu, vcpu->arch.dr6);
> >+	else if (likely(!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)))
> > 		svm_set_dr6(vcpu, DR6_ACTIVE_LOW);
> 
> ...
> 
> > void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
> > {
> > 	vmcs_writel(GUEST_DR7, val);
> >@@ -7371,6 +7365,9 @@ fastpath_t vmx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
> > 		vmcs_writel(GUEST_RIP, vcpu->arch.regs[VCPU_REGS_RIP]);
> > 	vcpu->arch.regs_dirty = 0;
> > 
> >+	if (run_flags & KVM_RUN_LOAD_GUEST_DR6)
> >+		set_debugreg(vcpu->arch.dr6, 6);
> >+
> > 	/*
> > 	 * Refresh vmcs.HOST_CR3 if necessary.  This must be done immediately
> > 	 * prior to VM-Enter, as the kernel may load a new ASID (PCID) any time
> >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> >index 25de78cdab42..684b8047e0f2 100644
> >--- a/arch/x86/kvm/x86.c
> >+++ b/arch/x86/kvm/x86.c
> >@@ -11019,7 +11019,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
> > 		set_debugreg(vcpu->arch.eff_db[3], 3);
> > 		/* When KVM_DEBUGREG_WONT_EXIT, dr6 is accessible in guest. */
> > 		if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT))
> >-			kvm_x86_call(set_dr6)(vcpu, vcpu->arch.dr6);
> >+			run_flags |= KVM_RUN_LOAD_GUEST_DR6;
> 
> The new KVM_RUN flag isn't necessary. Vendor code can directly check
> switch_db_regs to decide whether to load the guest dr6 into hardware.

So thought Paolo, too.

> In svm_vcpu_run(), referencing both run_flags and switch_db_regs is confusing.
> The following logic is much clearer:
> 
> 	if (likely(!(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT)))
>  		svm_set_dr6(vcpu, DR6_ACTIVE_LOW);
> 	else
> 		svm_set_dr6(vcpu, vcpu->arch.dr6);

And wrong :-)  vcpu->arch.dr6 is stale on fastpasth reentry, see commit c2fee09fc167
("KVM: x86: Load DR6 with guest value only before entering .vcpu_run() loop").