Re: [PATCH v7 3/8] riscv: ptrace support for hardware break/watchpoints

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] riscv/ptrace: HW breakpoints leaked across execve() via PTRACE_SETHBPREGS
- [High] riscv/ptrace: PTRACE_GETREGSET fails with -EINVAL when triggers > 16
- [High] riscv/ptrace: kernel stack leak via uninitialized memory in riscv_hw_break_set
- [Medium] riscv/ptrace: missing hardware breakpoint support for compat processes
- [Medium] riscv/ptrace: SETHBPREGS and SETREGSET operate on disjoint breakpoint arrays
- [Medium] riscv/ptrace: missing array_index_nospec in riscv_ptrace_bp_get/set
--

commit 62c2c75a3fb752fd74e38e9ad58187d943832421
Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>

riscv: ptrace support for hardware break/watchpoints

This commit adds ptrace support for hardware breakpoints and watchpoints
on RISC-V. It introduces NT_RISCV_HW_BREAK/WATCH for GETREGSET/SETREGSET
and a convenience request pair PTRACE_GETHBPREGS/SETHBPREGS.

> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> index f336a183667eb..2d4ee51e68597 100644
> --- a/arch/riscv/kernel/ptrace.c
> +++ b/arch/riscv/kernel/ptrace.c

[ ... ]

> @@ -372,6 +378,397 @@ static int riscv_cfi_set(struct task_struct *target,
> +void flush_ptrace_hw_breakpoint(struct task_struct *tsk)
> +{
> +	int i;
> +	struct thread_struct *t = &tsk->thread;
> +
> +	for (i = 0; i < RISCV_MAX_BP; i++) {
> +		if (t->debug.hbp_break[i]) {
> +			unregister_hw_breakpoint(t->debug.hbp_break[i]);
> +			t->debug.hbp_break[i] = NULL;
> +		}
> +	}

[Severity: High]
Does this code leak hardware breakpoints across execve()?

PTRACE_SETHBPREGS stores hardware breakpoints in child->thread.ptrace_bps,
but this function only unregisters breakpoints in debug.hbp_break and
completely ignores ptrace_bps. If the tracee executes a SUID binary, do the
hardware breakpoints remain active in the privileged context?

[ ... ]

> +static int ptrace_hbp_set_event(unsigned int note_type,
> +				struct task_struct *tsk,
> +				unsigned long idx,
> +				struct perf_event *bp)
> +{
> +	int err = -EINVAL;
> +
> +	switch (note_type) {
> +	case NT_RISCV_HW_BREAK:
> +		if (idx >= RISCV_MAX_BP)
> +			goto out;
> +		idx = array_index_nospec(idx, RISCV_MAX_BP);
> +		tsk->thread.debug.hbp_break[idx] = bp;

[Severity: Medium]
Are PTRACE_SETHBPREGS and PTRACE_SETREGSET operating on completely disjoint
memory locations?

PTRACE_SETREGSET populates tsk->thread.debug.hbp_break, while PTRACE_SETHBPREGS
populates tsk->thread.ptrace_bps. Can a debugger using both APIs independently
allocate up to 16 breakpoints in each array, potentially consuming the entire
hardware trigger pool and observing disconnected states?

[ ... ]

> +static int ptrace_hbp_get_resource_info(unsigned int note_type, u32 *info)
> +{
> +	u8 num;
> +
> +	switch (note_type) {
> +	case NT_RISCV_HW_BREAK:
> +		num = hw_breakpoint_slots(TYPE_INST);
> +		break;

[Severity: High]
Can hw_breakpoint_slots() return a value larger than 16 (RISCV_MAX_BP)?

If a platform has more than 16 triggers, riscv_hw_break_get() and
riscv_hw_break_set() will iterate up to that number. However, the accessor
ptrace_hbp_get_event() contains a hardcoded bounds check returning -EINVAL
if idx >= RISCV_MAX_BP.

Would this cause the entire PTRACE_GETREGSET/SETREGSET syscall to abort with
-EINVAL when hitting index 16 on such platforms?

[ ... ]

> +static int riscv_hw_break_set(struct task_struct *target,
> +			const struct user_regset *regset,
> +			unsigned int pos, unsigned int count,
> +			const void *kbuf, const void __user *ubuf)
> +{
> +	unsigned int note_type = regset->core_note_type;
> +	int ret, idx = 0, offset, limit;
> +	u32 ctrl;

[Severity: High]
Is ctrl properly initialized before use?

user_regset_copyin() allows partial copies if count is smaller than the target
size. If a tracer supplies an iov_len of 9 or 17, user_regset_copyin() will
copy only 1 byte into ctrl, leaving the remaining 3 bytes containing
uninitialized kernel stack data.

When this partially initialized ctrl is passed to ptrace_hbp_set_ctrl() and
later read out via PTRACE_GETREGSET, could it successfully leak uninitialized
kernel stack memory to userspace?

[ ... ]

> +static int riscv_ptrace_bp_get(struct task_struct *child, unsigned long idx,
> +			  struct __riscv_hwdebug_state *state)
> +{
> +	struct perf_event *bp;
> +
> +	if (idx >= RISCV_HW_BP_NUM_MAX)
> +		return -EINVAL;
> +
> +	bp = child->thread.ptrace_bps[idx];

[Severity: Medium]
Might an array_index_nospec() mitigation be needed here to prevent speculative
out-of-bounds reads?

The user-supplied index idx is bounds-checked, but without array_index_nospec(),
speculative execution can bypass the bounds check and read out-of-bounds from
the ptrace_bps array. The resulting pointer is immediately dereferenced, which
could provide an arbitrary speculative load gadget.

[ ... ]

Since compat_riscv_user_regset and compat_arch_ptrace() were not modified in
this patch:

arch/riscv/kernel/ptrace.c:compat_riscv_user_regset[] = {
...
arch/riscv/kernel/ptrace.c:compat_arch_ptrace() {

[Severity: Medium]
Will 32-bit processes running on a 64-bit kernel be able to use hardware
breakpoints?

These additions were omitted from compat_riscv_user_regset and
compat_arch_ptrace(), which seems to break feature parity for compat tracing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930063919.608911-1-himanshu.chauhan@oss.qualcomm.com?part=3