[PATCH] can: softing: validate firmware record spans

[PATCH] can: softing: validate firmware record spans

[Pengpeng Hou]

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 <pengpeng@iscas.ac.cn>
---
 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]);
-- 
2.50.1 (Apple Git-155)

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

[sashiko-bot]

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 <pengpeng@iscas.ac.cn>
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/20260722044347.2708-1-pengpeng@iscas.ac.cn?part=1

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

[Oliver Hartkopp]

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 <pengpeng@iscas.ac.cn>
> ---
>   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]);