Re: [PATCH net 16/20] can: softing: fw_parse(): validate firmware record spans

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unaligned memory dereference in `fw_parse()` triggers alignment faults.
- [High] Truncated or malformed firmware causes silent failure by returning 0 on error paths.
- [High] Unaligned MMIO accesses to DPRAM via `iowrite32` cause hardware exceptions.
--

--- Patch [16]: [PATCH net 16/20] can: softing: fw_parse(): validate firmware record spans ---
commit dd9e23c1084e29145c02ec85df264fc5faf868da
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

can: softing: fw_parse(): validate firmware record spans

This commit improves bounds checking when parsing softing firmware records
to prevent out-of-bounds reads and writes. It passes the firmware end to
the parser and validates the source record length before doing DPRAM staging
copies.

Link: https://patch.msgid.link/20260722044347.2708-1-pengpeng@iscas.ac.cn

> diff --git a/drivers/net/can/softing/softing_fw.c b/drivers/net/can/softing/softing_fw.c
> index 721df91cdbfbc..282570daf3efe 100644
> --- a/drivers/net/can/softing/softing_fw.c
> +++ b/drivers/net/can/softing/softing_fw.c

[ ... ]

> @@ -114,14 +114,21 @@ static int fw_parse(const uint8_t **pmem, uint16_t *ptype, uint32_t *paddr,
>  	 * endianness & alignment.
>  	 */
>  	mem = *pmem;
> +	/* A record needs an 8-byte prefix and a 2-byte checksum. */
> +	if (mem > limit || limit - mem < 10)
> +		return -EINVAL;
> +
>  	*ptype = le16_to_cpup((void *)&mem[0]);
>  	*paddr = le32_to_cpup((void *)&mem[2]);
>  	*plen = le16_to_cpup((void *)&mem[6]);

[Severity: High]
This is a pre-existing issue, but does using le16_to_cpup() and
le32_to_cpup() on an unaligned byte stream cause alignment faults?

Firmware records are packed without padding, meaning mem could rest on an odd
address. The leXX_to_cpup functions directly dereference the pointer, which
can trigger a fatal alignment fault exception (kernel oops) on architectures
that lack hardware unaligned access support. Should fw_parse() use
get_unaligned_le16() and get_unaligned_le32() here instead?

> +	if (*plen > limit - mem - 10)
> +		return -EINVAL;
> +
>  	*pdat = &mem[8];
>  	/* verify checksum */
> -	end = &mem[8 + *plen];
> -	checksum[0] = le16_to_cpup((void *)end);
> -	for (checksum[1] = 0; mem < end; ++mem)
> +	record_end = &mem[8 + *plen];
> +	checksum[0] = le16_to_cpup((void *)record_end);
> +	for (checksum[1] = 0; mem < record_end; ++mem)
>  		checksum[1] += *mem;

[ ... ]

> @@ -279,6 +291,12 @@ int softing_load_app_fw(const char *file, struct softing *card)
>  		/* work in 16bit (target) */
>  		sum &= 0xffff;
>  
> +		if (card->pdat->app.offs > card->dpram_size ||
> +		    len > card->dpram_size - card->pdat->app.offs) {
> +			ret = -EINVAL;
> +			goto failed;
> +		}
> +
>  		memcpy_toio(&card->dpram[card->pdat->app.offs], dat, len);
>  		iowrite32(card->pdat->app.offs + card->pdat->app.addr,
>  				&card->dpram[DPRAM_COMMAND + 2]);

[Severity: High]
This is a pre-existing issue, but does using iowrite32() on unaligned
offsets cause hardware exceptions?

These iowrite32() calls target DPRAM addresses shifted by unaligned offsets
like DPRAM_COMMAND + 2 and DPRAM_COMMAND + 6. Standard 32-bit I/O accessors
mandate 4-byte alignment on the target memory address. Executing an
unaligned MMIO write triggers a hardware alignment fault or bus error on
non-x86 architectures. Should these operations be split into sequential
16-bit writes using iowrite16()?

[Severity: High]
This is a pre-existing issue, but does a truncated or malformed firmware
blob cause a silent failure by returning 0?

In both softing_load_fw() and softing_load_app_fw(), if the firmware blob
does not contain an EOF record or an entrypoint record, the parsing loop
terminates without encountering an error:

softing_fw.c:softing_load_app_fw() {
    ...
    while (mem < end) {
        ret = fw_parse(&mem, end, &type, &addr, &len, &dat);
        if (ret)
            goto failed;
        ...
    }
    if (!type_end || !type_entrypoint)
        goto failed;
    ...
failed:
    ...
    return ret;
}

Because fw_parse() returns 0 on success, ret is left at 0 when the loop
finishes naturally. The code then detects the missing record and jumps to
the failed label, but since ret is 0, it incorrectly returns success to
the caller. This leaves the device uninitialized. Could ret be set to an
error code like -EINVAL before the goto?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729102802.505168-1-mkl@pengutronix.de?part=16