Re: [PATCH v16 02/13] KVM: x86: Prevent host DR7 debug register leak into guest OS on NMI

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

Hi Sean,

Thanks for your review!

On Mon, 14 Sep 2026 07:35:48 -0700
Sean Christopherson <seanjc@google.com> wrote:

> On Mon, Sep 14, 2026, Masami Hiramatsu (Google) wrote:
> > From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > 
> > When KVM enters a guest OS, host hardware breakpoints are disabled
> > before running the guest. However, an NMI can occur during guest
> > execution, where local_db_save() or arch_install_hw_breakpoint()
> > can be invoked.
> > 
> > In particular, if local_db_save() or arch_install_hw_breakpoint()
> > is executed from NMI, hardware DR7 can be modified or restored with
> > host breakpoint settings, leaking host breakpoints into the guest OS
> > or clobbering the guest's debug registers.
> 
> Not for local_db_save(), at least not AFAICT.  On VM-Exit, both Intel and AMD
> purge DR7, i.e. load 0x400, so local_db_save() => local_db_restore() is more or
> less a nop.  Even if that weren't the case, actually saving/restoring DR7 would
> be the right thing to do, in any context.

OK.

> 
> > Introduce a per-CPU flag, cpu_dr_in_guest, to indicate that the CPU
> > is executing in guest mode. Set this flag in vcpu_enter_guest()
> > during entering the guest with disabling host breakpoints.
> > If this flag is set, local_db_save() and local_db_restore() return
> > immediately, and arch_install_hw_breakpoint() returns an error.
> > In addition, protect cpu_dr_in_guest in within_cpu_entry() to
> > prevent recursive #DB exceptions.
> > 
> > Fixes: f85d40160691 ("KVM: X86: Disable hardware breakpoints unconditionally before kvm_x86->run()")
> > Assisted-by: Antigravity:gemini-3.8-flash
> > Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> > ---
> > Changes in v16:
> >  - Newly added.
> > ---
> >  arch/x86/include/asm/debugreg.h |    6 ++++++
> >  arch/x86/kernel/hw_breakpoint.c |   10 ++++++++++
> >  arch/x86/kvm/x86.c              |    7 +++++++
> >  3 files changed, 23 insertions(+)
> > 
> > diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
> > index 854d82b88ff4..50f830972698 100644
> > --- a/arch/x86/include/asm/debugreg.h
> > +++ b/arch/x86/include/asm/debugreg.h
> > @@ -18,6 +18,7 @@
> >  #define DR7_FIXED_1	0x00000400
> >  
> >  DECLARE_PER_CPU(unsigned long, cpu_dr7);
> > +DECLARE_PER_CPU(bool, cpu_dr_in_guest);
> >  
> >  #ifndef CONFIG_PARAVIRT_XXL
> >  /*
> > @@ -129,6 +130,9 @@ static __always_inline unsigned long local_db_save(void)
> >  {
> >  	unsigned long dr7;
> >  
> > +	if (this_cpu_read(cpu_dr_in_guest))
> > +		return 0;
> 
> This is broken.  If an NMI hits between KVM writing cpu_dr_in_guest and clearing
> DR7, and there are active breakpoints, then local_db_save() won't disable breakpoints
> as it should, and the relevant code in exc_nmi() will run with breakpoints enabled.
> 
> 	kvm_load_xfeatures(vcpu, true);
> 
> 	this_cpu_write(cpu_dr_in_guest, true);
> 	barrier();
> 
>   <NMI here is problematic>
> 
> 	if (unlikely(vcpu->arch.switch_db_regs &&
> 		     !(vcpu->arch.switch_db_regs & KVM_DEBUGREG_AUTO_SWITCH))) {
> 		set_debugreg(DR7_FIXED_1, 7);
> 		set_debugreg(vcpu->arch.eff_db[0], 0);

Oops, indeed! OK, so local_db_save/restore will just work as it is,
but modifying DR7 should directly be prohibited.

> 
> > +
> >  	if (cpu_feature_enabled(X86_FEATURE_HYPERVISOR) && !hw_breakpoint_active())
> >  		return 0;
> >  
> > @@ -157,6 +161,8 @@ static __always_inline void local_db_restore(unsigned long dr7)
> >  	 * not be good.
> >  	 */
> >  	barrier();
> > +	if (this_cpu_read(cpu_dr_in_guest))
> > +		return;
> >  	if (dr7)
> >  		set_debugreg(dr7, 7);
> >  }
> > diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> > index f846c15f21ca..68de7ed79d88 100644
> > --- a/arch/x86/kernel/hw_breakpoint.c
> > +++ b/arch/x86/kernel/hw_breakpoint.c
> > @@ -40,6 +40,9 @@
> >  DEFINE_PER_CPU(unsigned long, cpu_dr7);
> >  EXPORT_PER_CPU_SYMBOL(cpu_dr7);
> >  
> > +DEFINE_PER_CPU(bool, cpu_dr_in_guest);
> > +EXPORT_PER_CPU_SYMBOL_GPL(cpu_dr_in_guest);
> > +
> >  /* Per cpu debug address registers values */
> >  static DEFINE_PER_CPU(unsigned long, cpu_debugreg[HBP_NUM]);
> >  
> > @@ -102,6 +105,9 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
> >  
> >  	lockdep_assert_irqs_disabled();
> >  
> > +	if (this_cpu_read(cpu_dr_in_guest))
> > +		return -EBUSY;
> 
> If we decide this is how to fix arch_install_hw_breakpoint() clobbering DRs from
> NMI context, I would rather have more generic flag to tell perf that KVM is about
> to enter the guest, e.g. so that we don't have to separately solve the same problem
> for other perf events:
> 
> https://lore.kernel.org/all/3585d823-00f3-46ae-a799-b62a95743e76@linux.intel.com

OK, so introducing a new (someting like) cpu_in_trans_guest flag and check it
from all affected places?

Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>