[PATCH 04/11] boot: imagemap: free the reservation a replaced record owned

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel at makrotopia.org> wrote:
> boot: imagemap: free the reservation a replaced record owned
>
> imagemap_record() overwrites an entry that already covers the same image
> offset, including its lmb_reserved flag. When imagemap_map_to() replaces
> a region that imagemap_map() had allocated from LMB, the reservation is
> forgotten with the address that owned it, so imagemap_cleanup() walks a
> table that no longer mentions it and the memory stays reserved for the
> rest of the boot.
>
> Release the reservation before the entry is pointed at different memory.
>
> Fixes: 9b41c7dbee96 ("boot: add imagemap on-demand loading from storage")
> Signed-off-by: Daniel Golle <daniel at makrotopia.org>
>
> boot/imagemap.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)

> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -114,12 +114,22 @@ imagemap_record(struct udevice *dev, loff_t img_offset, ulong size,
> +             /*
> +              * The entry is about to describe different memory, so give up
> +              * the reservation it owns while its address is still known.
> +              */
> +             if (r->lmb_reserved && r->ram != ram)
> +                     lmb_free(map_to_sysmem(r->ram),
> +                              ALIGN(r->size, ARCH_DMA_MINALIGN), LMB_NONE);

This fixes the leak but adds a lifetime problem. imagemap_map() has
already given its caller a pointer into the old buffer, which used to
stay valid until imagemap_cleanup(). Now the memory goes back to LMB
while the caller may still hold the pointer, and a later lmb_alloc()
can hand it out again, e.g. for the FDT or ramdisk relocation in
bootm. One way to hit this is two FIT images sharing the same external
data position, where only one has a load address. The first gets an
imagemap buffer, which fit_image_get_data() returns via
imagemap_lookup(). The second then calls imagemap_map_to() at the same
offset and frees that buffer under the first.

My suggestion is to stop imagemap_map_to() overwriting an LMB-owned
entry. If the existing entry is lmb_reserved and the memory differs,
append a new entry instead. imagemap_cleanup() still frees the
reservation, so nothing leaks, and earlier pointers stay valid until
then. What do you think?

> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -114,12 +114,22 @@ imagemap_record(struct udevice *dev, loff_t img_offset, ulong size,
> +             r->size = size;
> +             r->ram = ram;
> +             r->lmb_reserved = lmb_reserved;

Just to check: if r->ram == ram, r->lmb_reserved is true and the new
lmb_reserved is false, the flag is cleared and the reservation still
leaks. That happens if a caller passes an imagemap-owned pointer as
dst to imagemap_map_to(). Unlikely, but the fix is simple: keep the
flag when the memory doesn't change. Also, r->size can shrink here,
and the lmb_free() in imagemap_cleanup() then uses the smaller size.

> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -114,12 +114,22 @@ imagemap_record(struct udevice *dev, loff_t img_offset, ulong size,
>       /* Check for an existing entry at the same base that we can extend */

This comment no longer fits, since the loop now replaces the entry
rather than extending it. The function comment also says the entry is
updated only 'if the new size is larger', which the code has never
done. Please can you update both?

Please can you also add a test to test/boot/imagemap.c?
imagemap_test_lmb_reserve() is a good model: call imagemap_map() then
imagemap_map_to() at the same block-aligned offset, then check that
the first buffer is no longer reserved after cleanup.

Regards,
Simon