arch/arm64/kernel/ptrace.c | 36 +++++++++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 5 deletions(-)
After revisiting my patches a few weeks later, I have some additional
thoughts:
1. Keep arm32 ptrace as it. Legacy APIs only need critical bug fix.
2. For arm64 ptrace, the patch is not only a fix for 32-bit tracee
but also a refactor that removes partially set state, making logic
clearer and preventing similar bugs.
The patch can be tested with the code in my previous mail:
https://lore.kernel.org/all/0fd573cc044584f00976c410955ed486@ntu.edu.tw/
---
Changes in v3:
- Remove patches for arm32 and compat ptrace
- Rewrite commit message
- Reformat the arg of the added function
Changes in v2:
- Fix username
Bill Tsui (1):
arm64: ptrace: fix hw_break_set() to set addr and ctrl together
arch/arm64/kernel/ptrace.c | 36 +++++++++++++++++++++++++++++++-----
1 file changed, 31 insertions(+), 5 deletions(-)
--
2.51.0
Fix the unused functions detected by kernel test robot. Link: https://lore.kernel.org/all/202510181547.dI5kyPuT-lkp@intel.com/ --- Changes in v4: - Fix build warning of unused functions by moving ptrace_hbp_set_ctrl() and ptrace_hbp_set_addr() under CONFIG_COMPAT and add compat_ prefix to the function names. Changes in v3: - Remove patches for arm32 and compat ptrace - Rewrite commit message - Reformat the arg of the added function Changes in v2: - Fix username Bill Tsui (1): arm64: ptrace: fix hw_break_set() to set addr and ctrl together arch/arm64/kernel/ptrace.c | 90 ++++++++++++++++++++++++-------------- 1 file changed, 58 insertions(+), 32 deletions(-) -- 2.51.0
This patch fixes the failure of PTRACE_SETREGSET when setting a hardware
breakpoint on a non-4-byte aligned address with a valid control to a
32-bit tracee. The issue was discovered while testing LLDB.
Link: https://github.com/llvm/llvm-project/pull/152284
The failure happens because hw_break_set() checks and sets the breakpoint
address and control separately. This can result in an check failure when
it first validates the address to be set with old control.
For example, the control are initialized with breakpoint length of 4.
Combining with a non-4-byte aligned address would cross a 4-byte boundary,
which is invalid. However, the user-provided control may actually specify a
length of 1, which should be valid.
The fix is to set the address and control together. This is supported
by modify_user_hw_breakpoint(), the function that sets and checks
breakpoints. The original implementation wrap this function to
ptrace_hbp_set_addr() and ptrace_hbp_set_ctrl() for 32-bit API
PTRACE_SETHBPREGS simply because it can only modify one register (address
or control) per call. For the 64-bit PTRACE_SETREGSET API, this
restriction does not apply, so a new helper function ptrace_hbp_set() was
added to set both in a single call to modify_user_hw_breakpoint().
Then ptrace_hbp_set_addr() and ptrace_hbp_set_ctrl() are only used by
compat_ptrace_hbp_set(), so moved into CONFIG_COMPAT block and renamed
with prefix compat_.
For reference, the check is in
arch/arm64/kernel/hw_breakpoint.c:hw_breakpoint_arch_parse()
which is called via:
modify_user_hw_breakpoint()
-> modify_user_hw_breakpoint_check()
-> hw_breakpoint_parse()
-> hw_breakpoint_arch_parse()
Signed-off-by: Bill Tsui <b10902118@ntu.edu.tw>
---
arch/arm64/kernel/ptrace.c | 90 ++++++++++++++++++++++++--------------
1 file changed, 58 insertions(+), 32 deletions(-)
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 4b001121c72d..c8caf4e0805e 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -421,10 +421,11 @@ static struct perf_event *ptrace_hbp_get_initialised_bp(unsigned int note_type,
return bp;
}
-static int ptrace_hbp_set_ctrl(unsigned int note_type,
- struct task_struct *tsk,
- unsigned long idx,
- u32 uctrl)
+/* Set the address and control together for non-compat ptrace */
+static int ptrace_hbp_set(unsigned int note_type,
+ struct task_struct *tsk,
+ unsigned long idx,
+ u64 addr, u32 uctrl)
{
int err;
struct perf_event *bp;
@@ -438,6 +439,8 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
}
attr = bp->attr;
+ attr.bp_addr = addr;
+
decode_ctrl_reg(uctrl, &ctrl);
err = ptrace_hbp_fill_attr_ctrl(note_type, ctrl, &attr);
if (err)
@@ -446,27 +449,6 @@ static int ptrace_hbp_set_ctrl(unsigned int note_type,
return modify_user_hw_breakpoint(bp, &attr);
}
-static int ptrace_hbp_set_addr(unsigned int note_type,
- struct task_struct *tsk,
- unsigned long idx,
- u64 addr)
-{
- int err;
- struct perf_event *bp;
- struct perf_event_attr attr;
-
- bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
- if (IS_ERR(bp)) {
- err = PTR_ERR(bp);
- return err;
- }
-
- attr = bp->attr;
- attr.bp_addr = addr;
- err = modify_user_hw_breakpoint(bp, &attr);
- return err;
-}
-
#define PTRACE_HBP_ADDR_SZ sizeof(u64)
#define PTRACE_HBP_CTRL_SZ sizeof(u32)
#define PTRACE_HBP_PAD_SZ sizeof(u32)
@@ -524,9 +506,6 @@ static int hw_break_set(struct task_struct *target,
return -EINVAL;
ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &addr,
offset, offset + PTRACE_HBP_ADDR_SZ);
- if (ret)
- return ret;
- ret = ptrace_hbp_set_addr(note_type, target, idx, addr);
if (ret)
return ret;
offset += PTRACE_HBP_ADDR_SZ;
@@ -537,10 +516,11 @@ static int hw_break_set(struct task_struct *target,
offset, offset + PTRACE_HBP_CTRL_SZ);
if (ret)
return ret;
- ret = ptrace_hbp_set_ctrl(note_type, target, idx, ctrl);
+ offset += PTRACE_HBP_CTRL_SZ;
+
+ ret = ptrace_hbp_set(note_type, target, idx, addr, ctrl);
if (ret)
return ret;
- offset += PTRACE_HBP_CTRL_SZ;
user_regset_copyin_ignore(&pos, &count, &kbuf, &ubuf,
offset, offset + PTRACE_HBP_PAD_SZ);
@@ -2139,6 +2119,52 @@ static int compat_ptrace_hbp_get(unsigned int note_type,
return err;
}
+static int compat_ptrace_hbp_set_ctrl(unsigned int note_type,
+ struct task_struct *tsk,
+ unsigned long idx,
+ u32 uctrl)
+{
+ int err;
+ struct perf_event *bp;
+ struct perf_event_attr attr;
+ struct arch_hw_breakpoint_ctrl ctrl;
+
+ bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
+ if (IS_ERR(bp)) {
+ err = PTR_ERR(bp);
+ return err;
+ }
+
+ attr = bp->attr;
+ decode_ctrl_reg(uctrl, &ctrl);
+ err = ptrace_hbp_fill_attr_ctrl(note_type, ctrl, &attr);
+ if (err)
+ return err;
+
+ return modify_user_hw_breakpoint(bp, &attr);
+}
+
+static int compat_ptrace_hbp_set_addr(unsigned int note_type,
+ struct task_struct *tsk,
+ unsigned long idx,
+ u64 addr)
+{
+ int err;
+ struct perf_event *bp;
+ struct perf_event_attr attr;
+
+ bp = ptrace_hbp_get_initialised_bp(note_type, tsk, idx);
+ if (IS_ERR(bp)) {
+ err = PTR_ERR(bp);
+ return err;
+ }
+
+ attr = bp->attr;
+ attr.bp_addr = addr;
+ err = modify_user_hw_breakpoint(bp, &attr);
+ return err;
+}
+
static int compat_ptrace_hbp_set(unsigned int note_type,
struct task_struct *tsk,
compat_long_t num,
@@ -2151,10 +2177,10 @@ static int compat_ptrace_hbp_set(unsigned int note_type,
if (num & 1) {
addr = *kdata;
- err = ptrace_hbp_set_addr(note_type, tsk, idx, addr);
+ err = compat_ptrace_hbp_set_addr(note_type, tsk, idx, addr);
} else {
ctrl = *kdata;
- err = ptrace_hbp_set_ctrl(note_type, tsk, idx, ctrl);
+ err = compat_ptrace_hbp_set_ctrl(note_type, tsk, idx, ctrl);
}
return err;
--
2.51.0On Sat, Oct 18, 2025 at 09:37:31PM +0800, Bill Tsui wrote: > This patch fixes the failure of PTRACE_SETREGSET when setting a hardware > breakpoint on a non-4-byte aligned address with a valid control to a > 32-bit tracee. The issue was discovered while testing LLDB. > > Link: https://github.com/llvm/llvm-project/pull/152284 > > The failure happens because hw_break_set() checks and sets the breakpoint > address and control separately. This can result in an check failure when > it first validates the address to be set with old control. > > For example, the control are initialized with breakpoint length of 4. > Combining with a non-4-byte aligned address would cross a 4-byte boundary, > which is invalid. However, the user-provided control may actually specify a > length of 1, which should be valid. > > The fix is to set the address and control together. ... but you only implement this for the native (64-bit) case, so I don't understand how it helps with the problem above. > For reference, the check is in > arch/arm64/kernel/hw_breakpoint.c:hw_breakpoint_arch_parse() > which is called via: > modify_user_hw_breakpoint() > -> modify_user_hw_breakpoint_check() > -> hw_breakpoint_parse() > -> hw_breakpoint_arch_parse() You don't need to include these details here. > @@ -524,9 +506,6 @@ static int hw_break_set(struct task_struct *target, > return -EINVAL; > ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &addr, > offset, offset + PTRACE_HBP_ADDR_SZ); > - if (ret) > - return ret; > - ret = ptrace_hbp_set_addr(note_type, target, idx, addr); > if (ret) > return ret; > offset += PTRACE_HBP_ADDR_SZ; > @@ -537,10 +516,11 @@ static int hw_break_set(struct task_struct *target, > offset, offset + PTRACE_HBP_CTRL_SZ); > if (ret) > return ret; > - ret = ptrace_hbp_set_ctrl(note_type, target, idx, ctrl); > + offset += PTRACE_HBP_CTRL_SZ; > + > + ret = ptrace_hbp_set(note_type, target, idx, addr, ctrl); Doesn't this break the case where userspace tries only to set the address? The loop will break out when !count without updating anything. As I mentioned before, I'd prefer to leave this code as-is short of removing the indirection through perf entirely. Will
On 2025-11-15 00:14, Will Deacon wrote:
> ... but you only implement this for the native (64-bit) case, so I
> don't
> understand how it helps with the problem above.
Yes it is only for 64-bit tracer to debug 32-bit processes because
64-bit tracer always use 64-bit ptrace API (PTRACE_SETREGSET) regardless
of tracee, so only 64-bit code were changed.
>> @@ -524,9 +506,6 @@ static int hw_break_set(struct task_struct
>> *target,
>> return -EINVAL;
>> ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &addr,
>> offset, offset + PTRACE_HBP_ADDR_SZ);
>> - if (ret)
>> - return ret;
>> - ret = ptrace_hbp_set_addr(note_type, target, idx, addr);
>> if (ret)
>> return ret;
>> offset += PTRACE_HBP_ADDR_SZ;
>> @@ -537,10 +516,11 @@ static int hw_break_set(struct task_struct
>> *target,
>> offset, offset + PTRACE_HBP_CTRL_SZ);
>> if (ret)
>> return ret;
>> - ret = ptrace_hbp_set_ctrl(note_type, target, idx, ctrl);
>> + offset += PTRACE_HBP_CTRL_SZ;
>> +
>> + ret = ptrace_hbp_set(note_type, target, idx, addr, ctrl);
>
> Doesn't this break the case where userspace tries only to set the
> address?
> The loop will break out when !count without updating anything.
>
The case doesn't exist for normal use. I saw that as a validation
rather than an intended stop point. If in doubt, I can remove it.
The signature from man ptrace(2)
ptrace(PTRACE_SETREGSET, pid, NT_foo, &iov)
requires all registers' values to be specified in iov in order. And the
unit of debug registers is the inline structure in user_hwdebug_state:
struct {
__u64 addr;
__u32 ctrl;
__u32 pad;
} dbg_regs[16];
and userspace uses by:
struct user_hwdebug_state dreg_state;
ioVec.iov_base = &dreg_state;
ioVec.iov_len = sizeof(dreg_state.dbg_info) + sizeof(dreg_state.pad)
+ (sizeof(dreg_state.dbg_regs[0]) * m_max_hwp_supported);
for (uint32_t i = 0; i < m_max_hwp_supported; i++) {
dreg_state.dbg_regs[i].addr = m_hwp_regs[i].address;
dreg_state.dbg_regs[i].ctrl = m_hwp_regs[i].control;
}
The only case of setting only address is to have iov length only reach
the first register's .addr, but I don't think that memory layout should
be relied on.
> As I mentioned before, I'd prefer to leave this code as-is short of
> removing the indirection through perf entirely.
I am working on that, the side effects are hard to trace, and I think
this patch can make removing perf easier by reducing calls to it and
make clearer what hardware API ptrace needs. This recurs to me and I
think it can be a separated patch.
Here is my progress on removing perf:
I tried to directly set by hw_breakpoint_control, which perf finally
use, but we also relied on perf event handling cpu interrupts when
breakpoints get triggered. This is the hardest part and I am learning
how the kernel handles it across so many others. I feel that the use
of perf has its reasons but definitely there is space to improve.
Anyway, I respect maintainer's decision. I just felt that you thought
this is complicated so I simplifed it. Now I explains the correctness
and how it relates to your final goal. If there are still concerns,
please directly reject me, then I can also stop holding on it.
Thanks,
Bill
© 2016 - 2026 Red Hat, Inc.