Re: [PATCH v8 6/8] riscv: Add shared instruction fetch and decode helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] riscv: missing s32 sign extension of offsets in
  get_next_insn_address()
- [Low] riscv: duplicate include of asm/insn.h in traps_misaligned.c
--

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

riscv: Add shared instruction fetch and decode helpers

This commit moves get_insn() into arch/riscv/lib/insn.c and adds helpers to
fetch instructions and compute the next PC for control-flow instructions
during software single-stepping.

> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
> index 6e8ae6c66322c..541e6a5b80394 100644
> --- a/arch/riscv/kernel/traps_misaligned.c
> +++ b/arch/riscv/kernel/traps_misaligned.c
> @@ -10,6 +10,7 @@
>  #include <linux/irq.h>
>  #include <linux/stringify.h>
>  
> +#include <asm/insn.h>
>  #include <asm/processor.h>
>  #include <asm/ptrace.h>
>  #include <asm/csr.h>

[Severity: Low]
This isn't a bug, but arch/riscv/kernel/traps_misaligned.c already includes
asm/insn.h a few lines below.

[ ... ]

> diff --git a/arch/riscv/lib/insn.c b/arch/riscv/lib/insn.c
> new file mode 100644
> index 0000000000000..fc44e2aa6f5f0
> --- /dev/null
> +++ b/arch/riscv/lib/insn.c

[ ... ]

> +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 riscv_jalr_target(riscv_get_reg_value(regs, rs1_num), 0);
> +	}
> +
> +	if (riscv_insn_is_c_j(insn) || riscv_insn_is_c_jal(insn))
> +		return RVC_EXTRACT_JTYPE_IMM(insn) + pc;
> +
> +	if (riscv_insn_is_c_beqz(insn)) {
> +		rs1_num = RV_X(insn, RVC_C1_RS1_OPOFF, 3) + 8;
> +		if (!rs1_num || riscv_get_reg_value(regs, rs1_num) == 0)
> +			return RVC_EXTRACT_BTYPE_IMM(insn) + pc;
> +		return pc + 2;
> +	}
> +
> +	if (riscv_insn_is_c_bnez(insn)) {
> +		rs1_num = RV_X(insn, RVC_C1_RS1_OPOFF, 3) + 8;
> +		if (rs1_num && riscv_get_reg_value(regs, rs1_num) != 0)
> +			return RVC_EXTRACT_BTYPE_IMM(insn) + pc;
> +		return pc + 2;
> +	}
> +
> +	return pc + 2;
> +}

[ ... ]

> +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 riscv_jalr_target(riscv_get_reg_value(regs, rs1_num),
> +					 RV_EXTRACT_ITYPE_IMM(insn));
> +	}

[Severity: High]
On 64-bit RISC-V, do RVC_EXTRACT_*_IMM(insn) and RV_EXTRACT_*_IMM(insn)
zero-extend negative branch and jump offsets to 64 bits instead of
sign-extending them?

Because insn is u32, RV_IMM_SIGN(x_) and RVC_IMM_SIGN(x_) evaluate in 32-bit
unsigned arithmetic (producing a 32-bit unsigned mask such as 0xfffff000U),
and bitwise-ORing that with the GENMASK()-based unsigned long bitfields
zero-extends the result to 64-bit unsigned long. Any backward branch,
backward jump, or negative-offset JALR in get_next_insn_address_compressed()
and get_next_insn_address_standard() will miscompute the next PC by +4 GiB,
arming software single-step breakpoints at the wrong address.

Casting the extracted immediates to s32 before adding pc or passing the
offset to riscv_jalr_target() (as done in simulate-insn.c) would sign-extend
them properly.

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