Re: [PATCH v7 6/8] riscv: Add software supported single stepping with mc/mc6 triggers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] riscv/hw_breakpoint: Unprivileged user can arm HW breakpoints on kernel addresses (DoS)
- [High] riscv/hw_breakpoint: HW breakpoint permanently lost on control flow divergence
- [High] riscv/hw_breakpoint: infinite trap loop if get_insn_safe fails
- [High] riscv/hw_breakpoint: setup_singlestep overrides synchronous event disablement
- [High] riscv/hw_breakpoint: CPU-wide breakpoints permanently lost on task migration
- [Medium] riscv/hw_breakpoint: state machine corruption on install failure
- [Medium] riscv/hw_breakpoint: infinite loop when single-stepping sret
- [Low] riscv/hw_breakpoint: saved_tdata fields are completely unused
- [Low] riscv/hw_breakpoint: incorrect next_addr logged due to loop variable shadowing
- [Low] riscv/hw_breakpoint: redundant bit clearing on zero-initialized fields
--

commit 39e397c6cb04690cc3a8477d767b61ec32e69fda
Author: Himanshu Chauhan <himanshu.chauhan@oss.qualcomm.com>
Subject: riscv: Add software supported single stepping with mc/mc6 triggers

This commit adds software-supported single-stepping for mc/mc6 triggers on 
RISC-V. It attempts to avoid loops by temporarily disabling the current 
breakpoint, setting a new execute breakpoint at the next instruction's 
address, and restoring the original breakpoint once the next instruction is hit.

> diff --git a/arch/riscv/kernel/hw_breakpoint.c b/arch/riscv/kernel/hw_breakpoint.c
> index dd6f639539691..9597ed3625378 100644
> --- a/arch/riscv/kernel/hw_breakpoint.c
> +++ b/arch/riscv/kernel/hw_breakpoint.c
[ ... ]
> +static int setup_singlestep(struct perf_event *event, struct pt_regs *regs)
> +{
> +	struct arch_hw_breakpoint *bp = counter_arch_bp(event);
> +	unsigned long insn, next_addr = 0;
> +	int ret;
> +	struct arch_hw_breakpoint tmp = {};
> +
> +	/*
> +	 * Save the original trigger configuration so we can restore it
> +	 * after the single-step fires.
> +	 */
> +	bp->saved_tdata1 = bp->tdata1;
> +	bp->saved_tdata2 = bp->tdata2;
> +	bp->saved_tdata3 = bp->tdata3;

[Severity: Low]
Are these saved values actually consumed? The restore logic in
hw_breakpoint_handler() seems to ignore them and completely recreates the
state from scratch via hw_breakpoint_arch_parse().

> +
> +	ret = get_insn_safe(regs, regs->epc, &insn);
> +	if (ret < 0)
> +		return ret;

[Severity: High]
What happens if get_insn_safe() fails (for example, if the user space
instruction page is swapped out, causing get_insn() to fail under
pagefault_disable())?

Since hw_breakpoint_handler() logs the error and returns NOTIFY_DONE without
advancing the PC or disabling the execute trigger, will the CPU immediately
re-trap on the exact same instruction upon resuming, leading to an infinite
loop?

> +
> +	next_addr = get_step_address(regs, insn);

[Severity: Medium]
When single-stepping an sret instruction, the underlying call to
get_next_insn_address_standard() calculates the step target to be the
current instruction itself. Since this configures an execute breakpoint
at the same address, does this cause an infinite loop where the CPU
repeatedly traps before executing the sret?

> +
> +	/*
> +	 * Software path: update the trigger in-place to an execute
> +	 * breakpoint at next_addr.  Build the tdata directly without
> +	 * calling hw_breakpoint_arch_parse() so that bp->len, bp->type
> +	 * and bp->address are not overwritten and remain valid for the
> +	 * handler's matching logic after restore.
> +	 */
> +	tmp.tdata1 = 0;
> +	tmp.tdata2 = next_addr;
> +	tmp.tdata3 = 0;
> +	switch (dbtr_type) {
> +	case RISCV_DBTR_TRIG_MCONTROL6:
> +		RISCV_DBTR_SET_MC6_EXEC_BIT(tmp.tdata1);
> +		tmp.tdata1 = RISCV_DBTR_SET_MC6_SIZE(tmp.tdata1, 0);
> +		tmp.tdata1 = RISCV_DBTR_SET_MC6_TYPE(tmp.tdata1,
> +						     RISCV_DBTR_TRIG_MCONTROL6);
> +		CLEAR_DBTR_BIT(tmp.tdata1, RISCV_DBTR_MC6_DMODE_BIT);
> +		CLEAR_DBTR_BIT(tmp.tdata1, RISCV_DBTR_MC6_TIMING_BIT);
> +		CLEAR_DBTR_BIT(tmp.tdata1, RISCV_DBTR_MC6_SELECT_BIT);
> +		CLEAR_DBTR_BIT(tmp.tdata1, RISCV_DBTR_MC6_ACTION_BIT);

[Severity: Low]
Since tmp.tdata1 is explicitly initialized to 0 above, are these
CLEAR_DBTR_BIT operations necessary? 

Also, does clearing the ACTION_BIT this way correctly handle the multi-bit
nature of this field, or does it only clear the lowest bit?

> +		CLEAR_DBTR_BIT(tmp.tdata1, RISCV_DBTR_MC6_CHAIN_BIT);
> +		CLEAR_DBTR_BIT(tmp.tdata1, RISCV_DBTR_MC6_MATCH_BIT);
> +		CLEAR_DBTR_BIT(tmp.tdata1, RISCV_DBTR_MC6_M_BIT);
> +		CLEAR_DBTR_BIT(tmp.tdata1, RISCV_DBTR_MC6_VS_BIT);
> +		CLEAR_DBTR_BIT(tmp.tdata1, RISCV_DBTR_MC6_VU_BIT);
> +		SET_DBTR_BIT(tmp.tdata1, RISCV_DBTR_MC6_S_BIT);
> +		SET_DBTR_BIT(tmp.tdata1, RISCV_DBTR_MC6_U_BIT);

[Severity: Critical]
Does this allow an unprivileged user to arm hardware breakpoints on kernel
addresses? 

Since this unconditionally sets both the Supervisor and User bits and
bypasses arch_check_bp_in_kernelspace(), could a user place a user-space
breakpoint on a jalr instruction and set the target register to a critical
kernel address (like the stvec trap vector)? If the hardware trigger fires
in Supervisor mode, could this instantly lock up the CPU in a recursive
breakpoint exception loop?

[ ... ]
> +	arch_update_hw_breakpoint(event);
> +
> +	bp->in_callback = true;
> +	bp->next_addr = next_addr;
> +	return 0;
> +}

[Severity: High]
How does this interact with CPU-wide hardware breakpoints (such as
perf record -a)? 

If the currently running task migrates to another CPU before reaching
next_addr, does the original CPU get permanently stuck waiting for an
instruction it will never execute, effectively losing the CPU-wide
breakpoint?

[ ... ]
>  static int hw_breakpoint_handler(struct die_args *args)
>  {
[ ... ]
> +		if (bp->in_callback) {
> +			expecting_callback = true;
> +			if (args->regs->epc != bp->next_addr)
> +				continue;

[Severity: High]
If execution diverges before next_addr is reached (for instance, due to an
interrupt, a signal handler, or task migration), args->regs->epc will not
match next_addr. Since the handler just bails out here, is the original
breakpoint permanently lost? Is there a fallback mechanism to recover the
lost trigger?

> +
> +			arch_uninstall_hw_breakpoint(event);
> +
> +			/* Restore original breakpoint */
> +			if (hw_breakpoint_arch_parse(NULL, &event->attr, bp))
> +				goto exit;
> +
> +			if (arch_install_hw_breakpoint(event))
> +				goto exit;

[Severity: Medium]
If arch_install_hw_breakpoint() fails here, we jump to exit without clearing
bp->in_callback = false. Will this leave the perf event alive in the software
state but corrupted for any future re-enabling?

> +
> +			bp->in_callback = false;
> +			bp_ret = NOTIFY_STOP;
> +			goto exit;
> +		}
> +
> +		switch (event->attr.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;
> +
> +				if (hit) {
> +					perf_bp_event(event, args->regs);
> +					ret = setup_singlestep(event, args->regs);

[Severity: High]
Can setup_singlestep() override a synchronous event disablement?

If perf_bp_event() triggers the perf PMU to synchronously disable the event
(e.g., if a sample limit is reached), setup_singlestep() executes immediately
afterwards. Since it unconditionally sets the enable bits and calls
arch_update_hw_breakpoint(), does this forcefully re-enable the breakpoint in
hardware in single-step mode, overriding the request to disable it?

[ ... ]
>  	}
>  
> -	return ret;
> +	if (expecting_callback) {
> +		pr_err("in_callback set but epc (%lx) not at next_addr (%lx).\n",
> +		       args->regs->epc, bp->next_addr);
> +	}

[Severity: Low]
At this point outside the loop, doesn't 'bp' hold the value of the last
event processed in the array, rather than the event that actually had
in_callback == true? Will this output an unrelated next_addr value in the
error log?

> +exit:
> +	return bp_ret;
>  }

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