On Sun, 30 Aug 2026 14:45:19 +0000
[email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
> to consider:
>
> Pre-existing issues:
> - [Critical] Preexisting race condition in `local_db_save()` leads to
> recursive exceptions. The function reads DR7, then checks if it was non-zero
> before disabling hardware breakpoints. If an NMI (e.g., KGDB) installs a
> breakpoint after DR7 is read but before the check, `local_db_save()` will
> falsely believe no breakpoints are active and skip disabling them. The
> exception handler (e.g., `#DB` or `#MC`) will then run with hardware
> breakpoints enabled, risking a recursive exception.
> - [High] The patch adds `if (!dr7) return;` in `local_db_restore()` to
> ostensibly prevent an NMI from overwriting an intentionally cleared hardware
> DR7 state (such as during KVM guest entry). However, if an NMI (e.g., from
> KGDB) installs a hardware breakpoint, `arch_install_hw_breakpoint()` directly
> overwrites the hardware DR7 register with `cpu_dr7`. By returning immediately
> when `dr7 == 0`, `local_db_restore()` fails to restore the hardware DR7 back
> to its cleared state (0x400). Consequently, KVM resumes the guest with the
> host's newly installed hardware breakpoint active.
> --
>
> commit e145a86b764e63c7282dc5c5fb7c745db147953a
> Author: Jinchao Wang <[email protected]>
>
> x86/hw_breakpoints: Make DR7 updates NMI safe
>
> Hardware breakpoint installation and removal run with IRQs disabled, but
> an NMI can still enter the same code through KGDB. This patch uses cmpxchg
> and a per-CPU sequence number to make DR7 updates safe from NMI interruptions.
>
> Link:
> https://lore.kernel.org/all/4ee0a2efc9e8387af83286b8495b7d490247e165.1785067572.git.wangjinchao...@gmail.com/
>
> > diff --git a/arch/x86/include/asm/debugreg.h
> > b/arch/x86/include/asm/debugreg.h
> > index a2c1f2d24b645..65e80cd62193f 100644
> > --- a/arch/x86/include/asm/debugreg.h
> > +++ b/arch/x86/include/asm/debugreg.h
>
> [ ... ]
>
> > @@ -125,18 +126,20 @@ static __always_inline bool hw_breakpoint_active(void)
> >
> > extern void hw_breakpoint_restore(void);
> >
> > -static __always_inline unsigned long local_db_save(void)
> > +static __always_inline void local_db_save(unsigned long *dr7,
> > + unsigned int *dr7_seq)
> > {
> > - unsigned long dr7;
> > + *dr7 = 0;
> > + *dr7_seq = this_cpu_read(cpu_dr7_seq);
> >
> > if (static_cpu_has(X86_FEATURE_HYPERVISOR) && !hw_breakpoint_active())
> > - return 0;
> > + return;
> >
> > - get_debugreg(dr7, 7);
> > + get_debugreg(*dr7, 7);
> >
> > /* Architecturally set bit */
> > - dr7 &= ~DR7_FIXED_1;
> > - if (dr7)
> > + *dr7 &= ~DR7_FIXED_1;
> > + if (*dr7)
> > set_debugreg(DR7_FIXED_1, 7);
>
> [Severity: Critical]
> This is a pre-existing issue, but does this conditional check rely on a stale
> read of DR7?
>
> If an NMI (e.g. KGDB) installs a breakpoint after DR7 is read via
> get_debugreg() but before this check, local_db_save() will evaluate the old 0
> and skip disabling DR7. Can this cause the exception handler (such as #DB or
> #MC) to run with hardware breakpoints still enabled, risking a recursive
> exception?
Ah, good catch. It also need to check cpu_dr7_seq is not bumped too.
>
> >
> > /*
> > @@ -145,20 +148,34 @@ static __always_inline unsigned long
> > local_db_save(void)
> > * be good.
> > */
> > barrier();
> > -
> > - return dr7;
> > }
> >
> > -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)
> > +
> > + if (!dr7)
> > + return;
>
> [Severity: High]
> This isn't a bug introduced by this patch, but if an NMI interrupts a
> cleared DR7 state (such as during KVM guest entry), and the NMI installs
> a hardware breakpoint, doesn't this early return fail to restore the hardware
> DR7 back to its cleared state (0x400)?
Hmm, this was intended to avoid restoring cpu_dr7 over KVM's cleared state.
But if the NMI already overwrote hardware DR7 here, returning early leaves
the host breakpoint active when entering the guest.
OK, instead of an unconditional early return on "!dr7", local_db_restore()
should check if cpu_dr7_seq changed too...
Let me fix it.
>
> Consequently, it looks like KVM would resume the guest with the host's
> newly installed hardware breakpoint active.
>
> [ ... ]
>
> > diff --git a/arch/x86/kernel/hw_breakpoint.c
> > b/arch/x86/kernel/hw_breakpoint.c
> > index f846c15f21ca1..3204e75dc4546 100644
> > --- a/arch/x86/kernel/hw_breakpoint.c
> > +++ b/arch/x86/kernel/hw_breakpoint.c
>
> [ ... ]
>
> > @@ -97,38 +100,31 @@ int decode_dr7(unsigned long dr7, int bpnum, unsigned
> > *len, unsigned *type)
> > int arch_install_hw_breakpoint(struct perf_event *bp)
> > {
> > struct arch_hw_breakpoint *info = counter_arch_bp(bp);
> > - unsigned long *dr7;
> > + unsigned int seq;
> > int i;
> >
> > lockdep_assert_irqs_disabled();
> >
> > for (i = 0; i < HBP_NUM; i++) {
> > - struct perf_event **slot = this_cpu_ptr(&bp_per_reg[i]);
> > -
> > - if (!*slot) {
> > - *slot = bp;
> > + if (!this_cpu_cmpxchg(bp_per_reg[i], NULL, bp))
> > break;
> > - }
> > }
> >
> > if (WARN_ONCE(i == HBP_NUM, "Can't find any breakpoint slot"))
> > return -EBUSY;
> >
> > - set_debugreg(info->address, i);
> > - __this_cpu_write(cpu_debugreg[i], info->address);
> > -
> > - dr7 = this_cpu_ptr(&cpu_dr7);
> > - *dr7 |= encode_dr7(i, info->len, info->type);
> > -
> > - /*
> > - * Ensure we first write cpu_dr7 before we set the DR7 register.
> > - * This ensures an NMI never see cpu_dr7 0 when DR7 is not.
> > - */
> > - barrier();
> > -
> > - set_debugreg(*dr7, 7);
> > - if (info->mask)
> > - amd_set_dr_addr_mask(info->mask, i);
> > + do {
> > + seq = this_cpu_inc_return(cpu_dr7_seq);
> > + this_cpu_write(cpu_debugreg[i], info->address);
> > + barrier();
> > + set_debugreg(info->address, i);
> > + if (info->mask)
> > + amd_set_dr_addr_mask(info->mask, i);
> > + this_cpu_or(cpu_dr7, encode_dr7(i, info->len, info->type));
> > + barrier();
> > + set_debugreg(this_cpu_read(cpu_dr7), 7);
>
> [Severity: High]
> This is a pre-existing issue, but since arch_install_hw_breakpoint()
> unconditionally overwrites the hardware DR7 register inside the NMI,
> does this mean the KVM cleared state is already lost here before
> local_db_restore() is even called?
Yes, so checking the cpu_dr7_seq will avoid this issue too.
Thanks,
>
> > + barrier();
> > + } while (seq != this_cpu_read(cpu_dr7_seq));
> >
> > return 0;
> > }
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/178810001186.64882.2161016469449127450.stgit@devnote2?part=4
--
Masami Hiramatsu (Google) <[email protected]>
