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 <[email protected]> wrote: > Hi Anton, > > On 2026-06-02T18:30:52, Anton Ivanov <[email protected]> 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 <[email protected]> > > > > 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 >
