Re: [PATCH v7 1/8] riscv: Introduce support for hardware break/watchpoints

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] perf/riscv: deadlock and IRQ state corruption via per-CPU lock flags
- [High] perf/riscv: hardware trigger hit bit is not cleared causing stuck triggers
- [High] perf/riscv: arch_update_hw_breakpoint passes zero count and corrupts memory
- [High] perf/riscv: use-after-free of sbi_dbtr_shmem on hotplug setup failure
- [Medium] perf/riscv: hardware triggers are leaked on software allocation failure
- [Medium] perf/riscv: integer underflow in arch_check_bp_in_kernelspace
- [Medium] perf/riscv: race condition in arch_uninstall_hw_breakpoint crashes kernel
- [Medium] perf/riscv: uninitialized tstate passed to hypervisor on trigger install
- [Low] perf/riscv: arch_hw_breakpoint_init returns positive integer on success
--

commit 43e04941130b3d79825bf1347e698fc0ed2e7a0d
Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>

riscv: Introduce support for hardware break/watchpoints

This commit introduces hardware breakpoint and watchpoint support for RISC-V.
It builds on the perf subsystem and uses the SBI debug trigger extension to
manage hardware triggers.

> diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
> --- /dev/null
> +++ b/arch/riscv/kernel/hw_breakpoint.c
> @@ -0,0 +1,746 @@
[ ... ]
> +/* Registered per-cpu bp/wp */
> +static DEFINE_PER_CPU(struct perf_event *, pcpu_hw_bp_events[RISCV_HW_BP_NUM_MAX]);
> +static DEFINE_PER_CPU(unsigned long, ecall_lock_flags);

[Severity: High]
Does defining ecall_lock_flags as a global per-CPU variable cause IRQ state
corruption? The raw_spin_lock_irqsave() macro expects a local stack variable
to save the IRQ state, but this variable is shared globally across the CPU.

> +static DEFINE_PER_CPU(raw_spinlock_t, ecall_lock);
[ ... ]
> +int arch_check_bp_in_kernelspace(struct arch_hw_breakpoint *hw)
> +{
> +	unsigned int len;
> +	unsigned long va;
> +
> +	va = hw->address;
> +	len = hw->len;
> +
> +	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);

[Severity: Medium]
For execute breakpoints, len is 0. Does va + len - 1 underflow, causing the
bounds check to wrap around when added to a 64-bit kernel address? This
might evaluate to less than TASK_SIZE and erroneously return false.

> +}
[ ... ]
> +static int hw_breakpoint_handler(struct die_args *args)
> +{
> +	int ret = NOTIFY_DONE;
> +	struct arch_hw_breakpoint *bp;
> +	struct perf_event *event;
> +	int i;
> +
> +	for (i = 0; i < dbtr_total_num; i++) {
> +		event = this_cpu_read(pcpu_hw_bp_events[i]);
> +		if (!event)
> +			continue;
> +
> +		bp = counter_arch_bp(event);
> +		switch (bp->type) {
> +		/* Breakpoint */
> +		case HW_BREAKPOINT_X:
> +		{
> +			bool hit = bp->address == args->regs->epc;
> +
> +			if (!hit && dbtr_type == RISCV_DBTR_TRIG_MCONTROL6)
> +				hit = mc6_read_and_clear_hit(i) != RISCV_DBTR_MC6_HIT_FALSE;

[Severity: High]
If the address matches, this short-circuit logic prevents calling
mc6_read_and_clear_hit(i). Does skipping this leave the hardware hit bit
stuck at 1, causing endless spurious traps on unrelated exceptions?

> +
> +			if (hit) {
> +				perf_bp_event(event, args->regs);
> +				ret = NOTIFY_STOP;
> +			}
> +			break;
> +		}
[ ... ]
> +			if (dbtr_type == RISCV_DBTR_TRIG_MCONTROL) {
> +				unsigned long tdata1;
> +				struct sbiret sret;
> +				union sbi_dbtr_shmem_entry *shmem;
> +
> +				raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> +						      *this_cpu_ptr(&ecall_lock_flags));

[Severity: High]
If a synchronous exception like a hardware breakpoint occurs while ecall_lock
is already held, this trap handler will attempt to acquire the exact same
non-recursive local lock and overwrite ecall_lock_flags. Could this result
in an immediate inescapable deadlock?

> +				shmem = this_cpu_ptr(sbi_dbtr_shmem);
> +				sret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_READ,
> +						 i, 1, 0, 0, 0, 0);
> +				if (!sret.error) {
> +					tdata1 = le_to_cpu(shmem->data.tdata1);
> +					hit = !!(tdata1 & RISCV_DBTR_MC_HIT_BIT_MASK);

[Severity: High]
For MCONTROL triggers, the hit bit is read here, but it doesn't appear to be
cleared in tdata1 or updated via SBI_EXT_DBTR_TRIG_UPDATE. Does this leave
the hardware state corrupted with the hit bit permanently set?

> +				}
> +				raw_spin_unlock_irqrestore(this_cpu_ptr(&ecall_lock),
> +							   *this_cpu_ptr(&ecall_lock_flags));
> +			}
[ ... ]
> +/* atomic: counter->ctx->lock is held */
> +int arch_install_hw_breakpoint(struct perf_event *event)
> +{
> +	struct arch_hw_breakpoint *bp = counter_arch_bp(event);
> +	union sbi_dbtr_shmem_entry *shmem = this_cpu_ptr(sbi_dbtr_shmem);
> +	struct sbi_dbtr_data_msg *xmit;
> +	struct sbi_dbtr_id_msg *recv;
> +	struct perf_event **slot;
> +	unsigned long idx;
> +	struct sbiret ret;
> +	int err = 0;
> +
> +	raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> +			      *this_cpu_ptr(&ecall_lock_flags));
> +
> +	xmit = &shmem->data;
> +	recv = &shmem->id;
> +	xmit->tdata1 = cpu_to_le(bp->tdata1);
> +	xmit->tdata2 = cpu_to_le(bp->tdata2);
> +	xmit->tdata3 = cpu_to_le(bp->tdata3);

[Severity: Medium]
Because shmem->id and shmem->data share the same memory via a union, does
xmit->tstate need to be initialized here? Otherwise, it might retain the
trigger index from previous recv->idx calls and pass uninitialized garbage
to the hypervisor as the trigger configuration state.

> +
> +	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_INSTALL,
> +			1, 0, 0, 0, 0, 0);
> +
> +	if (ret.error) {
> +		pr_warn("Failed to install trigger\n");
> +		err = sbi_err_map_linux_errno(ret.error);
> +		goto done;
> +	}
> +
> +	idx = le_to_cpu(recv->idx);
> +	if (idx >= dbtr_total_num) {
> +		pr_warn("Invalid trigger index %lu\n", idx);
> +		err = -EINVAL;
> +		goto done;

[Severity: Medium]
If this software validation fails, the error path jumps to done without
issuing an SBI_EXT_DBTR_TRIG_UNINSTALL. Does this permanently leak the
successfully allocated hardware trigger?

> +	}
> +
> +	slot = this_cpu_ptr(&pcpu_hw_bp_events[idx]);
> +	if (*slot) {
> +		pr_warn("Slot %lu is in use\n", idx);
> +		err = -EBUSY;
> +		goto done;

[Severity: Medium]
Similarly, does this slot check also bail out without cleaning up the
hardware state, causing a trigger leak?

> +	}
[ ... ]
> +/* atomic: counter->ctx->lock is held */
> +void arch_uninstall_hw_breakpoint(struct perf_event *event)
> +{
> +	struct sbiret ret;
> +	int i;
> +
> +	raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> +			      *this_cpu_ptr(&ecall_lock_flags));
> +
> +	for (i = 0; i < dbtr_total_num; i++) {
> +		struct perf_event **slot = this_cpu_ptr(&pcpu_hw_bp_events[i]);
> +
> +		if (*slot == event) {
> +			*slot = NULL;
> +			break;
> +		}
> +	}

[Severity: Medium]
Does clearing the software slot before uninstalling the hardware trigger
open a dangerous race window? If a kernel watchpoint fires right here before
SBI_EXT_DBTR_TRIG_UNINSTALL completes, the handler will observe a NULL slot
and return NOTIFY_DONE, causing handle_break() to crash the system with an
unhandled DIE_DEBUG trap.

> +
> +	if (i == dbtr_total_num) {
> +		pr_warn("Breakpoint not installed.\n");
> +		goto out;
> +	}
> +
> +	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_UNINSTALL,
> +			i, 1, 0, 0, 0, 0);
[ ... ]
> +void arch_update_hw_breakpoint(struct perf_event *event)
> +{
> +	struct arch_hw_breakpoint *bp = counter_arch_bp(event);
> +	union sbi_dbtr_shmem_entry *shmem = this_cpu_ptr(sbi_dbtr_shmem);
> +	struct sbi_dbtr_data_msg *xmit;
> +	struct sbi_dbtr_id_msg *id;
> +	struct perf_event **slot;
> +	struct sbiret ret;
> +	int i;
[ ... ]
> +	raw_spin_lock_irqsave(this_cpu_ptr(&ecall_lock),
> +			      *this_cpu_ptr(&ecall_lock_flags));
> +
> +	id = &shmem->id;
> +	xmit = &shmem->data;
> +	id->idx = cpu_to_le(i);
> +	xmit->tdata1 = cpu_to_le(bp->tdata1);

[Severity: High]
Writing to id->idx and xmit->tdata1 here accesses the same shared memory union.
Does writing to id->idx corrupt the aliased xmit->tstate before it is sent to
the hypervisor?

> +	xmit->tdata2 = cpu_to_le(bp->tdata2);
> +	xmit->tdata3 = cpu_to_le(bp->tdata3);
> +
> +	ret = sbi_ecall(SBI_EXT_DBTR, SBI_EXT_DBTR_TRIG_UPDATE,
> +			1, 0, 0, 0, 0, 0);

[Severity: High]
Are we invoking the update call with a hardcoded zero count in a1? This seems
like it would make the update a hardware no-op and cause breakpoint updates
to silently fail.

> +	if (ret.error)
> +		pr_warn("Failed to update trigger %d.\n", i);
[ ... ]
> +static int __init arch_hw_breakpoint_init(void)
> +{
[ ... ]
> +	/* Allocate per-cpu shared memory */
> +	sbi_dbtr_shmem = __alloc_percpu(sizeof(*sbi_dbtr_shmem) * dbtr_total_num,
> +					PAGE_SIZE);
> +
> +	if (!sbi_dbtr_shmem) {
> +		pr_warn("Failed to allocate shared memory.\n");
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/* Hotplug handler to register/unregister shared memory with SBI */
> +	rc = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +			       "riscv/hw_breakpoint:prepare",
> +			       arch_smp_setup_sbi_shmem,
> +			       arch_smp_teardown_sbi_shmem);
> +
> +	if (rc < 0) {
> +		pr_warn("Failed to setup CPU hotplug state\n");
> +		free_percpu(sbi_dbtr_shmem);
> +		return rc;
> +	}

[Severity: High]
If cpuhp_setup_state() fails, we free sbi_dbtr_shmem. However, since the PMU
has already been registered earlier in the generic subsystem, can users still
create hardware breakpoints and cause a use-after-free when they access the
freed sbi_dbtr_shmem?

> + out:
> +	return rc;

[Severity: Low]
When cpuhp_setup_state() succeeds, it returns a positive state ID which is
stored in rc. Should this initcall return 0 on success to avoid the initcall
core logging an erroneous failure message?

> +}

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