Re: [PATCH v3 05/17] riscv: kprobes: Use generated instruction headers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Simulation of C.JR and C.JALR instructions fails to clear the least-significant bit (LSB) of the target address.
--

--- Patch [5]: [PATCH v3 05/17] riscv: kprobes: Use generated instruction headers ---
commit afd6227eeae883036e027b0893e434a89b21ebab
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

riscv: kprobes: Use generated instruction headers

Migrate the code that is decoding instruction for the use of kprobes to
use the generated instruction headers instead of the hand-written
instruction functions. With the more granular instruction support, split
the decoding of branches into their own functions.

> diff --git a/arch/riscv/kernel/probes/simulate-insn.c b/arch/riscv/kernel/probes/simulate-insn.c
> index fa581590c1f8b..a443883c32370 100644
> --- a/arch/riscv/kernel/probes/simulate-insn.c
> +++ b/arch/riscv/kernel/probes/simulate-insn.c
> @@ -4,222 +4,152 @@
[ ... ]
>  bool __kprobes simulate_c_jr(u32 opcode, unsigned long addr, struct pt_regs *regs)
>  {
> -	return simulate_c_jr_jalr(opcode, addr, regs, false);
> +	unsigned long next_addr;
> +	unsigned long *regs_ptr = (unsigned long *)regs;
> +
> +	/* xs1 == 0 is invalid so riscv_insn_reg_get_val() isn't needed */
> +	next_addr = regs_ptr[riscv_insn_c_jr_extract_xs1(opcode)];
> +	instruction_pointer_set(regs, next_addr);

[Severity: High]
This is a pre-existing issue, but does this code need to clear the least
significant bit (LSB) of next_addr before updating the instruction pointer?

If a C.JR instruction executes where the source register contains an odd
address, applying it directly without masking (e.g., using & ~1) can trigger
a misaligned instruction pointer exception (a kernel Oops or userspace SIGBUS)
upon returning from the probe.

This could allow unprivileged userspace to trigger a process crash via uprobes,
breaking ABI functional correctness.

It also appears the KUnit test suite explicitly asserts this behavior in
test_kprobe_simulate_riscv(), where it expects the unmasked address rather than
the properly aligned address.

> +
> +	return true;
>  }
>  
>  bool __kprobes simulate_c_jalr(u32 opcode, unsigned long addr, struct pt_regs *regs)
>  {
> -	return simulate_c_jr_jalr(opcode, addr, regs, true);
> +	unsigned long next_addr;
> +	unsigned long *regs_ptr = (unsigned long *)regs;
> +
> +	/* xs1 == 0 is invalid so riscv_insn_reg_get_val() isn't needed */
> +	next_addr = regs_ptr[riscv_insn_c_jalr_extract_xs1(opcode)];
> +	instruction_pointer_set(regs, next_addr);

[Severity: High]
This is a pre-existing issue, but similar to C.JR, does this target address
also need to be masked before updating the instruction pointer?

> +
> +	regs->ra = addr + 2;
> +	return true;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707-riscv_insn_table-v3-0-3f8e5f75ba97@gmail.com?part=5