mbox series

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

Message ID cover.1748463049.git.dan.carpenter@linaro.org
Headers show
Series ihex: add some bounds checking to firmware parsing | expand

Message

Dan Carpenter May 28, 2025, 8:22 p.m. UTC 
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(-)

Comments

Kees Cook May 28, 2025, 11:26 p.m. UTC | #1 
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
>