[PATCH 05/11] boot: imagemap: do not resize a region the caller owns

[Simon Glass]

Hi Daniel,

On 2026-09-29T00:04:16, Daniel Golle <daniel at makrotopia.org> wrote:
> boot: imagemap: do not resize a region the caller owns
>
> The extend path reserves memory at the recorded address and re-reads the
> region into it. For a region recorded by imagemap_map_to() that address
> belongs to the caller, which asked for the payload to be placed there:
> imagemap reserves memory it was never given and overwrites whatever the
> caller had put at the destination.
>
> Restrict the extend path to regions allocated from LMB by imagemap
> itself. A larger request against a caller-owned region now falls through
> to a fresh allocation, leaving the caller's memory untouched.
>
> Fixes: 9b41c7dbee96 ("boot: add imagemap on-demand loading from storage")
> Signed-off-by: Daniel Golle <daniel at makrotopia.org>
>
> boot/imagemap.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

> diff --git a/boot/imagemap.c b/boot/imagemap.c
> @@ -233,13 +233,18 @@ void *imagemap_map(struct udevice *dev, loff_t img_offset, ulong size)
> +                     /*
> +                      * Only a region this device allocated may be resized;
> +                      * one recorded by imagemap_map_to() lives at an address
> +                      * the caller chose and owns.
> +                      */
> +                     if (!r->lmb_reserved)
> +                             break;

The logic looks right to me. imagemap_record() dedups on img_offset,
so there is at most one match and the break is fine.

Please can you add a test? For example, call imagemap_map_to(dev, 0,
64, dst) with a guard pattern in dst beyond 64 bytes, then
imagemap_map(dev, 0, 256), and check that the guard bytes are
unchanged, the returned pointer is not dst, and the new region is
LMB-reserved. Without a test this is easy to break again.

It may also be worth saying in the commit message that the
fall-through replaces the caller's record in the table, via
imagemap_record(). A later imagemap_map_to() to the same dst then
misses the 'already mapped' shortcut and reads the data again. That is
harmless, but not obvious from the description.

> +                     lmb_free(addr, ALIGN(r->size, ARCH_DMA_MINALIGN),
> +                              LMB_NONE);

BTW, this is not from your patch, but it is the same class of bug the
series is fixing. The old reservation is freed here, but r is only
updated on success. Both the -ENOMEM and read-failure exits leave
r->lmb_reserved true and r->ram pointing at memory that is no longer
reserved. imagemap_cleanup() then frees it a second time, which could
drop someone else's reservation, and a later imagemap_lookup() can
return a pointer into unreserved memory. On the in-place read-failure
path, r also still claims r->size valid bytes, although the partial
read may have overwritten them. Perhaps those exits should drop the
entry or re-reserve the old range? After patch 6, the in-place case
could instead shrink the reservation back to old_size, since those
bytes are still valid. It could go in this series or the follow-up,
whichever you prefer.

Regards,
Simon