Re: [PATCH v13 02/12] x86/hw_breakpoints: Make DR7 updates NMI safe

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

On Sat, 22 Aug 2026 09:41:06 +0000
sashiko-bot@kernel.org wrote:

> > -static __always_inline void local_db_restore(unsigned long dr7)
> > +static __always_inline void local_db_restore(unsigned long dr7,
> > +					     unsigned int dr7_seq)
> >  {
> > +	unsigned int seq;
> > +
> >  	/*
> >  	 * Ensure the compiler doesn't raise this statement into
> >  	 * the critical section; enabling breakpoints early would
> >  	 * not be good.
> >  	 */
> >  	barrier();
> > -	if (dr7)
> > +
> > +	do {
> > +		seq = this_cpu_read(cpu_dr7_seq);
> > +		if (seq == dr7_seq) {
> > +			if (!dr7)
> > +				return;
> > +		} else {
> > +			dr7 = this_cpu_read(cpu_dr7);
> > +			if (!dr7)
> > +				dr7 = DR7_FIXED_1;
> > +		}
> > +
> >  		set_debugreg(dr7, 7);
> > +		barrier();
> > +	} while (unlikely(seq != this_cpu_read(cpu_dr7_seq)));
> >  }
> 
> [Severity: High]
> If KVM intentionally clears the hardware DR7 state to isolate the guest from
> host breakpoints before VMENTER without modifying the host's cpu_dr7 shadow
> state:
> 
> arch/x86/kvm/x86.c:vcpu_enter_guest() {
>     ...
>     if (unlikely(vcpu->arch.switch_db_regs &&
>                  !(vcpu->arch.switch_db_regs & KVM_DEBUGREG_AUTO_SWITCH))) {
>         set_debugreg(DR7_FIXED_1, 7);
>     ...
>     } else if (unlikely(hw_breakpoint_active())) {
>         set_debugreg(DR7_FIXED_1, 7);
>     }
>     ...
> }
> 
> Could an NMI firing shortly after this permanently undo this isolation
> upon return?
> 
> If an NMI (such as a perf event) fires before vmlaunch/vmresume, exc_nmi
> calls local_db_save, saving dr7=0. If the NMI modifies a breakpoint,
> cpu_dr7_seq increments.
> 
> When exc_nmi calls local_db_restore, the sequence mismatch causes it to
> discard the saved dr7=0 state and instead unconditionally restore the host's
> active breakpoints (this_cpu_read(cpu_dr7)) to the hardware DR7. This could
> leak host hardware breakpoints into a KVM guest and cause unexpected #DB
> exceptions.

OK, this seems only happen when KGDB/KDB sets the watchpoint from
NMI context (right after KVM disables dr7 for transient to guest).
We can avoid this by early returning if dr7 == 0 in local_db_restore().
(But..., if user intensively set the dr7 in that timing, it will be
ignored.)

> 
> [ ... ]
> > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
> > index 9bba1e2f03af7..8dba9cd04bfa5 100644
> > --- a/arch/x86/kernel/cpu/mce/core.c
> > +++ b/arch/x86/kernel/cpu/mce/core.c
> [ ... ]
> > @@ -2139,20 +2139,22 @@ static __always_inline void exc_machine_check_user(struct pt_regs *regs)
> >  DEFINE_IDTENTRY_MCE(exc_machine_check)
> >  {
> >  	unsigned long dr7;
> > +	unsigned int dr7_seq;
> >  
> > -	dr7 = local_db_save();
> > +	local_db_save(&dr7, &dr7_seq);
> >  	exc_machine_check_kernel(regs);
> > -	local_db_restore(dr7);
> > +	local_db_restore(dr7, dr7_seq);
> >  }
> >  
> >  /* The user mode variant. */
> >  DEFINE_IDTENTRY_MCE_USER(exc_machine_check)
> >  {
> >  	unsigned long dr7;
> > +	unsigned int dr7_seq;
> >  
> > -	dr7 = local_db_save();
> > +	local_db_save(&dr7, &dr7_seq);
> >  	exc_machine_check_user(regs);
> > -	local_db_restore(dr7);
> > +	local_db_restore(dr7, dr7_seq);
> >  }
> 
> [Severity: High]
> This is a pre-existing issue, but does task migration during user-mode
> machine check exceptions corrupt hardware debug registers on the new CPU?
> 
> exc_machine_check_user invokes irqentry_exit_to_user_mode, which processes
> pending thread work (like TIF_NEED_RESCHED), enabling interrupts and allowing
> the task to be scheduled out and migrate to a new CPU:
> 
> CPU1
> exc_machine_check()
>     local_db_save(&dr7, &dr7_seq);
>     exc_machine_check_user(regs)
>         irqentry_exit_to_user_mode(regs)
>             // Task migrates to CPU2
> 
> CPU2
>     local_db_restore(dr7, dr7_seq);
> 
> When it resumes on the new CPU, local_db_restore is executed with the dr7
> and dr7_seq values saved from the old CPU. If the new CPU's cpu_dr7_seq
> matches the old CPU's saved dr7_seq, it would erroneously write the old CPU's
> hardware DR7 state to the new CPU's hardware DR7 register. Could this cause
> spurious #DB exceptions or break hardware breakpoints on the new CPU?

OK, this is another problem to be fixed in a bugfix patch. We should do
local_db_save/restore() inside exc_machine_check_user() before calling
irqentry_exit_to_user_mode().

Thank you,

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