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

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.

Regards,
Simon