[PATCH 0/3] ihex: add some bounds checking to firmware parsing

[PATCH 0/3] ihex: add some bounds checking to firmware parsing

[Dan Carpenter]

These three patches go to different subsystems so hopefully the
individual maintainers can apply them.  I can resend them as
individual patches if that's easier.

The ihex firmware code is a list of records of various lengths.  The
ihex code ensures that total length of the records doesn't read
beyond the end of the fw->data[], however the parsers need to check
that individual records are not too large.

Dan Carpenter (3):
  media: gspca: Add bounds checking to firmware parser
  watchdog: ziirave_wdt: check record length in ziirave_firm_verify()
  Input: ims-pcu - Check record size in ims_pcu_flash_firmware()

 drivers/input/misc/ims-pcu.c    |  6 ++++++
 drivers/media/usb/gspca/vicam.c | 10 ++++++++--
 drivers/watchdog/ziirave_wdt.c  |  3 +++
 3 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.47.2

[PATCH 1/3] media: gspca: Add bounds checking to firmware parser

[Dan Carpenter]

This sd_init() function reads the firmware.  The firmware data holds a
series of records and the function reads each record and sends the data
to the device.  The request_ihex_firmware() function
calls ihex_validate_fw() which ensures that the total length of all the
records won't read out of bounds of the fw->data[].

However, a potential issue is if there is a single very large
record (larger than PAGE_SIZE) and that would result in memory
corruption.  Generally we trust the firmware, but it's always better to
double check.

Fixes: 49b61ec9b5af ("[media] gspca: Add new vicam subdriver")
Cc: stable@vger.kernel.org
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/media/usb/gspca/vicam.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/gspca/vicam.c b/drivers/media/usb/gspca/vicam.c
index d98343fd33fe..91e177aa8136 100644
--- a/drivers/media/usb/gspca/vicam.c
+++ b/drivers/media/usb/gspca/vicam.c
@@ -227,6 +227,7 @@ static int sd_init(struct gspca_dev *gspca_dev)
 	const struct ihex_binrec *rec;
 	const struct firmware *fw;
 	u8 *firmware_buf;
+	int len;
 
 	ret = request_ihex_firmware(&fw, VICAM_FIRMWARE,
 				    &gspca_dev->dev->dev);
@@ -241,9 +242,14 @@ static int sd_init(struct gspca_dev *gspca_dev)
 		goto exit;
 	}
 	for (rec = (void *)fw->data; rec; rec = ihex_next_binrec(rec)) {
-		memcpy(firmware_buf, rec->data, be16_to_cpu(rec->len));
+		len = be16_to_cpu(rec->len);
+		if (len > PAGE_SIZE) {
+			ret = -EINVAL;
+			break;
+		}
+		memcpy(firmware_buf, rec->data, len);
 		ret = vicam_control_msg(gspca_dev, 0xff, 0, 0, firmware_buf,
-					be16_to_cpu(rec->len));
+					len);
 		if (ret < 0)
 			break;
 	}
-- 
2.47.2

[PATCH 2/3] watchdog: ziirave_wdt: check record length in ziirave_firm_verify()

[Dan Carpenter]

The "rec->len" value comes from the firmware.  We generally do
trust firmware, but it's always better to double check.  If
the length value is too large it would lead to memory corruption
when we set "data[i] = ret;"

Fixes: 217209db0204 ("watchdog: ziirave_wdt: Add support to upload the firmware.")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/watchdog/ziirave_wdt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
index fcc1ba02e75b..5c6e3fa001d8 100644
--- a/drivers/watchdog/ziirave_wdt.c
+++ b/drivers/watchdog/ziirave_wdt.c
@@ -302,6 +302,9 @@ static int ziirave_firm_verify(struct watchdog_device *wdd,
 		const u16 len = be16_to_cpu(rec->len);
 		const u32 addr = be32_to_cpu(rec->addr);
 
+		if (len > sizeof(data))
+			return -EINVAL;
+
 		if (ziirave_firm_addr_readonly(addr))
 			continue;
 
-- 
2.47.2

Re: [PATCH 2/3] watchdog: ziirave_wdt: check record length in ziirave_firm_verify()

[Guenter Roeck]

On 5/28/25 13:22, Dan Carpenter wrote:
> The "rec->len" value comes from the firmware.  We generally do
> trust firmware, but it's always better to double check.  If
> the length value is too large it would lead to memory corruption
> when we set "data[i] = ret;"
> 
> Fixes: 217209db0204 ("watchdog: ziirave_wdt: Add support to upload the firmware.")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Reviewed-by: Guenetr Roeck <linux@roeck-us.net>

> ---
>   drivers/watchdog/ziirave_wdt.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/watchdog/ziirave_wdt.c b/drivers/watchdog/ziirave_wdt.c
> index fcc1ba02e75b..5c6e3fa001d8 100644
> --- a/drivers/watchdog/ziirave_wdt.c
> +++ b/drivers/watchdog/ziirave_wdt.c
> @@ -302,6 +302,9 @@ static int ziirave_firm_verify(struct watchdog_device *wdd,
>   		const u16 len = be16_to_cpu(rec->len);
>   		const u32 addr = be32_to_cpu(rec->addr);
>   
> +		if (len > sizeof(data))
> +			return -EINVAL;
> +
>   		if (ziirave_firm_addr_readonly(addr))
>   			continue;
>   

[PATCH 3/3] Input: ims-pcu - Check record size in ims_pcu_flash_firmware()

[Dan Carpenter]

The "len" variable comes from the firmware and we generally do
trust firmware, but it's always better to double check.  If the "len"
is too large it could result in memory corruption when we do
"memcpy(fragment->data, rec->data, len);"

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
Kees, this is a __counted_by() thing.  Would the checkers catch this?
We know the maximum valid length for "fragment" is and so it's maybe
possible to know that "fragment->len = len;" is too long?

 drivers/input/misc/ims-pcu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index d9ee14b1f451..4581f1c53644 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -844,6 +844,12 @@ static int ims_pcu_flash_firmware(struct ims_pcu *pcu,
 		addr = be32_to_cpu(rec->addr) / 2;
 		len = be16_to_cpu(rec->len);
 
+		if (len > sizeof(pcu->cmd_buf) - 1 - sizeof(*fragment)) {
+			dev_err(pcu->dev,
+				"Invalid record length in firmware: %d\n", len);
+			return -EINVAL;
+		}
+
 		fragment = (void *)&pcu->cmd_buf[1];
 		put_unaligned_le32(addr, &fragment->addr);
 		fragment->len = len;
-- 
2.47.2

Re: [PATCH 3/3] Input: ims-pcu - Check record size in ims_pcu_flash_firmware()

[Kees Cook]

On Wed, May 28, 2025 at 11:22:24PM +0300, Dan Carpenter wrote:
> The "len" variable comes from the firmware and we generally do
> trust firmware, but it's always better to double check.  If the "len"
> is too large it could result in memory corruption when we do
> "memcpy(fragment->data, rec->data, len);"
> 
> Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> Kees, this is a __counted_by() thing.  Would the checkers catch this?
> We know the maximum valid length for "fragment" is and so it's maybe
> possible to know that "fragment->len = len;" is too long?

I see:

pcu->cmd_buf as:

        u8 cmd_buf[IMS_PCU_BUF_SIZE];

and fragment is:

struct ims_pcu_flash_fmt {
        __le32 addr;
        u8 len;
        u8 data[] __counted_by(len);
};

I assume you're asking about this line:

		fragment->len = len;

I'm not aware of any compiler instrumentation that would bounds check
this -- it was designed to trust these sort of explicit assignments.

This is hardly the only place in the kernel doing this kind of
deserialization into a flexible array structure, so maybe there should
be some kind of helper to do the bounds checking and set the
"counted_by" counter?

#define gimme(from, into, counter, len)				\
	({							\
		int __gimme_rc = -EINVAL			\
		size_t __gimme_size = __member_size(from);	\
		if (__gimme_size >= sizeof(*into) &&		\
		    __gimme_size - sizeof(*into) >= len) {	\
			into = (void *)from;			\
			into->counter = len;			\
			__gimme_rc = 0;				\
		}						\
		__gimme_rc;					\
	})

	rc = gimme(&pcu->cmd_buf[1], fragment, len, len);
	if (rc) {
		dev_err(pcu->dev,
			"Invalid record length in firmware: %d\n", len);
		return rc;
	}

-Kees

> 
>  drivers/input/misc/ims-pcu.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index d9ee14b1f451..4581f1c53644 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -844,6 +844,12 @@ static int ims_pcu_flash_firmware(struct ims_pcu *pcu,
>  		addr = be32_to_cpu(rec->addr) / 2;
>  		len = be16_to_cpu(rec->len);
>  
> +		if (len > sizeof(pcu->cmd_buf) - 1 - sizeof(*fragment)) {
> +			dev_err(pcu->dev,
> +				"Invalid record length in firmware: %d\n", len);
> +			return -EINVAL;
> +		}
> +
>  		fragment = (void *)&pcu->cmd_buf[1];
>  		put_unaligned_le32(addr, &fragment->addr);
>  		fragment->len = len;
> -- 
> 2.47.2
> 

-- 
Kees Cook

Re: [PATCH 3/3] Input: ims-pcu - Check record size in ims_pcu_flash_firmware()

[Dan Carpenter]

On Wed, May 28, 2025 at 04:26:18PM -0700, Kees Cook wrote:
> On Wed, May 28, 2025 at 11:22:24PM +0300, Dan Carpenter wrote:
> > The "len" variable comes from the firmware and we generally do
> > trust firmware, but it's always better to double check.  If the "len"
> > is too large it could result in memory corruption when we do
> > "memcpy(fragment->data, rec->data, len);"
> > 
> > Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> > Kees, this is a __counted_by() thing.  Would the checkers catch this?
> > We know the maximum valid length for "fragment" is and so it's maybe
> > possible to know that "fragment->len = len;" is too long?
> 
> I see:
> 
> pcu->cmd_buf as:
> 
>         u8 cmd_buf[IMS_PCU_BUF_SIZE];
> 
> and fragment is:
> 
> struct ims_pcu_flash_fmt {
>         __le32 addr;
>         u8 len;
>         u8 data[] __counted_by(len);
> };
> 
> I assume you're asking about this line:
> 
> 		fragment->len = len;
> 
> I'm not aware of any compiler instrumentation that would bounds check
> this -- it was designed to trust these sort of explicit assignments.
> 
> This is hardly the only place in the kernel doing this kind of
> deserialization into a flexible array structure, so maybe there should
> be some kind of helper to do the bounds checking and set the
> "counted_by" counter?
> 
> #define gimme(from, into, counter, len)				\
> 	({							\
> 		int __gimme_rc = -EINVAL			\
> 		size_t __gimme_size = __member_size(from);	\
> 		if (__gimme_size >= sizeof(*into) &&		\
> 		    __gimme_size - sizeof(*into) >= len) {	\
> 			into = (void *)from;			\
> 			into->counter = len;			\
> 			__gimme_rc = 0;				\
> 		}						\
> 		__gimme_rc;					\
> 	})
> 
> 	rc = gimme(&pcu->cmd_buf[1], fragment, len, len);
> 	if (rc) {
> 		dev_err(pcu->dev,
> 			"Invalid record length in firmware: %d\n", len);
> 		return rc;
> 	}

I don't think that really scales...  I don't know how KASAN works
internally.  I was thinking it might track the buffer size when we
assign "fragment = (void *)&pcu->cmd_buf[1];" so it could calculate
the valid values of ->len.  But that's actually quite complicated.

Smatch does track this:

drivers/input/misc/ims-pcu.c:856 ims_pcu_flash_firmware() buf size: 'fragment->data' 119 elements, 119 bytes

But:

1) Smatch doesn't know about __counted_by().  This is just a matter of
   writing the code in Sparse.
2) It's not treating fw->data[] as user controlled data because this
   driver loads the firmware asynchronously and Smatch gets confused by
   threads.

regards,
dan carpenter

Re: [PATCH 3/3] Input: ims-pcu - Check record size in ims_pcu_flash_firmware()

[Dmitry Torokhov]

On Wed, May 28, 2025 at 11:22:24PM +0300, Dan Carpenter wrote:
> The "len" variable comes from the firmware and we generally do
> trust firmware, but it's always better to double check.  If the "len"
> is too large it could result in memory corruption when we do
> "memcpy(fragment->data, rec->data, len);"
> 
> Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Applied, thank you.

-- 
Dmitry