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
