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

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

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

--- Patch [3]: [PATCH v12 03/11] HWBP: Add modify_local_hw_breakpoint_addr() API ---
commit 894126e474a8f66ca7e12e6c727a417646ec68a9
Author: Masami Hiramatsu (Google) <[email protected]>

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