| Message ID | 20260902-b4-fit-ext-data-hardening-v4-0-a884a5de96ea@binarly.io |
|---|---|
| Headers | show |
| Series | fit: Harden handling of external-data properties | expand |
On Wed, 02 Sep 2026 22:23:22 +0100, Anton Ivanov wrote: > The data-offset, data-position and data-size FIT properties are 32-bit > unsigned values, but were read through signed int. Also, they are > excluded from the configuration signature, so they are attacker > controlled. > > Patch 1 switches the accessors and their callers to u32, removing > the ad-hoc handling of "negative" values in U-Boot proper. This > transition was discussed and agreed on in [1]. > > [...] Applied to u-boot/main, thanks! [1/4] image-fit: Use unsigned types for external data properties commit: c4d81d062790c76ef0db4ead2ac69cab7245c2fc [2/4] test: spl: Factor out external-data FIT property checks commit: 506ef1fd79c49d9c8326c9e73e89d35771cb650f [3/4] spl: fit: Harden external-data offset and size arithmetic commit: 01460dfbda162ca9a2fddbba001edacabc38dd2f [4/4] spl: fit: Bound external data like U-Boot proper commit: 3158b844335a4a84a5504a813f2daa6acff5985d
Hi Anton, et. al. On Wed, 2026-09-02 at 22:23 +0100, Anton Ivanov wrote: > The data-offset, data-position and data-size FIT properties are > excluded from the configuration signature, so they are attacker > controlled even when signature verification succeeds. The offset and > size arithmetic in load_simple_fit() can wrap on hostile values: > > - adding the external-data base offset to data-offset can wrap past > UINT32_MAX, > - get_aligned_image_size() adds the block-alignment overhead and > rounds up to the block length, which can wrap past ULONG_MAX, > - adding the FIT's device offset to the aligned external-data offset > can wrap past ULONG_MAX. > > Make get_aligned_image_size() return the aligned size through an out > parameter and fail with -EOVERFLOW when the computation would wrap, > check the two offset additions explicitly, and compare the > block-aligned size (the amount info->read() actually transfers) > against max_size before reading. Do the same for the FIT header read > in spl_simple_fit_read(). I bisected this commit to regress on the LicheePi 4A in the sense that the SPL is no longer able to load/execute OpenSBI/U-Boot proper: U-Boot SPL 2026.10 (Oct 10 2026 - 21:45:55 +0200) Trying to boot from RAM => hangs My fix reverting this and a second patch [1] got accepted in meta-riscv, see: https://github.com/riscv/meta-riscv/pull/711 Before I dig deeper, I would welcome any insight anybody could have. Thanks! Cheers Marcel [1] https://lore.kernel.org/all/20260421075439.16696-6-ravi@prevas.dk > Signed-off-by: Anton Ivanov <anton@binarly.io> > Reviewed-by: Simon Glass <sjg@chromium.org> > > --- > Changed since v3: the new diagnostics use log_debug() instead of > printf(), which compiles out in a non-debug SPL build. > > Code size, from buildman -S in the u-boot-gitlab-ci-runner container > (gcc 14.2.0). The 413 boards that build spl_fit.c cover seven > combinations of CONFIG_SPL_LOAD_BLOCK (BLK), CONFIG_SPL_FIT_SIGNATURE > (SIG) and bitness, one board each below. SPL only; U-Boot proper is > unchanged: > > BLK SIG bit board text rodata data all > y n 32 am335x_evm +24 -49 -4 -29 > y y 32 firefly-rk3288 +20 -49 0 -29 > n n 32 am335x_evm_spiboot +4 -49 0 -45 > n y 32 evb-ast2600 -8 -49 0 -57 > y n 64 imx8mm_evk +16 -49 0 -33 > y y 64 orangepi-5-ultra-rk3588 +28 -49 0 -21 > n n 64 r8a779g0_whitehawk 0 -49 0 -49 > > The rodata drop is the "FIT image too large" printf() this patch > removes. > --- > common/spl/spl_fit.c | 98 +++++++++++++++++++++++++++++---------------------- > test/image/spl_load.c | 57 ++++++++++++++++++++++++++++++ > 2 files changed, 112 insertions(+), 43 deletions(-) > > diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c > index 9b16f91cc6f..65934674b6c 100644 > --- a/common/spl/spl_fit.c > +++ b/common/spl/spl_fit.c > @@ -175,22 +175,32 @@ static int spl_fit_get_image_node(const struct spl_fit_info *ctx, > return node; > } > > -static int get_aligned_image_offset(struct spl_load_info *info, int offset) > +static u32 get_aligned_image_offset(struct spl_load_info *info, u32 offset) > { > return ALIGN_DOWN(offset, spl_get_bl_len(info)); > } > > -static int get_aligned_image_overhead(struct spl_load_info *info, int offset) > +static u32 get_aligned_image_overhead(struct spl_load_info *info, u32 offset) > { > return offset & (spl_get_bl_len(info) - 1); > } > > -static int get_aligned_image_size(struct spl_load_info *info, int data_size, > - int offset) > +static int get_aligned_image_size(struct spl_load_info *info, ulong data_size, > + u32 offset, ulong *aligned_size) > { > - data_size = data_size + get_aligned_image_overhead(info, offset); > + u32 overhead = get_aligned_image_overhead(info, offset); > > - return ALIGN(data_size, spl_get_bl_len(info)); > + if (data_size > ULONG_MAX - overhead) > + return -EOVERFLOW; > + data_size += overhead; > + > + if (data_size > ULONG_MAX - (spl_get_bl_len(info) - 1)) > + return -EOVERFLOW; > + data_size = ALIGN(data_size, spl_get_bl_len(info)); > + > + *aligned_size = data_size; > + > + return 0; > } > > /** > @@ -223,17 +233,16 @@ static int load_simple_fit(struct spl_load_info *info, ulong fit_offset, > ulong load_addr; > void *load_ptr; > void *src; > - ulong overhead; > uint8_t image_comp = -1, type = -1; > const void *data; > const void *fit = ctx->fit; > bool external_data = false; > + int ret; > > log_debug("starting\n"); > if (CONFIG_IS_ENABLED(BOOTMETH_VBE) && > xpl_get_phase(info) != IH_PHASE_NONE) { > enum image_phase_t phase; > - int ret; > > ret = fit_image_get_phase(fit, node, &phase); > /* if the image is for any phase, let's use it */ > @@ -273,13 +282,19 @@ static int load_simple_fit(struct spl_load_info *info, ulong fit_offset, > if (!fit_image_get_data_position(fit, node, &offset)) { > external_data = true; > } else if (!fit_image_get_data_offset(fit, node, &offset)) { > - log_debug("read offset %x = offset from fit %lx\n", > - offset, (ulong)offset + ctx->ext_data_offset); > + /* The resulting offset cannot exceed UINT32_MAX */ > + if (ctx->ext_data_offset > UINT32_MAX - offset) { > + log_debug("Invalid external data offset: %u\n", offset); > + return -EINVAL; > + } > + log_debug("read offset %x = offset from fit %x\n", offset, > + (u32)(offset + ctx->ext_data_offset)); > offset += ctx->ext_data_offset; > external_data = true; > } > > if (external_data) { > + u32 aligned_offset; > ulong read_offset; > void *src_ptr; > > @@ -300,16 +315,26 @@ static int load_simple_fit(struct spl_load_info *info, ulong fit_offset, > * controlled even after fit_config_verify() succeeds. The > * image hash is only verified after the device read below, so > * an oversized value has to be rejected here. > - * > - * Bail out before get_aligned_image_size() runs on a hostile > - * len: that helper does its arithmetic in int and would > - * invoke signed-integer overflow on a value close to or above > - * INT_MAX. The block-aligned check further down is the > - * mathematically binding one, since size is len rounded up to > - * the device block length. > */ > - if ((ulong)len > max_size) > - goto too_big; > + ret = get_aligned_image_size(info, len, offset, &size); > + if (ret) { > + log_debug("Invalid external data size: %u\n", len); > + return ret; > + } > + > + if (size > max_size) { > + log_debug("Image too large: aligned size %lu, max %lu (data-size %u)\n", > + size, max_size, len); > + return -EFBIG; > + } > + > + aligned_offset = get_aligned_image_offset(info, offset); > + if (aligned_offset > ULONG_MAX - fit_offset) { > + log_debug("Invalid aligned external data offset: %u\n", > + aligned_offset); > + return -EINVAL; > + } > + read_offset = fit_offset + aligned_offset; > > if (spl_decompression_enabled() && > (image_comp == IH_COMP_GZIP || image_comp == IH_COMP_LZMA)) > @@ -318,19 +343,6 @@ static int load_simple_fit(struct spl_load_info *info, ulong fit_offset, > src_ptr = map_sysmem(ALIGN(load_addr, ARCH_DMA_MINALIGN), len); > length = len; > > - overhead = get_aligned_image_overhead(info, offset); > - size = get_aligned_image_size(info, length, offset); > - read_offset = fit_offset + get_aligned_image_offset(info, > - offset); > - > - /* > - * info->read() transfers the block-aligned size into the > - * destination, so this is the bound that actually matters; > - * len was rejected above only to keep this computation safe. > - */ > - if (size > max_size) > - goto too_big; > - > log_debug("reading from offset %x / %lx size %lx to %p: ", > offset, read_offset, size, src_ptr); > > @@ -339,7 +351,7 @@ static int load_simple_fit(struct spl_load_info *info, ulong fit_offset, > > debug("External data: dst=%p, offset=%x, size=%lx\n", > src_ptr, offset, (unsigned long)length); > - src = src_ptr + overhead; > + src = src_ptr + get_aligned_image_overhead(info, offset); > } else { > /* Embedded data */ > if (fit_image_get_emb_data(fit, node, &data, &length)) { > @@ -401,11 +413,6 @@ static int load_simple_fit(struct spl_load_info *info, ulong fit_offset, > upl_add_image(fit, node, load_addr, length); > > return 0; > - > -too_big: > - printf("%s: FIT image too large (data-size %u, max %lu)\n", > - __func__, (u32)len, max_size); > - return -EFBIG; > } > > static bool os_takes_devicetree(uint8_t os) > @@ -737,8 +744,9 @@ static int spl_simple_fit_read(struct spl_fit_info *ctx, > struct spl_load_info *info, ulong offset, > const void *fit_header) > { > - unsigned long count, size; > + unsigned long aligned_size, count, size; > void *buf; > + int ret; > > /* > * For FIT with external data, figure out where the external images > @@ -756,8 +764,12 @@ static int spl_simple_fit_read(struct spl_fit_info *ctx, > * For FIT with data embedded, data is loaded as part of FIT image. > * For FIT with external data, data is not loaded in this step. > */ > - size = get_aligned_image_size(info, size, 0); > - buf = board_spl_fit_buffer_addr(size, size, 1); > + ret = get_aligned_image_size(info, size, 0, &aligned_size); > + if (ret) { > + log_debug("Invalid FIT size: %lu\n", size); > + return ret; > + } > + buf = board_spl_fit_buffer_addr(aligned_size, aligned_size, 1); > if (!buf) { > /* > * We assume that none of the board will ever use 0x0 as a > @@ -767,7 +779,7 @@ static int spl_simple_fit_read(struct spl_fit_info *ctx, > return -EIO; > } > > - count = info->read(info, offset, size, buf); > + count = info->read(info, offset, aligned_size, buf); > if (!count) { > /* > * FIT could not be read. This means we should free the > @@ -800,7 +812,7 @@ static int spl_simple_fit_read(struct spl_fit_info *ctx, > > ctx->fit = buf; > debug("fit read offset %lx, size=%lu, dst=%p, count=%lu\n", > - offset, size, buf, count); > + offset, aligned_size, buf, count); > > return 0; > } > diff --git a/test/image/spl_load.c b/test/image/spl_load.c > index 49bfce15c08..557362ba113 100644 > --- a/test/image/spl_load.c > +++ b/test/image/spl_load.c > @@ -426,6 +426,63 @@ static int spl_test_fit_external_oversize(struct unit_test_state *uts) > } > SPL_TEST(spl_test_fit_external_oversize, 0); > > +/* > + * A data-offset which wraps past UINT32_MAX once the external-data base > + * offset is added must be rejected before it is used as a read offset. > + */ > +static int spl_test_fit_data_offset_overflow(struct unit_test_state *uts) > +{ > + if (!image_supported(FIT_EXTERNAL)) > + return -EAGAIN; > + > + return check_fit_ext_prop(uts, FIT_DATA_OFFSET_PROP, 0xffffffff, 1, > + spl_test_read, 0, -EINVAL); > +} > +SPL_TEST(spl_test_fit_data_offset_overflow, 0); > + > +/* > + * A data-size whose block-aligned read size wraps past ULONG_MAX must be > + * rejected. Since data-size is a 32-bit property, the wrap is only reachable > + * when ulong is 32 bits wide, so skip the test on other targets. > + */ > +static int spl_test_fit_data_size_overflow(struct unit_test_state *uts) > +{ > + if (!image_supported(FIT_EXTERNAL) || sizeof(ulong) != 4) > + return -EAGAIN; > + > + return check_fit_ext_prop(uts, FIT_DATA_SIZE_PROP, 0xffffffff, 2, > + spl_test_read, 0, -EOVERFLOW); > +} > +SPL_TEST(spl_test_fit_data_size_overflow, 0); > + > +/* Device offset the reader pretends the FIT was loaded from */ > +static ulong spl_test_fit_offset; > + > +static ulong spl_test_read_fit_offset(struct spl_load_info *load, ulong sector, > + ulong count, void *buf) > +{ > + return spl_test_read(load, sector - spl_test_fit_offset, count, buf); > +} > + > +/* > + * An aligned external-data offset which wraps past ULONG_MAX once the FIT's > + * offset on the device is added must be rejected before it is used as a read > + * offset. > + */ > +static int spl_test_fit_read_offset_overflow(struct unit_test_state *uts) > +{ > + if (!image_supported(FIT_EXTERNAL)) > + return -EAGAIN; > + > + /* So that ULONG_MAX - fit_offset < the aligned external-data offset */ > + spl_test_fit_offset = ULONG_MAX - 0xfff; > + > + return check_fit_ext_prop(uts, FIT_DATA_OFFSET_PROP, 0x1000, 1, > + spl_test_read_fit_offset, spl_test_fit_offset, > + -EINVAL); > +} > +SPL_TEST(spl_test_fit_read_offset_overflow, 0); > + > /* > * LZMA is too complex to generate on the fly, so let's use some data I put in > * the oven^H^H^H^H compressed earlier
The data-offset, data-position and data-size FIT properties are 32-bit unsigned values, but were read through signed int. Also, they are excluded from the configuration signature, so they are attacker controlled. Patch 1 switches the accessors and their callers to u32, removing the ad-hoc handling of "negative" values in U-Boot proper. This transition was discussed and agreed on in [1]. The SPL loader has its own copy of this logic with the same problems and fewer checks. Patch 2 factors out a test helper, patch 3 makes the SPL offset/size arithmetic overflow-safe, and patch 4 adds the addressable-range and FIT_SIGNATURE_MAX_SIZE bounds check that U-Boot proper already performs in fit_image_get_data(). Patches 3 and 4 build on each other and on patch 1, so they are not intended to be cherry-picked individually. [1] https://lore.kernel.org/u-boot/CAPWaX55XWFLcMGRuaUuXXn__MX_UG4J8QTd6rvXZmJ4WSOCy9w@mail.gmail.com/ --- Changes in v4: - Patches 3 and 4: the new diagnostics use log_debug() instead of printf(), in order to keep SPL size minimal; log_debug() compiles out in a non-debug SPL build. This is the only code change. - Fixed broken build of am335x_evm, which has only 100 bytes spare under CONFIG_SPL_MAX_SIZE (0x1b000) at the base commit; v3 overflowed it by 216 bytes, while this version leaves 113 bytes spare. - Patches 3 and 4: code-size figures re-measured with buildman -S inside the u-boot-gitlab-ci-runner container, so they match what CI sees, and they now cover every configuration rather than a sample: of the 413 boards that build spl_fit.c there are seven combinations of the options the changed code depends on, and each patch quotes one board per combination. - Patch 3: added Simon's Reviewed-by tag. - Link to v3: https://lore.kernel.org/all/20260810-b4-fit-ext-data-hardening-v3-0-17184cdb87fd@binarly.io/ Changes in v3: - No code changes. - Patches 1, 2 and 4: added Simon's Reviewed-by tags, which were missing from the v2 patches. - Link to v2: https://lore.kernel.org/all/20260806-b4-fit-ext-data-hardening-v2-0-b181cac15c29@binarly.io/ Changes in v2: - Patch 1: keep the 0xffffffff data-position and data-size test cases instead of dropping them; they are renamed and repointed at the "FIT external data is out of bounds" message they now hit. - Patches 3 and 4: no code changes, added SPL code-size figures - Link to v1: https://lore.kernel.org/all/20260730-b4-fit-ext-data-hardening-v1-0-4a6b91e37b51@binarly.io/ All four patches carry Reviewed-by: Simon Glass. Patches 3 and 4 are unchanged since v3 apart from their diagnostics becoming log_debug(). --- Anton Ivanov (4): image-fit: Use unsigned types for external data properties test: spl: Factor out external-data FIT property checks spl: fit: Harden external-data offset and size arithmetic spl: fit: Bound external data like U-Boot proper boot/image-fit.c | 29 ++++------ common/spl/spl_fit.c | 122 +++++++++++++++++++++++++-------------- common/splash_source.c | 2 +- drivers/fpga/socfpga_arria10.c | 3 +- include/image.h | 6 +- test/image/spl_load.c | 128 +++++++++++++++++++++++++++++++++++------ test/py/tests/test_vboot.py | 10 ++-- 7 files changed, 210 insertions(+), 90 deletions(-) --- base-commit: 94b349bd902d9e38e1846c157fadd9054c34680a change-id: 20260730-b4-fit-ext-data-hardening-1734a9203753 Best regards, -- Anton Ivanov <anton@binarly.io>