Re: [PATCH 1/3] bootm: size the noload decompression buffer from the compressor header

Hi Simon, Tom,

On Sun, Aug 16, 2026 at 2:33 AM Simon Glass <[email protected]> wrote:
>
> Hi,
>
> On Wed, 12 Aug 2026 at 09:57, Tom Rini <[email protected]> wrote:
> >
> > On Wed, Aug 12, 2026 at 09:45:52AM +0200, Nora Schiffer wrote:
> > > On Mon, 2026-08-10 at 10:37 -0600, Tom Rini wrote:
> > > > On Mon, Aug 10, 2026 at 10:32:12AM +0800, Aristo Chen wrote:
> > > > > On Sun, Aug 9, 2026 at 11:27 PM Tom Rini <[email protected]> wrote:
> > > > > >
> > > > > > On Sun, Aug 09, 2026 at 04:23:27AM +0000, Aristo Chen wrote:
> > > > > >
> > > > > > > For a compressed kernel_noload image, bootm_load_os() allocates a
> > > > > > > per-image decompression buffer of ALIGN(image_len * 8, SZ_1M). The 8x
> > > > > > > multiplier is a heuristic: it comfortably covers what zstd and xz
> > > > > > > achieve on real kernels, but any well-compressed payload (say, a big
> > > > > > > run of zeros) can exceed it and fail decompression, and no fixed
> > > > > > > multiplier is safe against arbitrarily compressible input.
> > > > > > >
> > > > > > > Read the real uncompressed size from the compressor header instead.
> > > > > > > Add a small helper image_decomp_get_uncompressed_size() that returns
> > > > > > > the uncompressed size when the format carries one: gzip ISIZE, lzma
> > > > > > > header uncompressed size, lz4 frame Content_Size when the FLG bit is
> > > > > > > set, and zstd Frame_Content_Size. Other formats return -EOPNOTSUPP.
> > > > > > > Bootm uses it to size the buffer to ALIGN(hdr_size, SZ_1M), capped at
> > > > > > > CONFIG_SYS_BOOTM_LEN because the value is attacker-controlled, and
> > > > > > > falls back to the 8x heuristic for formats without a size field
> > > > > > > (bzip2, lzo, xz) or when the header lacks the size (some lzma or lz4
> > > > > > > streams).
> > > > > >
> > > > > > Have we gotten actual problem reports? This is a good bit of growth for
> > > > > > a problem I'm not sure we're seeing. Thanks.
> > > > >
> > > > > Thanks for the review! Honest answer: no bug report against the
> > > > > current 8x multiplier has crossed the list. This is preventive rather
> > > > > than reactive, and I should have made that clearer in the cover
> > > > > letter.
> > > > >
> > > > > The reasons for this patch set are:
> > > > >   * The multiplier is fundamentally a heuristic. Nora raised the same
> > > > > concern in the v1 round of the earlier
> > > > > series(<https://lists.denx.de/pipermail/u-boot/2026-June/621575.html>):
> > > > > "Deriving a buffer size from the compressed size is not possible, as
> > > > > the compression ratio may be arbitrarily high for data with many
> > > > > repetitions (for example ranges of 0x00 or 0xff)."She dropped her
> > > > > replacement patch when we bumped 4x to 8x, but the underlying point
> > > > > stands: any fixed factor can be defeated by a highly    compressible
> > > > > payload, and further bumps are just moving the ceiling.
> > > >
> > > > Yeah, I recall this. But we aren't really handling arbitrary data here,
> > > > so it's not as much of a valid concern I think, without real examples.
> > >
> > > It's probably not a problem when the OS image is a proper kernel, but if the
> > > next image is a tiny loader itself, even a small amount of padding (either
> > > inside the .data section or at the end of the image) might result in high
> > > compression ratios.
> > >
> > > While irrelevant for current U-Boot, one example would be OpenWrt's lzma-loader:
> > > it has a build mode where the <100KiB binary is padded to 1MiB (I may be
> > > remembering the exact numbers wrong) before compression to force a cache
> > > writeback during decompression (to work around ancient U-Boot versions that did
> > > not implement cache handling correctly.)
> > >
> > > Specifically the case of kernel_noload would usually be used with EFI
> > > applications, for which additional loaders (shim, systemd-boot, ...) are quite
> > > common. The combination with FIT and compression is probably less common...
> > >
> > > Nonetheless, I think a principled fix is preferable - I like the EFI-in-FIT
> > > approach a lot (we may make that the default setup in our TQ-Systems standard
> > > BSPs in the future), thus I would like the feature to be well-supported and
> > > without known bugs.
> >
> > Thanks for explaining. My concern, now that I've put it through a wider
> > test, is that of about 1550 platforms, 1297 grow under this as-is. Of
> > those, ~375 grow by around 400 bytes (380 is average, a few go higher).
> > The rest are around 170 bytes. This is all presumably the difference
> > between gzip only and gzip+others (with the few much high growth being
> > all algorithms).
> >
> > Maybe a question here is, haven't we already validated the compression
> > header, and so don't need to do it a second time? If we really can't
> > live with a good enough heuristic, we need to work the size growth as
> > this is very much not an opt-in feature.
>
> Given these comments I'm going to hold off reviewing this series. I
> agree that getting the real uncompressed size is a nice idea, but if
> it is too expensive in terms of code size, then we might be better to
> stick with what we have. Another options is to write the uncompressed
> size as a property in the FIT image.

Thanks Simon, I think a FIT property is an attractive option,
especially for the EFI-in-FIT case that motivated this: the boot-side
cost becomes a single property read, the ITS author or build system
already knows the uncompressed size so nothing needs to parse the
stream anywhere, it works even for formats whose streams carry no
size field, and images without the property simply keep the current
8x fallback. The trade-offs are that it needs a binding addition plus
image-generation support, only images that carry the property
benefit, and the legacy uImage form of kernel_noload stays on the
heuristic (which is probably acceptable). The property value would
still need the CONFIG_SYS_BOOTM_LEN cap before allocating, same as a
header value.

To Tom's earlier question about validating the header twice: the
value is used only as an allocation hint. bootm performs only the
format-specific parsing needed to obtain the size, caps it at
CONFIG_SYS_BOOTM_LEN, and the decompressor remains authoritative for
validating and decoding the stream. But I agree the property answers
that concern even more directly, since bootm then reads nothing from
the stream at all.

On the size growth, since that was the blocker: I reworked the v1
implementation into per-format helpers that are only compiled when
the matching decompressor is enabled, and re-ran the world build
(all 1550 defconfigs, v1 and the rework applied to the same base
commit). Median growth on changed boards drops from +160 to +96
bytes, the 856 gzip-only boards go from +112 to +80, boards without
any of the formats go from +108 to zero, and 1222 of 1496 comparable
boards end up smaller than with v1. Tom, the "few go higher"
outliers in your run should be the binutils Cortex-A53 erratum
843419 workaround: each triggered veneer is padded to a full 4 KiB
page, and any few-hundred-byte change re-rolls which arm64 boards
gain or lose one, so those jumps are not code from the patch itself.

So from my side both directions are workable:

  - if the reworked cost is acceptable, I can post it as v2, split
    per format so any individual decompressor can be dropped;
  - if the property route is preferred, I am happy to prototype that
    instead (binding plus the bootm side, with the 8x fallback for
    images without the property) and hold the rework.

The two also compose rather than conflict: with both in place bootm
would prefer the property, then the stream header, then the 8x
fallback, so picking one now does not rule out adding the other
later. I have not listed the combination as a third option only
because its boot-side cost is the sum of the two, so it becomes
interesting once the header cost itself is judged acceptable.

Which would you prefer?

>
> Regards,
> Simon

Regards,
Aristo