Re: [PATCH v3 08/17] riscv: Use generated instruction headers for misaligned loads/stores

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Infinite trap loop (DoS) and lost updates on misaligned loads to `zero` or `f0` due to skipped `epc` advancement.
- [Critical] Uninitialized kernel stack leak and Undefined Behavior on 32-bit systems via instruction TOCTOU.
- [High] Silent data corruption on misaligned stores from `zero` or `f0` registers.
- [High] Loss of sign extension on signed misaligned loads causes data corruption.
--

--- Patch [8]: [PATCH v3 08/17] riscv: Use generated instruction headers for misaligned loads/stores ---
commit 9243cf56f2dcd4d2a9e3d43d550df4f5779ccbf5
Author: Charlie Jenkins <thecharlesjenkins@gmail.com>

riscv: Use generated instruction headers for misaligned loads/stores

This patch migrates the misaligned loads/store code to use the generated
instruction headers. It replaces the hand-written instruction composition
functions with the new generated helpers.

> diff --git a/arch/riscv/kernel/traps_misaligned.c b/arch/riscv/kernel/traps_misaligned.c
> --- a/arch/riscv/kernel/traps_misaligned.c
> +++ b/arch/riscv/kernel/traps_misaligned.c
> @@ -238,68 +217,71 @@ static int handle_scalar_misaligned_load(struct pt_regs *regs)
>  	if (get_insn(regs, epc, &insn))
>  		return -1;
>  
> -	if ((insn & INSN_MASK_LW) == INSN_MATCH_LW) {
> +	if (riscv_insn_is_lw(insn)) {
>  		len = 4;
>  		shift = 8 * (sizeof(unsigned long) - len);
> -#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;
>  		shift = 8 * (sizeof(unsigned long) - len);

[Severity: Critical]
Since the CONFIG_64BIT guard has been removed, what happens on 32-bit
systems if a userspace thread replaces the instruction with a 64-bit ld
before the kernel reads it? Does this result in a negative shift value
(-32) since sizeof(unsigned long) is 4 and len is 8, leading to undefined
behavior?

[ ... ]

> @@ -315,14 +297,16 @@ static int handle_scalar_misaligned_load(struct pt_regs *regs)
>  		memcpy(&val, (u8 *)addr, len);
>  	}
>  
> -	if (!fp)
> -		SET_RD(insn, regs, (long)(val.data_ulong << shift) >> shift);
> -	else if (len == 8)
> -		set_f64_rd(insn, regs, val.data_u64);
> -	else
> -		set_f32_rd(insn, regs, val.data_ulong);
> +	if (rd) {
> +		if (!fp)
> +			*(unsigned long *)((unsigned long *)regs + rd) = (val.data_ulong << shift) >> shift;

[Severity: High]
Does this lose sign extension for signed misaligned loads? The previous code
used a (long) cast to ensure an arithmetic right shift. Without the cast,
doesn't this perform a logical right shift and incorrectly zero-extend
negative values?

> +		else if (len == 8)
> +			set_f64_rd(rd, regs, val.data_u64);
> +		else
> +			set_f32_rd(rd, regs, val.data_ulong);
>  
> -	regs->epc = epc + INSN_LEN(insn);
> +		regs->epc = epc + INSN_LEN(insn);
> +	}
>  
>  	return 0;
>  }

[Severity: Critical]
If the target register is x0 (zero) or f0, rd will be 0 and this entire
block is skipped. Does this mean regs->epc is never advanced for loads to
these zero registers, resulting in an infinite trap loop and kernel lockup?

[ ... ]

> @@ -346,71 +330,76 @@ static int handle_scalar_misaligned_store(struct pt_regs *regs)
>  	if (get_insn(regs, epc, &insn))
>  		return -1;
>  
> -	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_SD) == INSN_MATCH_SD) {
> +		rd = riscv_insn_sw_extract_xs2(insn);
> +	} else if (riscv_insn_is_sd(insn)) {
>  		len = 8;
> -#endif

[Severity: Critical]
Similar to the load case, on a 32-bit kernel, parsing sd sets len to 8
because the CONFIG_64BIT guard was removed. Later in this function,
val.data_ulong is populated (which is 4 bytes on 32-bit), but 8 bytes are
subsequently copied. Could this leak 4 bytes of uninitialized kernel stack
memory to userspace?

[ ... ]

> -	if (user_mode(regs)) {
> -		if (copy_to_user((u8 __user *)addr, &val, len))
> -			return -1;
> -	} else {
> -		memcpy((u8 *)addr, &val, len);
> +	if (rd) {
> +		if (!fp)
> +			val.data_ulong = *(unsigned long *)((unsigned long *)regs + rd);
> +		else if (len == 8)
> +			val.data_u64 = get_f64_rs(rd, regs);
> +		else
> +			val.data_ulong = get_f32_rs(rd, regs);
> +
> +		if (user_mode(regs)) {
> +			if (copy_to_user((u8 __user *)addr, &val, len))
> +				return -1;
> +		} else {
> +			memcpy((u8 *)addr, &val, len);
> +		}
>  	}
>  
>  	regs->epc = epc + INSN_LEN(insn);

[Severity: High]
By gating the memory copy operations inside the if (rd) check, doesn't this
silently drop stores where the source register is x0 or f0 instead of
actually writing to the misaligned address?

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