Re: [PATCH] arm64/hw_breakpoint: reject unaligned watchpoints that would truncate BAS

[Breno Leitao <leitao@debian.org>]

On Fri, May 29, 2026 at 03:53:58PM +0100, Will Deacon wrote:
> Hi Breno,
> 
> Thanks for sending this out.
> 
> On Thu, Apr 30, 2026 at 02:40:10AM -0700, Breno Leitao wrote:
> > hw_breakpoint_arch_parse() positions the BAS bit pattern in
> > hw->ctrl.len with
> > 
> > 	offset = hw->address & alignment_mask;	/* 0..7 */
> > 	hw->ctrl.len <<= offset;
> > 
> > ctrl.len is an 8-bit bitfield (struct arch_hw_breakpoint_ctrl::len is
> > u32 :8), so the shift silently drops any bits past bit 7.  For
> > non-compat AArch64 watchpoints the offset is unbounded relative to
> > ctrl.len: a perf_event_open(PERF_TYPE_BREAKPOINT) caller asking for
> > HW_BREAKPOINT_W with bp_addr=page+1 and bp_len=HW_BREAKPOINT_LEN_8
> > ends up with 0xff << 1 = 0x1fe, stored as 0xfe.  The kernel programs
> > WCR.BAS=0xfe and the hardware watches bytes [1..7] instead of the
> > requested [1..8] -- the eighth byte is silently dropped.  The
> > syscall still returns success, leaving userspace to discover the
> > gap by empirical probing.
> > 
> > The same class affects HW_BREAKPOINT_LEN_{2,4} when offset pushes the
> > high BAS bit past bit 7 (e.g. LEN_4 with offset=5 yields 0xe0
> > instead of 0x1e0).  No memory-safety impact -- the value is masked
> > into 8 bits before encoding -- but debuggers and perf users observe
> > missed events on bytes they thought they were watching.
> > 
> > The AArch32 branch immediately above already rejects unrepresentable
> > (offset, len) combinations via an explicit switch.  Mirror that for
> > the non-compat branch by checking that the shifted pattern fits in
> > the BAS field, returning -EINVAL when it does not.
> > 
> > Reproducer:
> > 
> >   struct perf_event_attr a = {
> >       .type = PERF_TYPE_BREAKPOINT, .size = sizeof(a),
> >       .bp_type = HW_BREAKPOINT_W,
> >       .bp_addr = (uintptr_t)(buf + 1),
> >       .bp_len = HW_BREAKPOINT_LEN_8,
> >       .exclude_kernel = 1, .exclude_hv = 1,
> >   };
> >   int fd = perf_event_open(&a, 0, -1, -1, 0);
> >   /* before this fix: succeeds, watches 7 bytes (buf+1..buf+7)   */
> >   /* after  this fix: fails with EINVAL                          */
> > 
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > Fixes: b08fb180bb88 ("arm64: Allow hw watchpoint at varied offset from base address")
> 
> Oh man, this has been broken for nearly a decade :/
> 
> I think we probably should've stuck with the old behaviour of rejecting
> unaligned base addresses, but it's too late now. Damn.
> 
> >  arch/arm64/kernel/hw_breakpoint.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> > index ab76b36dce820..b8a1402119f3a 100644
> > --- a/arch/arm64/kernel/hw_breakpoint.c
> > +++ b/arch/arm64/kernel/hw_breakpoint.c
> > @@ -559,6 +559,15 @@ int hw_breakpoint_arch_parse(struct perf_event *bp,
> >  		else
> >  			alignment_mask = 0x7;
> >  		offset = hw->address & alignment_mask;
> > +
> > +		/*
> > +		 * BAS is an 8-bit field in WCR/BCR; the shift below would
> > +		 * silently drop the high bits of ctrl.len when offset + len
> > +		 * exceeds 8, programming hardware to watch fewer bytes than
> > +		 * the user requested.
> > +		 */
> > +		if (((u32)hw->ctrl.len << offset) > 0xff)
> 
> nit: Use ARM_BREAKPOINT_LEN_8 instead of 0xff
> 
> > +			return -EINVAL;
> >  	}
> 
> I must confess, I'm very nervous about breaking userspace here. If GDB
> is triggering this path, then this patch will change an unreliable
> watchpoint into a hard error (which probably means GDB exits). Have you
> looked to see what GDB and/or any other debuggers do?
> 
> I had a quick peek and found the bugzilla entry which motivated the
> buggy change in the first place:
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=20207
> 
> and it looks like the aarch64_align_watchpoint() function does try to
> spill into multiple watchpoints, so perhaps your patch is ok. I'd
> appreciate your opinion, though.

It won't, for two independent reasons.

The new -EINVAL is unreachable from GDB; only a raw perf_event_open() passing
an unaligned base with an oversized bp_len hits it, which is the bug.

Second, even if a debugger did hand the kernel such a request, GDB
already treats EINVAL on NT_ARM_HW_WATCH as a downgrade signal, not a
fatal error. aarch64_linux_set_debug_regs() catches it, clears
kernel_supports_any_contiguous_range, calls aarch64_downgrade_regs()
(which rounds the BAS up to a legacy 0x01/03/0f/ff mask and aligns the
base down), and retries. That fallback is exactly the PR-20207 path.

Confirmed on a Grace box: with the patch applied (under virtme-ng), the
reproducer's unaligned LEN_8 now returns -EINVAL while aligned LEN_8
still succeeds, and GDB still inserts the watchpoint and it still fires
on every write.

GDB in fact downgrades on the current kernel independently of this patch, so
behaviour is unchanged for it.