Re: [PATCH v2 07/16] riscv: Use generated instruction headers for misaligned loads/stores

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Missing arithmetic right shift and sign-extension in emulated scalar misaligned loads corrupts returned data.
- [Critical] Missing +8 register index offset for compressed C-extension store instructions reads from incorrect registers.
- [High] Removal of CONFIG_64BIT guards causes overlapping RV32 floating-point and RV64 integer compressed instructions to be incorrectly decoded.
- [High] Instruction pointer (`regs->epc`) is left as 0 on unhandled store instructions.
- [Medium] Missing validation for reserved encodings in compressed SP-relative loads allows reserved instructions to be emulated.

Pre-existing issues:
- [High] Emulating misaligned stores with the zero register (x0) leaks the instruction pointer (epc) to memory.
- [High] Instruction pointer (`regs->epc`) is not restored when user memory copy fails.
--

commit ebbff49ec30db1d18deced4c1343cfcdd49aef84
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

riscv: Use generated instruction headers for misaligned loads/stores

This commit migrates the misaligned loads/store code to use the generated
instruction headers instead of the hand-written instruction composition
functions.

> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
> index 81b7682e6c6db..4d2a2432e0c45 100644
> --- a/arch/riscv/kernel/traps_misaligned.c
> +++ b/arch/riscv/kernel/traps_misaligned.c
[ ... ]
> @@ -240,68 +219,71 @@ static int handle_scalar_misaligned_load(struct pt_regs *regs)
>  
>  	regs->epc = 0;
>  
> -	if ((insn & INSN_MASK_LW) == INSN_MATCH_LW) {
> +	if (riscv_insn_is_lw(insn)) {
>  		len = 4;
[ ... ]
> -#if defined(CONFIG_64BIT)
> -	} else if ((insn & INSN_MASK_LD) == INSN_MATCH_LD) {
> +		rd = riscv_insn_lw_extract_xd(insn);
> +	} else if (riscv_insn_is_ld(insn)) {
>  		len = 8;
[ ... ]
> -	} else if ((insn & INSN_MASK_C_LD) == INSN_MATCH_C_LD) {
> +		rd = riscv_insn_lhu_extract_xd(insn);
> +	} else if (riscv_insn_is_c_ld(insn)) {
>  		len = 8;
>  		shift = 8 * (sizeof(unsigned long) - len);
> -		insn = RVC_RS2S(insn) << SH_RD;

[Severity: High]
Could this incorrectly decode a compressed floating point instruction as an
integer load on 32-bit systems? Since RV64 compressed integer instructions
and RV32 compressed floating-point instructions share the same encoding
bitmasks, removing the bitness guards and checking for RV64 instructions
first might result in a negative shift amount in
handle_scalar_misaligned_load(), leading to an invalid memory read.

> -	} else if ((insn & INSN_MASK_C_LDSP) == INSN_MATCH_C_LDSP &&
> -		   ((insn >> SH_RD) & 0x1f)) {
> +	} else if (riscv_insn_is_c_ldsp(insn)) {
>  		len = 8;
>  		shift = 8 * (sizeof(unsigned long) - len);
> -#endif
> -	} else if ((insn & INSN_MASK_C_LW) == INSN_MATCH_C_LW) {
> +		rd = riscv_insn_c_ldsp_extract_xd(insn);

[Severity: Medium]
Does this bypass validation for reserved encodings? The specification
dictates that compressed SP-relative loads must have a non-zero destination
register. The previous code enforced this, but the new branch seems to allow
processing reserved instructions as valid loads if the destination register
is zero.

[ ... ]
> @@ -319,11 +301,11 @@ static int handle_scalar_misaligned_load(struct pt_regs *regs)
>  	}
>  
>  	if (!fp)
> -		SET_RD(insn, regs, (long)(val.data_ulong << shift) >> shift);
> +		*(unsigned long *)((unsigned long *)regs + rd) = val.data_ulong << shift;

[Severity: Critical]
Does this code miss an arithmetic right shift? For loads smaller than the
native register width in handle_scalar_misaligned_load(), left-shifting the
loaded data into the most significant bits of the register without
arithmetically right-shifting it back leaves the lower bits zeroed out and
fails to sign-extend the value.

[ ... ]
> @@ -351,67 +333,68 @@ static int handle_scalar_misaligned_store(struct pt_regs *regs)
>  
>  	regs->epc = 0;
>  
> -	val.data_ulong = GET_RS2(insn, regs);
> -
> -	if ((insn & INSN_MASK_SW) == INSN_MATCH_SW) {
> +	if (riscv_insn_is_sw(insn)) {
>  		len = 4;
[ ... ]
> -#if defined(CONFIG_64BIT)
> -	} else if ((insn & INSN_MASK_C_SD) == INSN_MATCH_C_SD) {
> +		rd = riscv_insn_sh_extract_xs2(insn);
> +	} else if (riscv_insn_is_c_sd(insn)) {
>  		len = 8;
> -		val.data_ulong = GET_RS2S(insn, regs);
> -	} else if ((insn & INSN_MASK_C_SDSP) == INSN_MATCH_C_SDSP) {
> +		rd = riscv_insn_c_sd_extract_xs2(insn);

[Severity: Critical]
Could this read from the wrong registers? The load handler adds the required
offset of 8 for compressed instructions, but it appears missing here in
handle_scalar_misaligned_store() for all compressed stores.

[ ... ]
> -	} else if ((insn & INSN_MASK_C_SH) == INSN_MATCH_C_SH) {
> +	} else if (riscv_insn_is_c_sh(insn)) {
>  		len = 2;
> -		val.data_ulong = GET_RS2S(insn, regs);
> +		rd = riscv_insn_c_sh_extract_xs2(insn);
>  	} else {
> -		regs->epc = epc;
>  		return -1;
>  	}

[Severity: High]
Is it intentional to return without restoring regs->epc here? Since regs->epc
is temporarily set to 0 during instruction evaluation, returning -1 without
restoring it might cause the task's instruction pointer in the signal context
to be incorrectly reported as 0 when a SIGBUS is delivered.

>  	if (!IS_ENABLED(CONFIG_FPU) && fp)
>  		return -EOPNOTSUPP;
>  
> +	if (!fp)
> +		val.data_ulong = *(unsigned long *)((unsigned long *)regs + rd);

[Severity: High]
This isn't a bug introduced by this patch, but does this leak the instruction
pointer to memory? When a misaligned store uses the hardwired zero register,
the extracted register index is 0. Since struct pt_regs stores the epc at
index 0, the emulator writes the epc value to memory instead of 0.

Also, due to the missing offset for compressed stores mentioned earlier,
reading without the offset might cause the handler to read from kernel state
pointers like epc in pt_regs instead of the correct user registers.

[ ... ]

[Severity: High]
This is a pre-existing issue, but should we restore regs->epc when the user
memory copy fails?

In handle_scalar_misaligned_load() and handle_scalar_misaligned_store():

    if (user_mode(regs)) {
            if (copy_from_user(&val, (u8 __user *)addr, len))
                    return -1;
    }

Returning -1 on memory copy failure while regs->epc is still set to 0 breaks
user-space signal handlers because the program counter in the signal context
will be 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260621-riscv_insn_table-v2-0-7f0810ee2274@gmail.com?part=7