On Mon, 07 Sep 2026 04:00:16 +0000
[email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s)
> to consider:
>
> New issues:
> - [High] local_db_restore() incorrectly uses dr7 == 0 to discard
> NMI-installed hardware breakpoints, permanently losing debug state and
> corrupting KVM guest debug registers.
> - [High] local_db_save() destructively clears hardware DR7 inside its
> sequence retry loop, permanently corrupting the restored state if a retry
> occurs.
>
> Pre-existing issues:
> - [High] The early return in local_db_save() bypasses the sequence retry
> loop, leaving a race window where an NMI can leave hardware breakpoints
> enabled during critical exception handlers.
> --
>
> commit 48808d0a31b45134e9ef897c90c0f96bd987d9ce
> Author: Jinchao Wang <[email protected]>
>
> x86/hw_breakpoints: Make DR7 updates NMI safe
>
> This commit introduces atomic per-CPU operations and sequence numbers to
> make DR7 hardware breakpoint updates NMI safe. It updates local_db_save()
> and local_db_restore() to use retry loops that detect NMI interruptions,
> preventing state corruption.
>
> 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 854d82b88ff4e..3399e166d3044 100644
> > --- a/arch/x86/include/asm/debugreg.h
> > +++ b/arch/x86/include/asm/debugreg.h
> [ ... ]
> > @@ -125,40 +126,66 @@ 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;
> > + do {
> > + *dr7_seq = this_cpu_read(cpu_dr7_seq);
> > + *dr7 = 0;
> >
> > - if (cpu_feature_enabled(X86_FEATURE_HYPERVISOR) &&
> > !hw_breakpoint_active())
> > - return 0;
> > -
> > - get_debugreg(dr7, 7);
> > + if (cpu_feature_enabled(X86_FEATURE_HYPERVISOR) &&
> > + !hw_breakpoint_active())
> > + return;
>
> [Severity: High]
> This isn't a bug introduced by this patch, but could this early return
> in local_db_save() bypass the sequence retry loop?
>
> If an NMI hits exactly before the return instruction and installs a
> breakpoint, the function exits immediately. This leaves the newly installed
> hardware breakpoint active during exception handlers like MCE, which could
> trigger a recursive #DB exception and lead to a kernel panic.
Hmm, OK. But this should be fixed in this patch because this is for solving
this kind of NMI related issue.
Let me update checking cpu_dr7_seq is updated or not before returning.
>
> >
> > + get_debugreg(*dr7, 7);
> >
> > - /* Architecturally set bit */
> > - dr7 &= ~DR7_FIXED_1;
> > - if (dr7)
> > - set_debugreg(DR7_FIXED_1, 7);
> > + /* Architecturally set bit */
> > + *dr7 &= ~DR7_FIXED_1;
> > + if (*dr7)
> > + set_debugreg(DR7_FIXED_1, 7);
>
> [Severity: High]
> Can this destructively overwrite the hardware DR7 state inside the sequence
> retry loop?
>
> If an NMI interrupts execution between reading and clearing DR7, installs a
> breakpoint, and increments cpu_dr7_seq, this set_debugreg() call will
> overwrite the NMI's new hardware state with 0x400.
>
> When the loop detects the sequence mismatch and retries, the next
> iteration's get_debugreg() will read this clobbered 0x400 value instead of
> the state installed by the NMI, permanently corrupting the restored state.
Ah, OK. Let me add sequence number check there and use cpu_dr7 instead of
raw dr7 value.
>
> >
> > - /*
> > - * Ensure the compiler doesn't lower the above statements into
> > - * the critical section; disabling breakpoints late would not
> > - * be good.
> > - */
> > - barrier();
> > + /*
> > + * Ensure the compiler doesn't lower the above statements into
> > + * the critical section; disabling breakpoints late would not
> > + * be good.
> > + */
> > + barrier();
> > + } while (unlikely(*dr7_seq != this_cpu_read(cpu_dr7_seq)));
> > }
> [ ... ]
> > -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 long val;
> > + 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)
> > - set_debugreg(dr7, 7);
> > +
> > + do {
> > + seq = this_cpu_read(cpu_dr7_seq);
> > + if (seq == dr7_seq) {
> > + if (!dr7)
> > + return;
> > + val = dr7;
> > + } else {
> > + /*
> > + * If an NMI modified breakpoints while DR7 was saved
> > as 0
> > + * (e.g. KVM guest entry), arch_install_hw_breakpoint()
> > + * clobbered hardware DR7. Restore val = 0 (DR7_FIXED_1)
> > + * to clean up the NMI's write and preserve guest
> > isolation.
> > + * Otherwise, load the latest cpu_dr7 state.
> > + */
> > + val = dr7 ? this_cpu_read(cpu_dr7) : 0;
>
> [Severity: High]
> Does this logic permanently discard the hardware breakpoint installed by an
> NMI if dr7 was initially 0?
>
> If dr7 was saved as 0, which happens when no host breakpoints were active,
> and an NMI subsequently installs a breakpoint, this logic evaluates to 0
> and ignores the new cpu_dr7 state.
>
> It will then write 0x400 to hardware DR7, completely overriding the NMI
> update. Furthermore, if an NMI interrupts a KVM guest, the guest's DR7
> state is completely destroyed and replaced with 0x400.
OK, then I always use cpu_dr7 for value.
Thanks,
>
> > + }
> > +
> > + set_debugreg(val | DR7_FIXED_1, 7);
> > + barrier();
> > + } while (unlikely(seq != this_cpu_read(cpu_dr7_seq)));
> > }
>
> --
> Sashiko AI review ยท
> https://sashiko.dev/#/patchset/178875277830.93794.14247844688761142429.stgit@devnote2?part=2
--
Masami Hiramatsu (Google) <[email protected]>
