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

Hi all,

thanks for the patch!

In fact I was thinking about entirely removing this driver.

I am not aware if anyone still uses this over 30 y/o hardware in 2026.
Since I changed my department in 2017 my last Softing CANCard was 
accidentally thrown away at a clean up attempt.

IMO neither this PCMCIA Card from 1994 nor some Linux hardware with 
PCMCIA slot would still run Linux 7.2 today where a 13€ CANdelight USB 
adapter would do a better job.

So thanks but I tend to remove the softing driver instead of creating 
the effort for a stable patch series.

@Marc/Vincent: I still have two SJA1000 memory-mapped (no firmware!) 
PCMCIA CAN cards from PEAK and EMS Wuensche on my desk - and a Centrino 
Laptop from 2006 with PCMCIA slot. But I wonder if we should remove the 
entire PCMCIA CAN drivers while we are at it?

The last fix that I reviewed and tested on my Laptop was from 2021:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit?id=3ec6ca6b1a8e64389f0212b5a1b0f6fed1909e45

Best regards,
Oliver


On 22.07.26 06:43, Pengpeng Hou wrote:
> fw_parse() reads a fixed record header, a firmware-provided payload,
> and a trailing checksum without knowing the end of the firmware blob. A
> truncated record can therefore make those reads exceed the blob.
> 
> The same record also supplies addresses and lengths for writes into
> DPRAM. The generic loader uses wrap-prone mixed signed arithmetic for its
> bounds check, while the application loader does not bound the staging
> copy at all.
> 
> Pass the firmware end to the parser and validate the full source record.
> Use a signed wide offset for generic DPRAM records and validate the
> application staging span against the mapped DPRAM before copying.
> 
> Fixes: 03fd3cf5a179 ("can: add driver for Softing card")
> Signed-off-by: Pengpeng Hou <[email protected]>
> ---
>   drivers/net/can/softing/softing_fw.c | 46 +++++++++++++++++++---------
>   1 file changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/can/softing/softing_fw.c b/drivers/net/can/softing/softing_fw.c
> index 721df91cdbfb..282570daf3ef 100644
> --- a/drivers/net/can/softing/softing_fw.c
> +++ b/drivers/net/can/softing/softing_fw.c
> @@ -91,12 +91,12 @@ int softing_bootloader_command(struct softing *card, int16_t cmd,
>   	return ret;
>   }
>   
> -static int fw_parse(const uint8_t **pmem, uint16_t *ptype, uint32_t *paddr,
> -		uint16_t *plen, const uint8_t **pdat)
> +static int fw_parse(const u8 **pmem, const u8 *limit, u16 *ptype,
> +		    u32 *paddr, u16 *plen, const u8 **pdat)
>   {
>   	uint16_t checksum[2];
> -	const uint8_t *mem;
> -	const uint8_t *end;
> +	const u8 *mem;
> +	const u8 *record_end;
>   
>   	/*
>   	 * firmware records are a binary, unaligned stream composed of:
> @@ -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]);
> +	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;
>   	if (checksum[0] != checksum[1])
>   		return -EINVAL;
> @@ -139,6 +146,7 @@ int softing_load_fw(const char *file, struct softing *card,
>   	uint16_t type, len;
>   	uint32_t addr;
>   	uint8_t *buf = NULL, *new_buf;
> +	s64 dpram_offset;
>   	int buflen = 0;
>   	int8_t type_end = 0;
>   
> @@ -153,7 +161,7 @@ int softing_load_fw(const char *file, struct softing *card,
>   	mem = fw->data;
>   	end = &mem[fw->size];
>   	/* look for header record */
> -	ret = fw_parse(&mem, &type, &addr, &len, &dat);
> +	ret = fw_parse(&mem, end, &type, &addr, &len, &dat);
>   	if (ret < 0)
>   		goto failed;
>   	if (type != 0xffff)
> @@ -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);
>   		if (ret < 0)
>   			goto failed;
>   		if (type == 3) {
> @@ -179,9 +187,13 @@ int softing_load_fw(const char *file, struct softing *card,
>   			goto failed;
>   		}
>   
> -		if ((addr + len + offset) > size)
> +		dpram_offset = (s64)addr + offset;
> +		if (dpram_offset < 0 || dpram_offset > size ||
> +		    len > size - dpram_offset) {
> +			ret = -EINVAL;
>   			goto failed;
> -		memcpy_toio(&dpram[addr + offset], dat, len);
> +		}
> +		memcpy_toio(&dpram[dpram_offset], dat, len);
>   		/* be sure to flush caches from IO space */
>   		mb();
>   		if (len > buflen) {
> @@ -195,7 +207,7 @@ int softing_load_fw(const char *file, struct softing *card,
>   			buf = new_buf;
>   		}
>   		/* verify record data */
> -		memcpy_fromio(buf, &dpram[addr + offset], len);
> +		memcpy_fromio(buf, &dpram[dpram_offset], len);
>   		if (memcmp(buf, dat, len)) {
>   			/* is not ok */
>   			dev_alert(&card->pdev->dev, "DPRAM readback failed\n");
> @@ -237,7 +249,7 @@ int softing_load_app_fw(const char *file, struct softing *card)
>   	mem = fw->data;
>   	end = &mem[fw->size];
>   	/* look for header record */
> -	ret = fw_parse(&mem, &type, &addr, &len, &dat);
> +	ret = fw_parse(&mem, end, &type, &addr, &len, &dat);
>   	if (ret)
>   		goto failed;
>   	ret = -EINVAL;
> @@ -253,7 +265,7 @@ int softing_load_app_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);
>   		if (ret)
>   			goto failed;
>   
> @@ -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]);