Re: [PATCH 3/4] spl: fit: Harden external-data offset and size arithmetic

Hi Anton,

On 2026-07-30T15:03:40, Anton Ivanov <[email protected]> wrote:
> spl: fit: Harden external-data offset and size arithmetic
>
> The data-offset, data-position and data-size FIT properties are
> excluded from the configuration signature, so they are attacker
> controlled even when signature verification succeeds. The offset and
> size arithmetic in load_simple_fit() can wrap on hostile values:
>
>  - adding the external-data base offset to data-offset can wrap past
>    UINT32_MAX,
>  - get_aligned_image_size() adds the block-alignment overhead and
>    rounds up to the block length, which can wrap past ULONG_MAX,
>  - adding the FIT's device offset to the aligned external-data offset
>    can wrap past ULONG_MAX.
>
> Make get_aligned_image_size() return the aligned size through an out
> parameter and fail with -EOVERFLOW when the computation would wrap,
> check the two offset additions explicitly, and compare the
> block-aligned size (the amount info->read() actually transfers)
> against max_size before reading. Do the same for the FIT header read
> in spl_simple_fit_read().
> [...]
>
> common/spl/spl_fit.c  | 98 +++++++++++++++++++++++++++++----------------------
>  test/image/spl_load.c | 57 ++++++++++++++++++++++++++++++
>  2 files changed, 112 insertions(+), 43 deletions(-)

Just to flag - first_loading_rbf_to_buffer() has the same 'rbf_offset
+= ((fdt_totalsize(buffer_p) + 3) & ~3)' pattern...out of scope but
the maintainer may wish to take a look.

> diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
> @@ -700,12 +700,8 @@ def test_vboot_ext_data_bounds(ubman):
>          fd.write(500 * b'\0')
>
>      testcases = [
> -        ('negative data-position',
> -         {'data-position': 0xffffffff}, 'Invalid external data position'),
> -        ('negative data-offset',
> +        ('invalid data-offset',
>           {'data-offset': 0xffffffff}, 'Invalid external data offset'),
> -        ('negative data-size',
> -         {'data-size': 0xffffffff}, 'Invalid external data size'),
>          ('off-bounds data-position',
>           {'data-position': 0x7fffffff}, 'FIT external data is out of bounds'),
>          ('off-bounds data-offset',

The 0xffffffff data-position and data-size cases are dropped rather
than repointed at the 'out of bounds' message they now hit. Please
keep the 0xffffffff coverage so a regression that stopped rejecting a
UINT32_MAX property would still be caught - a rename to 'invalid
data-position'/'invalid data-size' with the new expected message would
do it.

Is there any code-size impact for this series?

Regards,
Simon