[PATCH] ihex: reject firmware images smaller than a single record

[PATCH] ihex: reject firmware images smaller than a single record

[Priyanka Mani]

ihex_validate_fw() computes the end of the record area as

	end = (const void *)&fw->data[fw->size - sizeof(*end)];

Both fw->size and sizeof(*end) are size_t, so when the firmware image
is smaller than a single struct ihex_binrec (6 bytes) the subtraction
wraps around. 'end' then points far beyond the buffer, the

	for (; rec <= end; rec = __ihex_next_binrec(rec))

loop condition is satisfied, and the body dereferences 'rec' while
walking records that do not exist.

This is trivially reachable from userspace via the sysfs firmware
fallback: loading an empty image (write "1" then "0" to .../loading
without writing any data) leaves fw->size == 0 and fw->data == NULL, so
the first ihex_binrec_size() reads rec->len at NULL + 4:

  Oops: general protection fault, probably for non-canonical address ...
  KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
  RIP: 0010:ihex_binrec_size include/linux/ihex.h:26 [inline]
  RIP: 0010:__ihex_next_binrec include/linux/ihex.h:35 [inline]
  RIP: 0010:ihex_validate_fw include/linux/ihex.h:54 [inline]
  RIP: 0010:request_ihex_firmware include/linux/ihex.h:74 [inline]
  RIP: 0010:emi26_load_firmware drivers/usb/misc/emi26.c:86 [inline]
  RIP: 0010:emi26_probe+0x283/0x1690 drivers/usb/misc/emi26.c:232

A valid ihex image must contain at least the mandatory zero-length
terminating record, so anything smaller than sizeof(struct ihex_binrec)
cannot be valid. Reject it up front before the wrapping subtraction.

Fixes: f1485f3deb89 ("ihex: request_ihex_firmware() function to load and validate firmware")
Reported-by: syzbot+baf3cbba7dd980984f0d@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=baf3cbba7dd980984f0d
Signed-off-by: Priyanka Mani <priyankamani2100@gmail.com>
---
 include/linux/ihex.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/ihex.h b/include/linux/ihex.h
index b824877e6d1b..61f1df4f7819 100644
--- a/include/linux/ihex.h
+++ b/include/linux/ihex.h
@@ -48,6 +48,16 @@ static inline int ihex_validate_fw(const struct firmware *fw)
 {
 	const struct ihex_binrec *end, *rec;
 
+	/*
+	 * The firmware must be large enough to hold at least the mandatory
+	 * zero-length terminating record. Without this check a short (e.g.
+	 * empty) image makes the fw->size - sizeof(*end) subtraction below
+	 * wrap around, yielding a bogus 'end' pointer and an out-of-bounds
+	 * walk over the records.
+	 */
+	if (fw->size < sizeof(*end))
+		return -EINVAL;
+
 	rec = (const void *)fw->data;
 	end = (const void *)&fw->data[fw->size - sizeof(*end)];
 
-- 
2.43.0

Re: [PATCH] ihex: reject firmware images smaller than a single record

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ihex: out-of-bounds read in consumers on 6-byte empty firmware
--

commit 6837e2beeeacda6e5491e8d86cf4e9038b12d3d2
Author: Priyanka Mani <priyankamani2100@gmail.com>

ihex: reject firmware images smaller than a single record

The commit checks that an ihex firmware image is at least large enough to hold
the terminating record, avoiding an out-of-bounds read when processing smaller
or empty images.

> diff --git a/include/linux/ihex.h b/include/linux/ihex.h
> index b824877e6d1b6..61f1df4f7819c 100644
> --- a/include/linux/ihex.h
> +++ b/include/linux/ihex.h
> @@ -48,6 +48,16 @@ static inline int ihex_validate_fw(const struct firmware *fw)
>  {
>  	const struct ihex_binrec *end, *rec;
>  
> +	/*
> +	 * The firmware must be large enough to hold at least the mandatory
> +	 * zero-length terminating record. Without this check a short (e.g.
> +	 * empty) image makes the fw->size - sizeof(*end) subtraction below
> +	 * wrap around, yielding a bogus 'end' pointer and an out-of-bounds
> +	 * walk over the records.
> +	 */
> +	if (fw->size < sizeof(*end))
> +		return -EINVAL;
> +

[Severity: High]
Does this check still allow a 6-byte firmware (consisting entirely of the
zero-length EOF record) to pass validation?

Since sizeof(*end) is 6, a 6-byte firmware size would bypass this check. When
drivers like io_edgeport process this firmware, they set rec = fw->data.
The io_edgeport driver then unconditionally reads up to 4 bytes from rec->data
in update_edgeport_E2PROM():

        BootMajorVersion = rec->data[0];
        BootMinorVersion = rec->data[1];
        BootBuildNumber = (rec->data[2] << 8) | rec->data[3];

Since rec->len is 0 in the EOF record, wouldn't this read out-of-bounds past
the 6-byte firmware allocation?

Additionally, if drivers iterate over records by calling ihex_next_binrec()
on the EOF record (as seen in ims_pcu_count_fw_records() for example):

        while (rec) {
                count++;
                rec = ihex_next_binrec(rec);
        }

Calling ihex_next_binrec() on the EOF record reads rec->len from rec + 12:

        return be16_to_cpu(rec->len) ? rec : NULL;

Could this trigger another out-of-bounds read on a 6-byte firmware image?

>  	rec = (const void *)fw->data;
>  	end = (const void *)&fw->data[fw->size - sizeof(*end)];
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/10da8e7f7c7070fc9a9da1d274b78f1d3f379703.1791521373.git.priyankamani2100@gmail.com?part=1