Re: [PATCH v7 5/8] RISC-V: Add fetch and decode helpers to a common file

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] riscv: raw pointer dereference in get_insn_safe() causes Oops
- [Medium] riscv: missing LSB clearing in JALR target address calculation
--

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

RISC-V: Add fetch and decode helpers to a common file

This commit adds helper functions for fetching and decoding RISC-V
instructions. It also introduces functions to retrieve register values and
calculate the next execution address based on the current PC, register states,
and control-flow instructions.

> diff --git a/arch/riscv/lib/insn.c b/arch/riscv/lib/insn.c
> new file mode 100644
> index 0000000000000..361cac7abe108
> --- /dev/null
> +++ b/arch/riscv/lib/insn.c
> @@ -0,0 +1,263 @@
[ ... ]
> +#define __fetch_insn(regs, insn, insn_addr, type)		\
> +({								\
> +	type __val;						\
> +	int __ret;						\
> +								\
> +	if (user_mode(regs))					\
> +		__ret = get_user(__val,				\
> +				 (type __user *)(insn_addr));	\
> +	else {							\
> +		__val = *(type *)(insn_addr);			\
> +		__ret = 0;					\
> +	}							\
> +								\
[ ... ]
> +int get_insn_safe(struct pt_regs *regs, ulong epc, ulong *r_insn)
> +{
> +	int ret;
> +
> +	pagefault_disable();
> +	ret = get_insn(regs, epc, r_insn);
> +	pagefault_enable();
> +
> +	return ret;
> +}

[Severity: High]
Does this raw pointer dereference in __fetch_insn() actually safely handle
faults on kernel addresses?

While get_insn_safe() disables page faults, this does not make unmapped memory
accesses safe. It just forces the fault handler to directly look up exception
tables and panic if none are found.

Because __fetch_insn() performs a raw C pointer dereference *(type *)(insn_addr)
without an exception table entry, any invalid kernel address will cause an
immediate kernel panic (Oops) instead of safely returning an error.

Should this use get_kernel_nofault() for safely reading arbitrary kernel memory?

[ ... ]
> +static unsigned long get_next_insn_address_compressed(struct pt_regs *regs, u32 insn,
> +						      unsigned long pc)
> +{
> +	unsigned int rs1_num;
> +
> +	if (riscv_insn_is_c_jalr(insn) || riscv_insn_is_c_jr(insn)) {
> +		rs1_num = RV_X(insn, RVC_C2_RS1_OPOFF, 5);
> +		return regs_get_register(regs, rs1_num * sizeof(unsigned long));
> +	}

[Severity: Medium]
Will this fail to clear the least-significant bit (LSB) of the target address?

The RISC-V ISA specification mandates that the target address for C.JALR and
C.JR instructions has its LSB set to zero by hardware. Since this function
does not apply & ~1UL to the register value, if the source register has the LSB
set, it will incorrectly return an odd (unaligned) address.

Subsystems relying on this for single-stepping might set breakpoints at the odd
address, which would fail to trigger when the CPU actually executes the aligned
target.

[ ... ]
> +static unsigned long get_next_insn_address_standard(struct pt_regs *regs, u32 insn,
> +						    unsigned long pc)
> +{
> +	unsigned int rs1_num;
> +
> +	if ((insn & __INSN_OPCODE_MASK) == __INSN_BRANCH_OPCODE)
> +		return riscv_branch_taken(regs, insn) ?
> +			RV_EXTRACT_BTYPE_IMM(insn) + pc : pc + 4;
> +
> +	if (riscv_insn_is_jal(insn))
> +		return RV_EXTRACT_JTYPE_IMM(insn) + pc;
> +
> +	if (riscv_insn_is_jalr(insn)) {
> +		rs1_num = RV_X(insn, RVG_RS1_OPOFF, 5);
> +		return RV_EXTRACT_ITYPE_IMM(insn) + riscv_get_reg_value(regs, rs1_num);
> +	}

[Severity: Medium]
Does this calculation also need to clear the LSB?

Similar to the compressed JALR handling above, JALR requires its target address
LSB to be cleared. Without applying & ~1UL, calculating the target address here
could also return an unaligned address if the LSB is set, breaking debug
control.

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