Re: [PATCH v8 4/8] riscv: add regset access for hardware breakpoints

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] riscv: ptrace: broken breakpoint disable in
  ptrace_hbp_fill_attr_ctrl()
- [Medium] riscv: ptrace: zero-filled holes in compat_riscv_user_regset
  cause div-by-zero
- [Medium] riscv: ptrace: ignored control fields in decode_ctrl_reg()
- [Medium] riscv: ptrace: wrong return value and slot bound in
  riscv_hw_break_get()
- [Medium] riscv: ptrace: leaked perf_event on error in
  riscv_hw_break_set()
--

commit 5940d70e24c6c5b6aa9c305717a9b36acc0903b2
Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>

riscv: add regset access for hardware breakpoints

This commit adds the NT_RISCV_HW_BREAK and NT_RISCV_HW_WATCH regsets to allow
reading and programming RISC-V hardware breakpoint and watchpoint slots via
PTRACE_GETREGSET and PTRACE_SETREGSET.

> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> index be076865d02d0..9959a8e4a3079 100644
> --- a/arch/riscv/kernel/ptrace.c
> +++ b/arch/riscv/kernel/ptrace.c
> @@ -37,6 +37,10 @@ enum riscv_regset {
>  #ifdef CONFIG_RISCV_USER_CFI
>  	REGSET_CFI,
>  #endif
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	REGSET_RISCV_HW_BREAK,
> +	REGSET_RISCV_HW_WATCH,
> +#endif
>  };
>  
>  static int riscv_gpr_get(struct task_struct *target,
> @@ -526,6 +530,302 @@ static struct perf_event *ptrace_hbp_create(enum riscv_hbp_type hbp_type,
>  	return bp;
>  }
>  
> +static int ptrace_hbp_note_to_type(unsigned int note_type,
> +				   enum riscv_hbp_type *hbp_type)
> +{
> +	switch (note_type) {
> +	case NT_RISCV_HW_BREAK:
> +		*hbp_type = RISCV_HBP_BREAK;
> +		return 0;
> +	case NT_RISCV_HW_WATCH:
> +		*hbp_type = RISCV_HBP_WATCH;
> +		return 0;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ptrace_hbp_fill_attr_ctrl(enum riscv_hbp_type hbp_type,
> +				     struct arch_hw_breakpoint *bpctrl,
> +				     struct perf_event_attr *attr)
> +{
> +	int len, type;
> +
> +	attr->disabled = 0;
> +	type = bpctrl->type;
> +	len = bpctrl->len;
> +
> +	switch (hbp_type) {
> +	case RISCV_HBP_BREAK:
> +		if ((type & HW_BREAKPOINT_X) != type)
> +			return -EINVAL;
> +		break;
> +	case RISCV_HBP_WATCH:
> +		if ((type & HW_BREAKPOINT_RW) != type)
> +			return -EINVAL;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	attr->bp_len	= len;
> +	attr->bp_type	= type;
> +	attr->bp_addr	= bpctrl->address;
> +
> +	return 0;
> +}

[ ... ]

> +static u32 encode_ctrl_reg(struct perf_event *bp)
> +{
> +	struct arch_hw_breakpoint *bpctrl = counter_arch_bp(bp);
> +	u32 ctrl = 0;
> +
> +	/* Expose the generic UAPI bp_type values in ptrace control bits. */
> +	ctrl |= HWDEBUG_MK_TYPE(bp->attr.bp_type);
> +	ctrl |= HWDEBUG_MK_MATCH(bpctrl->match);
> +	ctrl |= HWDEBUG_MK_SELECT(bpctrl->select);
> +	ctrl |= HWDEBUG_MK_WHEN(bpctrl->time);
> +	ctrl |= HWDEBUG_MK_SIZE(bp->attr.bp_len);
> +	ctrl |= HWDEBUG_MK_CHAIN(bpctrl->chain);
> +
> +	return ctrl;
> +}

[Severity: High]
As reported by Rui Qi on v6 [1], how can a tracer disable a hardware
breakpoint or watchpoint slot, or write a user_hwdebug_state buffer with
unused zeroed slots, when ptrace_hbp_fill_attr_ctrl() unconditionally sets
attr->disabled = 0?

The control word has no separate enable bit, so an unused or disabled slot
has control == 0 (bpctrl->type == HW_BREAKPOINT_EMPTY). Because (0 & mask) !=
0 is false, ptrace_hbp_fill_attr_ctrl() passes attr->disabled = 0 and
attr->bp_type = 0 to modify_user_hw_breakpoint(), which fails with -EINVAL in
hw_breakpoint_arch_parse() -> rv_init_mcontrol_trigger() while re-enabling an
already-active event at the new address set by ptrace_hbp_set_addr().

Conversely, encode_ctrl_reg() encodes bp->attr.bp_type without checking
bp->attr.disabled, so disabled events in a slot are reported as active by
PTRACE_GETREGSET. When uctrl == 0 (or type == HW_BREAKPOINT_EMPTY),
ptrace_hbp_fill_attr_ctrl() should set attr->disabled = 1 and return early,
and encode_ctrl_reg() should return 0 when bp->attr.disabled is set.

[ ... ]

> +static void decode_ctrl_reg(u32 uctrl, struct arch_hw_breakpoint *bpctrl)
> +{
> +	bpctrl->type = HWDEBUG_TYPE(uctrl);
> +	bpctrl->match = HWDEBUG_MATCH(uctrl);
> +	bpctrl->select = HWDEBUG_SELECT(uctrl);
> +	bpctrl->time = HWDEBUG_WHEN(uctrl);
> +	bpctrl->len = HWDEBUG_SIZE(uctrl);
> +	bpctrl->chain = HWDEBUG_CHAIN(uctrl);
> +}
> +
> +static int ptrace_hbp_set_ctrl(enum riscv_hbp_type hbp_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 bpctrl;
> +
> +	bp = ptrace_hbp_get_initialised_bp(hbp_type, tsk, idx);
> +	if (IS_ERR(bp)) {
> +		err = PTR_ERR(bp);
> +		return err;
> +	}
> +
> +	attr = bp->attr;
> +	decode_ctrl_reg(uctrl, &bpctrl);
> +	bpctrl.address = attr.bp_addr;
> +	err = ptrace_hbp_fill_attr_ctrl(hbp_type, &bpctrl, &attr);
> +	if (err)
> +		return err;
> +
> +	return modify_user_hw_breakpoint(bp, &attr);
> +}

[Severity: Medium]
Does ptrace_hbp_set_ctrl() silently discard the match, select, time, and
chain fields decoded by decode_ctrl_reg(), as well as any non-zero reserved
bits in uctrl?

decode_ctrl_reg() unpacks HWDEBUG_MATCH, HWDEBUG_SELECT, HWDEBUG_WHEN, and
HWDEBUG_CHAIN into the stack-local bpctrl, but ptrace_hbp_fill_attr_ctrl()
only copies len, type, and address into attr, and modify_user_hw_breakpoint()
re-initializes counter_arch_bp(bp) from attr alone. Any non-zero values
written to those fields or to reserved bits [31:17] are accepted without
taking effect and read back as 0 in encode_ctrl_reg(), preventing userspace
from detecting unsupported trigger modes.

> +static int ptrace_hbp_set_addr(enum riscv_hbp_type hbp_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(hbp_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;
> +}

[Severity: Medium]
Does ptrace_hbp_set_addr() leave a newly created perf_event registered in
tsk->thread.debug when modify_user_hw_breakpoint() or the subsequent control
write in riscv_hw_break_set() fails?

When a slot is initially empty, ptrace_hbp_get_initialised_bp() calls
ptrace_hbp_create(), which registers a disabled perf_event and reserves one
of the task's shared hardware trigger slots via reserve_bp_slot(). If
modify_user_hw_breakpoint() fails (for example on an address >= TASK_SIZE) or
riscv_hw_break_set() fails while copying or applying ctrl, the event is not
unregistered, exhausting the task's hardware trigger slots until task exit or
execve.

> +#define PTRACE_HBP_ADDR_SZ	sizeof(u64)
> +#define PTRACE_HBP_CTRL_SZ	sizeof(u32)
> +#define PTRACE_HBP_PAD_SZ	sizeof(u32)
> +
> +static int riscv_hw_break_get(struct task_struct *target,
> +			      const struct user_regset *regset,
> +			      struct membuf to)
> +{
> +	unsigned int note_type = regset->core_note_type;
> +	enum riscv_hbp_type hbp_type;
> +	int ret, idx, num_slots;
> +	u32 info, ctrl;
> +	u64 addr;
> +
> +	ret = ptrace_hbp_note_to_type(note_type, &hbp_type);
> +	if (ret)
> +		return ret;
> +
> +	/* Resource info: number of available slots */
> +	ret = ptrace_hbp_get_resource_info(hbp_type, &info);
> +	if (ret)
> +		return ret;
> +
> +	membuf_write(&to, &info, sizeof(info));
> +	membuf_zero(&to, sizeof(u32));
> +
> +	/* Emit one (address, ctrl, pad) entry per available slot */
> +	num_slots = (int)info;
> +	for (idx = 0; idx < num_slots; idx++) {
> +		ret = ptrace_hbp_get_addr(hbp_type, target, idx, &addr);
> +		if (ret)
> +			return ret;
> +		ret = ptrace_hbp_get_ctrl(hbp_type, target, idx, &ctrl);
> +		if (ret)
> +			return ret;
> +		membuf_store(&to, addr);
> +		membuf_store(&to, ctrl);
> +		membuf_zero(&to, sizeof(u32));
> +	}
> +	return 0;
> +}
> +
> +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)
> +{

[ ... ]

> +	/* (address, ctrl) registers */
> +	limit = regset->n * regset->size;
> +	while (count && offset < limit) {

[Severity: Medium]
Should riscv_hw_break_get() return to.left instead of 0, and should
riscv_hw_break_set() bound writes by num_slots rather than all 16 entries in
struct user_hwdebug_state?

__regset_get() computes the number of bytes produced as size - res, so
returning 0 after writing only num_slots entries causes PTRACE_GETREGSET and
ELF core dumps to report all 16 slots (264 bytes) as populated even when
hw_breakpoint_slots() is smaller. Meanwhile, riscv_hw_break_set() allows
programming slot indices num_slots <= idx < RISCV_MAX_BP, creating active
breakpoints that riscv_hw_break_get() never reads back.

[ ... ]

> @@ -919,6 +1237,28 @@ static const struct user_regset compat_riscv_user_regset[] = {
>  		.set = riscv_fpr_set,
>  	},
>  #endif
> +#ifdef CONFIG_HAVE_HW_BREAKPOINT
> +	/*
> +	 * The hardware-debug regset uses fixed-width UAPI fields, so the
> +	 * native layout is also the compat layout.
> +	 */
> +	[REGSET_RISCV_HW_BREAK] = {
> +		USER_REGSET_NOTE_TYPE(RISCV_HW_BREAK),
> +		.n = sizeof(struct user_hwdebug_state) / sizeof(u32),
> +		.size = sizeof(u32),
> +		.align = sizeof(u32),
> +		.regset_get = riscv_hw_break_get,
> +		.set = riscv_hw_break_set,
> +	},
> +	[REGSET_RISCV_HW_WATCH] = {
> +		USER_REGSET_NOTE_TYPE(RISCV_HW_WATCH),
> +		.n = sizeof(struct user_hwdebug_state) / sizeof(u32),
> +		.size = sizeof(u32),
> +		.align = sizeof(u32),
> +		.regset_get = riscv_hw_break_get,
> +		.set = riscv_hw_break_set,
> +	},
> +#endif
>  };

[Severity: Medium]
Does indexing compat_riscv_user_regset[] with REGSET_RISCV_HW_BREAK and
REGSET_RISCV_HW_WATCH create zero-initialized struct user_regset holes that
cause a division by zero in ptrace_regset()?

In enum riscv_regset, REGSET_RISCV_HW_BREAK and REGSET_RISCV_HW_WATCH follow
REGSET_V, REGSET_TAGGED_ADDR_CTRL, and REGSET_CFI, which are omitted from
compat_riscv_user_regset[]. When any of CONFIG_RISCV_ISA_V,
CONFIG_RISCV_ISA_SUPM, or CONFIG_RISCV_USER_CFI is enabled, those intermediate
slots in compat_riscv_user_regset[] are zero-filled with core_note_type == 0
and size == 0. Calling ptrace(PTRACE_GETREGSET, pid, 0, &iov) on a compat
task matches the hole in find_regset(view, 0) and evaluates kiov->iov_len %
regset->size with regset->size == 0 in kernel/ptrace.c:ptrace_regset().

[1] https://lore.kernel.org/r/5a7c08bd-4313-4424-b649-a19ffe5242e4@bytedance.com

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