Re: [PATCH] can: softing: validate firmware record spans

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

Pre-existing issues:
- [High] The firmware parser `fw_parse()` incorrectly uses `le16_to_cpup()` and `le32_to_cpup()` to extract fields from a packed binary stream that is typically unaligned.
- [High] The firmware loading functions silently return 0 (success) on failure paths when the firmware is missing an EOF or entrypoint record.
--

--- Patch [1]: [PATCH] can: softing: validate firmware record spans ---
commit 68a01b63a75d8a34f173d1c1f6e766add81e4a94
Author: Pengpeng Hou <[email protected]>
Subject: can: softing: validate firmware record spans

This commit adds bounds checking for firmware records parsed by fw_parse()
in the softing CAN driver. It validates that read operations do not exceed
the firmware blob and that DPRAM write operations fall within mapped memory
boundaries.

> 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 isn't a bug introduced by this patch, but does fw_parse() incorrectly
use le16_to_cpup() and le32_to_cpup() to extract fields from a packed binary
stream that is typically unaligned?

The author explicitly commented that leXX_to_cpup() was chosen to solve
alignment, but these macros directly cast and dereference pointers, which
faults on unaligned addresses on strict-alignment architectures.

Should this use the safe <asm/unaligned.h> helpers like get_unaligned_le16()
instead here and when reading the trailing checksum below?

[ ... ]

> @@ -164,7 +172,7 @@ int softing_load_fw(const char *file, struct softing *card,
>  	}
>  	/* ok, we had a header */
>  	while (mem < end) {
> -		ret = fw_parse(&mem, &type, &addr, &len, &dat);
> +		ret = fw_parse(&mem, end, &type, &addr, &len, &dat);

[Severity: High]
This is a pre-existing issue, but do softing_load_fw() and 
softing_load_app_fw() silently return 0 (success) on failure paths when
the firmware is missing an EOF or entrypoint record?

If the loop over the firmware records terminates normally because the
mandatory EOF or entrypoint is missing, the code subsequently triggers
a goto failed:

	if (!type_end)
		/* no end record seen */
		goto failed;

However, ret was set to 0 by the last successful fw_parse() loop 
iteration. Since no negative error code (e.g., -EINVAL) is assigned
before jumping to the error label, does the function incorrectly return 0?

>  		if (ret < 0)
>  			goto failed;
>  		if (type == 3) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1