[PATCH v1 1/2] spl: fit: bound the external data size before reading it

[Simon Glass]

Hi Aristo,

On 2026-06-18T15:47:45, Aristo Chen <aristo.chen at canonical.com> wrote:
> spl: fit: bound the external data size before reading it
>
> load_simple_fit() loads an image stored as external data by reading it
> from the boot device with a transfer sized from the FIT data-size
> property. That property is listed in exc_prop[] in image-fit-sig.c, so
> it is excluded from the configuration signature and stays under the
> control of anyone able to modify the boot medium even when
> CONFIG_SPL_FIT_SIGNATURE is enabled. The read happens before
> fit_image_verify_with_data() checks the image hash, so an inflated
> data-size overruns the destination before the corruption can be
> detected. The device-tree overlay path is the sharpest case, because
> there the destination is a fixed CONFIG_SPL_LOAD_FIT_APPLY_OVERLAY_BUF_SZ
> heap buffer.
>
> Pass the size of the destination into load_simple_fit() and reject an
> image whose data does not fit before the read is issued. The data-size
> property is read into an int, so a value with bit 31 set arrives as a
> negative number, and comparing it as an unsigned quantity rejects that
> case as well. The block-aligned transfer length is checked too, because
> info->read() moves data-size rounded up to the device block length.
> [...]
>
> common/spl/spl_fit.c | 41 +++++++++++++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 6 deletions(-)

> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> @@ -291,6 +294,18 @@ static int load_simple_fit(struct spl_load_info *info, ulong fit_offset,
> +             if ((ulong)len > max_size)
> +                     goto too_big;
> +

> @@ -302,6 +317,15 @@ static int load_simple_fit(struct spl_load_info *info, ulong fit_offset,
> +             /* The block-aligned read size must also fit the destination */
> +             if (size > max_size) {
> +too_big:
> +                     printf("%s: FIT image too large (max %lu)\n",
> +                            __func__, max_size);
> +                     return -EFBIG;
> +             }

Please can you avoid jumping into the body of an if-statement. A goto
landing inside a conditional block and falling through the printf and
return is hard to follow, and will likely trip up static analysers.
Put the shared label at the end of the function:

    if ((ulong)len > max_size)
        goto too_big;
    ...
    if (size > max_size)
        goto too_big;
    ...
    return 0;
    too_big:
        printf(...);
        return -EFBIG;

The cover letter quotes 57 bytes for the current shape; what does the
cleaner form cost on am335x_hs_evm? If it really does push past
CONFIG_SPL_MAX_SIZE then we'll have to use your code, but please say
so in the commit message.

> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> @@ -291,6 +294,18 @@ static int load_simple_fit(struct spl_load_info *info, ulong fit_offset,
> +             if ((ulong)len > max_size)
> +                     goto too_big;

Since size is the block-aligned length and size >= len, the second
check already covers everything this one catches, including the
negative-len case. The value of the early check is that it bails
before get_aligned_image_size() runs on a hostile value - please
mention that in the comment rather than the bit-31 reasoning, which
applies equally to the later check.

Also, the diagnostic only prints max_size. Including len (or size)
would make a real failure much easier to triage from a board log.

> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> @@ -427,7 +451,8 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image,
>       } else {
> -             ret = load_simple_fit(info, offset, ctx, node, &image_info);
> +             ret = load_simple_fit(info, offset, ctx, node, &image_info,
> +                                   CONFIG_SYS_BOOTM_LEN);

Just to check - the destination here is image_info.load_addr, set a
few lines up to ALIGN(spl_image->load_addr + spl_image->size, 8)  --
CONFIG_SYS_BOOTM_LEN is a sensible upper bound for the overall image,
but it isn't the space available at that address; the FDT is just
placed after the loaded image. Please note in the commit message that
for the non-overlay paths this is a conservative ceiling rather than
the true destination size, so it isn't mistaken for a tight bound
later. The overlay caller is the one actually sized to the buffer.

Regards,
Simon