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

[sashiko-bot@kernel.org]

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