[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. Thanks, Sriram
