Hi Simon, On Mon, Oct 12, 2026 at 10:10 PM Simon Glass <[email protected]> wrote: > > 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 will send the helper as a follow-up. > > 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. Thanks for checking all the callers, and agreed on the data-position case. > > Reviewed-by: Simon Glass <[email protected]> Best regards, Aristo
