Re: [PATCH v12 03/11] HWBP: Add modify_local_hw_breakpoint_addr() API

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

On Fri, 07 Aug 2026 15:58:01 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [High] Modifying hardware debug address registers (DR0-DR3) while they are still enabled in DR7 violates the x86 architectural guidelines and can cause unpredictable hardware behavior.

It uses local_db_save/restore() to atomically update it. So it should be safe.

> - [Medium] Temporary invalid software state in `bp->attr.bp_addr` before validation causes incorrect perf samples if interrupted by an NMI.

Note that this is only used from wprobe, which checks the address is correct.
Anyway, I will add a comment about that.

> 
> Pre-existing issues:
> - [High] Lockless traversal of `kprobe_blacklist` can lead to Use-After-Free during module unload.

This seems a funny hallucination... why have you pointed unrelated kprobe things??

> --
> 
> --- Patch [3]: [PATCH v12 03/11] HWBP: Add modify_local_hw_breakpoint_addr() API ---
> commit 894126e474a8f66ca7e12e6c727a417646ec68a9
> Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> HWBP: Add modify_local_hw_breakpoint_addr() API
> 
> This adds modify_local_hw_breakpoint_addr() to update the watched address
> of an installed hardware breakpoint on the local CPU without releasing and
> reserving its hardware slot. It is available when architectures select
> HAVE_MODIFY_LOCAL_HW_BREAKPOINT_ADDR.
> 
> Link: https://lore.kernel.org/all/f9c49dfa49bdc57ba8c0574bc9981c1e581acf92.1785067572.git.wangjinchao600@gmail.com/
> 
> > diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> > index 789add0c185a0..7d3a7bc050a40 100644
> > --- a/kernel/events/hw_breakpoint.c
> > +++ b/kernel/events/hw_breakpoint.c
> 
> [ ... ]
> 
> > @@ -888,6 +888,46 @@ void unregister_wide_hw_breakpoint(struct perf_event * __percpu *cpu_events)
> >  }
> >  EXPORT_SYMBOL_GPL(unregister_wide_hw_breakpoint);
> >  
> > +/**
> > + * modify_local_hw_breakpoint_addr - update a local breakpoint address
> > + * @bp: the hwbp perf event for this CPU
> > + * @addr: the new address for @bp
> > + *
> > + * Update only the address of an installed breakpoint on the local CPU without
> > + * releasing and reserving its hardware slot. The caller must update other CPUs.
> > + * Return 0, or -EOPNOTSUPP if the architecture does not support this operation.
> > + */
> > +#ifdef CONFIG_HAVE_MODIFY_LOCAL_HW_BREAKPOINT_ADDR
> > +int modify_local_hw_breakpoint_addr(struct perf_event *bp,
> > +				    unsigned long addr)
> > +{
> > +	unsigned long old_addr;
> > +	int ret;
> > +
> > +	lockdep_assert_irqs_disabled();
> > +
> > +	old_addr = bp->attr.bp_addr;
> > +	counter_arch_bp(bp)->address = addr;
> > +	bp->attr.bp_addr = addr;
> 
> [Severity: Medium]
> Could this early update of bp->attr.bp_addr lead to incorrect perf samples?
> 
> If an NMI triggers the hardware breakpoint on the old address before the
> hardware registers are updated and validated, the NMI handler might locklessly
> read the temporarily overwritten bp->attr.bp_addr. Would this attribute the
> sample to the new, potentially invalid address instead of the old one?
> 
> > +
> > +	ret = arch_modify_local_hw_breakpoint_addr(bp, addr);
> 
> [Severity: High]
> Does modifying the hardware debug address register without clearing the DR7
> enable bits violate x86 architectural guidelines?
> 
> When arch_modify_local_hw_breakpoint_addr() is called for x86:
> 
> arch/x86/kernel/hw_breakpoint.c:arch_modify_local_hw_breakpoint_addr() {
>     ...
>     do {
>         seq = this_cpu_inc_return(cpu_dr7_seq);
>         this_cpu_write(cpu_debugreg[i], addr);
>         barrier();
>         set_debugreg(addr, i);
>     } while (seq != this_cpu_read(cpu_dr7_seq));
>     ...
> }
> 
> Could this cause unpredictable hardware behavior since the Intel manual
> dictates that DR0-DR3 should only be modified when their corresponding
> enable bits in DR7 are clear?
> 
> [Severity: High]
> This is a pre-existing issue, but can the lockless traversal in the
> blacklist check lead to a use-after-free during module unload?
> 
> When parsing the breakpoint, the call chain goes through:
> arch_modify_local_hw_breakpoint_addr() -> hw_breakpoint_arch_parse() ->
> arch_build_bp_info() -> within_kprobe_blacklist()
> 
> kernel/kprobes.c:__within_kprobe_blacklist() {
>     ...
>     list_for_each_entry(ent, &kprobe_blacklist, list) {
>         if (addr >= ent->start_addr && addr < ent->end_addr)
>             return true;
>     }
>     ...
> }
> 
> If a module is unloaded concurrently, kprobe_remove_area_blacklist() deletes
> entries and immediately frees them with kfree(). Would a concurrent hardware
> breakpoint modification dereference freed memory here since kprobe_mutex
> is not held and RCU protection is not used?
> 
> > +	if (ret) {
> > +		counter_arch_bp(bp)->address = old_addr;
> > +		bp->attr.bp_addr = old_addr;
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/178611679753.237811.10190793347982785419.stgit@devnote2?part=3


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