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

Tom Rini <[email protected]> Mon, 17 Aug 2026 13:24:43 -0600
Newsgroups	org.u-boot-project.lists.u-boot
Message-ID	<20260817192443.GU3297518@bill-the-cat>
On Tue, Aug 18, 2026 at 12:01:31AM +0800, Aristo Chen wrote:
> 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?

Lets see a v2 of the rework. I think some of the higher size growth I
was talking about was on the platforms which enabled multiple
algorithms, fwiw.

-- 
Tom
signature.asc (application/pgp-signature, 228 B)