Re: [PATCH 3/3] HWBP: Add modify_local_hw_breakpoint_addr() API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] arch_modify_local_hw_breakpoint_addr() lacks NMI-safe sequence counter protection and violates the x86 SDM by modifying active debug registers.
- [High] modify_local_hw_breakpoint_addr() bypasses mandatory architecture-specific address validation, allowing safe-guards to be circumvented.
- [Medium] modify_local_hw_breakpoint_addr() updates generic software state locklessly before calling the arch-specific function, and returns success even if the arch function fails.
--

--- Patch [3]: [PATCH 3/3] HWBP: Add modify_local_hw_breakpoint_addr() API ---
commit 46c7b1b307ec0fc122a4f26e8e8f3b876fbd24b7
Author: Masami Hiramatsu (Google) <mhiramat@kernel.org>

HWBP: Add modify_local_hw_breakpoint_addr() API
    
Add modify_local_hw_breakpoint_addr() to update only the watched
address of an installed hardware breakpoint on the local CPU without
releasing and reserving its hardware slot. This is available when the
architecture selects CONFIG_HAVE_MODIFY_LOCAL_HW_BREAKPOINT_ADDR.
    
The caller must provide an installed local event and a valid address,
and update other CPUs separately.

[ ... ]

> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 789add0c185a0..0b1fc01f19349 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -888,6 +888,36 @@ 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)
> +{
> +	lockdep_assert_irqs_disabled();
> +
> +	counter_arch_bp(bp)->address = addr;
> +	bp->attr.bp_addr = addr;

[Severity: High]
Is mandatory architecture-specific address validation bypassed here?

The normal breakpoint installation path uses hw_breakpoint_arch_parse() to
ensure the address is safe (e.g., properly aligned, canonical, and not
overlapping with critical CPU entry areas like cpu_dr7_seq or the GDT).

By blindly assigning the new address and passing it to the architecture layer
without invoking hw_breakpoint_arch_parse() or any architectural validation,
modify_local_hw_breakpoint_addr() bypasses all these safe-guards.

Since generic code cannot know x86-specific exclusion zones, any caller
supplying an invalid address will directly program the hardware, potentially
triggering recursive exceptions that hang the system.

> +	arch_modify_local_hw_breakpoint_addr(bp, addr);

[Severity: High]
Are we missing NMI-safe sequence counter protection in
arch_modify_local_hw_breakpoint_addr()?

Looking at arch_modify_local_hw_breakpoint_addr() in
arch/x86/kernel/hw_breakpoint.c, it modifies the CPU debug register directly
without incrementing cpu_dr7_seq to protect against NMI races, and without
temporarily disabling the breakpoint in DR7:

    this_cpu_write(cpu_debugreg[i], addr);
    barrier();
    set_debugreg(addr, i);

If an NMI occurs inside hw_breakpoint_restore() between reading a debug
register and writing it, and the NMI handler uses
modify_local_hw_breakpoint_addr() to update the same debug register, the
sequence counter (cpu_dr7_seq) is not incremented.

When hw_breakpoint_restore() resumes, it will overwrite the debug register
with the old stale address without detecting the interference, while the
software state retains the new address.

Additionally, does writing directly to DR0-DR3 while the breakpoint is
enabled in DR7 violate the Intel SDM (Vol 3, 18.2.2)? This could result in
hardware and software breakpoint state becoming permanently desynced, and
undefined hardware behavior may occur.

> +
> +	return 0;

[Severity: Medium]
Could this unconditionally return success even if the underlying architecture
function fails?

If a caller passes an inactive or remote-CPU hardware breakpoint to
modify_local_hw_breakpoint_addr(), the generic code unconditionally updates
bp->attr.bp_addr and counter_arch_bp(bp)->address locklessly before calling
the arch-specific function.

The arch_modify_local_hw_breakpoint_addr() function will fail to find the
slot, trigger a WARN_ONCE, and return early without modifying hardware.

However, since it returns void, the generic function unconditionally
returns 0 (success). This leaves the software and hardware states permanently
desynced.

> +}
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785067572.git.wangjinchao600@gmail.com?part=3