Hi Aristo,
On 2026-10-11T15:21:13, Aristo Chen <[email protected]> wrote:
> image-fit: check the length of the external data properties
>
> fit_image_get_data_offset(), fit_image_get_data_position() and
> fit_image_get_data_size() pass NULL as fdt_getprop()'s length argument,
> so they accept a property of any size and then dereference the first
> four bytes of it. A property shorter than that is read past its end, and
> the bytes that follow it in the FIT are returned to the caller. An empty
> data-size, for example, is read as the structure tag that follows it, a
> small number such as 2 or 3, and selects a few bytes of the external
> data instead of failing.
>
> Ask for the length and require it to be exactly one fdt32_t, as
> commit c2b8eefd4e10 ("image-fit: check the length of the
> data-size-unciphered property") does for data-size-unciphered. Every
> caller already checks the return value and handles a failure like a
> missing property.
>
> Fixes: db1b79b886f0 ("splash: add support for loading splash from a FIT
> image")
> Fixes: a1be94b65410 ("SPL: Add FIT data-position property support")
> Signed-off-by: Aristo Chen <[email protected]>
>
> boot/image-fit.c | 21 ++++++++++++++++++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
> diff --git a/boot/image-fit.c b/boot/image-fit.c
> @@ -1012,15 +1022,20 @@ int fit_image_get_data_position(const void *fit, int
> noffset,
> int fit_image_get_data_size(const void *fit, int noffset, u32 *data_size)
> {
> const fdt32_t *val;
> + int len;
>
> - val = fdt_getprop(fit, noffset, FIT_DATA_SIZE_PROP, NULL);
> + val = fdt_getprop(fit, noffset, FIT_DATA_SIZE_PROP, &len);
> if (!val)
> return -ENOENT;
>
> + if (len != sizeof(*val))
> + return -EINVAL;
> +
Counting fit_image_get_data_size_unciphered(), this leaves four copies
of the same body. Please can you add a small static helper, e.g.
fit_get_u32_prop(fit, noffset, name, &val), and call it from each
accessor? That would also win back some of the 60 bytes this adds to
SPL. A follow-up patch is fine if you prefer to keep this one focused
on the fix.
I checked the callers in image-fit.c, spl_fit.c, splash_source.c,
socfpga_arria10.c and (after patch 4) fit_image.c. All of them treat a
non-zero return as 'not present' or bail out, so -EINVAL is safe.
There is one change in behaviour: if data-offset is also present, a
malformed data-position now falls through to it rather than winning.
That seems fine, since having both is already invalid.
Reviewed-by: Simon Glass <[email protected]>
