On Tue, Sep 15, 2026 at 12:10:20PM -0700, Sriram Sriram wrote: > [email protected] > Bcc: > Subject: Re: [PATCH 2/4] boot: image-fit: add overflow guard for FIT > decompression buffer > Reply-To: > In-Reply-To: <20260910190944.GN1764417@bill-the-cat> > > On Thu, Sep 10, 2026 at 01:09:44PM -0600, Tom Rini wrote: > > On Wed, Sep 09, 2026 at 12:20:14PM -0700, Sriram Sriram wrote: > > > > > From: Drew Kluemke <[email protected]> > > > > > > Add an overflow check before the 'len * 20' multiplication used to > > > compute the maximum decompression buffer size. On platforms where len > > > exceeds ULONG_MAX / 20 the multiplication wraps to a small value, > > > leading malloc to allocate a tiny buffer while decompression writes the > > > full stream -- a heap buffer overflow. > > > OK, so lets assume an overflow, or just an invalid max_decomp_len, > > shouldn't image_decomp handle this case and fail and we catch it? > > You are right, and I should not have sent this one as written. > > max_decomp_len is used for two things -- the size of the buffer we > allocate, and the unc_len bound we hand to image_decomp(): > > if (load == data) { > loadbuf = aligned_alloc(8, max_decomp_len); > load = map_to_sysmem(loadbuf); > } else { > loadbuf = map_sysmem(load, max_decomp_len); > } > if (image_decomp(comp, load, data, image_type, > loadbuf, buf, len, max_decomp_len, &load_end)) { > > so if the multiply wraps, the buffer and the bound wrap together. They > cannot diverge in the dangerous direction, which is precisely the > scenario the commit message claims. And every arm of image_decomp() > does honour unc_len -- IH_COMP_NONE has an explicit "image_len <= > unc_len" test that returns -ENOSPC, and gzip, bzip2, lzma, lzo, lz4 and > zstd all pass it through as the output size limit -- so a too-small > bound comes back as a failure and fit_image_load() catches it with > "Error decompressing %s" and returns -ENOEXEC. That is exactly the > behaviour you describe, and it is what already happens. > > For the record on reachability: on 64-bit ULONG_MAX / 20 is about > 9.2e17, so len would have to be an exabyte-scale subimage and the > multiply can never wrap in practice. On 32-bit the threshold is about > 205 MiB, which is reachable, but per the above it degrades to a failed > decompression rather than to memory corruption. > > So please drop 2/4. I would rather withdraw the claim than try to > defend it. > > While re-reading this, two things nearby do look like real (if smaller) > problems, if you think either is worth a patch: > > 1. aligned_alloc() is never checked for NULL. A len that is merely > large, with no wrap involved, makes the allocation fail and we hand > the NULL straight to map_to_sysmem() and then into the decompressor. > The IH_TYPE_FLATDT branch a few lines below has the same shape and > follows it with a memcpy(loadbuf, buf, len). > > 2. image_decomp() takes "uint unc_len" while max_decomp_len is a ulong, > so on 64-bit the bound is silently truncated. The truncation is in > the safe direction -- the bound ends up <= the allocation -- so it is > not a bug, just surprising: we can allocate 20x and then refuse to > use most of it. > > Happy to send (1) as a standalone patch, or to leave both alone. The > other three patches in the series do not depend on this one.
I think we can leave them alone, thanks. -- Tom
signature.asc
Description: PGP signature
