[PATCH v4] image-fit: Validate external data offset and size

[Anton Ivanov]

Hi Simon,

Thank you for the review. The feedback is addressed in v5.

> On your uint32_t question: yes, that's the right direction, but it may
> be best as a separate patch.

Agreed - we'll be happy to follow up with that as a separate patch once
this one is merged.

> drop the FIT_SIGNATURE gate so
> non-signature callers are protected too, and use if
> (CONFIG_IS_ENABLED(...)) rather than #if if you keep any config gate.

As we understand it, non-signed FITs can only be checked against
the valid addressable range. For signed FITs, we can also check that
(offset + len) does not exceed FIT_SIGNATURE_MAX_SIZE. We cannot use
CONFIG_IS_ENABLED(...) rather than #if because FIT_SIGNATURE_MAX_SIZE
depends on FIT_SIGNATURE. Therefore, CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE)
is undefined when signing is disabled, and referencing it here would
result in a compilation error.

Thank you,
Anton


On Wed, Jun 3, 2026 at 6:08 PM Simon Glass <sjg at chromium.org> wrote:

> Hi Anton,
>
> On 2026-06-02T18:30:52, Anton Ivanov <anton at binarly.io> wrote:
> > image-fit: Validate external data offset and size
> >
> > fit_image_get_data() uses the data-position, data-offset, and
> > data-size FIT properties without bounds checking. A crafted FIT
> > image can specify values that cause out-of-bounds read during
> > signature verification of an untrusted FIT.
> >
> > Validate that the external data offset and size are non-negative,
> > and that the data region fits within the FIT image bounds.
> >
> > Signed-off-by: Anton Ivanov <anton at binarly.io>
> >
> > boot/image-fit.c            |  16 +++++
> >  test/py/tests/test_vboot.py | 165
> ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 181 insertions(+)
>
> I think I have lost track of the versions here. But we are now on v4.
>
> > diff --git a/boot/image-fit.c b/boot/image-fit.c
> > @@ -1087,8 +1087,24 @@ int fit_image_get_data(const void *fit, int
> noffset, const void **data,
> >
> >       if (external_data) {
> >               debug("External Data\n");
> > +             if (offset < 0 || offset > UINTPTR_MAX - (uintptr_t)fit) {
> > +                     printf("Invalid external data offset: %d\n",
> offset);
> > +                     return -1;
> > +             }
>
> The offset < 0 check is after the offset += ((fdt_totalsize(fit) + 3)
> & ~3) addition on line 1082, so we could have a signed-overflow window
> is still open
>
> Should return -EINVAL as -1 is actually -EPERM
>
> On your uint32_t question: yes, that's the right direction, but it may
> be best as a separate patch. Changing the prototypes of
> fit_image_get_data_position(), fit_image_get_data_offset() and
> fit_image_get_data_size() to use uint32_t matches what was done for
> image-fit-sig: Validate hashed-strings region size. Please go ahead -
> it lets you drop the < 0 checks and do the bound arithmetic safely in
> unsigned. The callers I checked pass the result through as integers,
> so the churn should be small.
>
> > diff --git a/boot/image-fit.c b/boot/image-fit.c
> > @@ -1087,8 +1087,24 @@ int fit_image_get_data(const void *fit, int
> noffset, const void **data,
> > +#if CONFIG_IS_ENABLED(FIT_SIGNATURE)
> > +                     if (len > CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE) -
> offset) {
> > +                             printf("FIT external data is out of bounds
> (offset=%d, size=%d)\n",
> > +                                    offset, len);
> > +                             return -1;
> > +                     }
> > +#endif
>
> Please split into two checks so the subtraction can't underflow (if
> (offset > max || len > max - offset)), drop the FIT_SIGNATURE gate so
> non-signature callers are protected too, and use if
> (CONFIG_IS_ENABLED(...)) rather than #if if you keep any config gate.
>
> > diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
> > @@ -563,6 +563,171 @@ def test_vboot(ubman, name, sha_algo, padding,
> sign_options, required,
> > +        ('off-bounds data-position',
> > +         {'data-position': 0x7FFFFFFF}, 'FIT external data is out of
> bounds'),
>
> lower-case hex
>
> Regards,
> Simon
>