[PATCH v2 00/11] imagemap: structural and cosmetic review follow-ups

[PATCH v2 00/11] imagemap: structural and cosmetic review follow-ups

[Daniel Golle]

The clean-up half of the v1 review of the imagemap series
(cover.1787272661.git.daniel@makrotopia.org). It applies on top of
"imagemap: fixes from the v1 review", which carries the correctness
changes and which several of these patches touch the same lines as.

Patches 1 to 4 are self-contained tidy-ups without any functional
changes: the suite list put back in alphabetical order, a derived size
for the header probe in place of a bare 64, a corrected rationale on the
imagemap_lookup() guard, and test locals hoisted out of an anonymous
block and a mid-block declaration.

Patches 5 and 6 correct the documentation: the imagemap_create()
signature, which callers a reader can actually reach, bootmeth_openwrt
described as a future consumer, the index entry sorted, and the section
on phase gating that the commit adding the file claimed was there.

Patches 7 to 10 are the structural ones which do introduce functional
changes: imagemap devices are named after the partition they cover and
hold a name driver model owns, so two mappings on one block device stay
distinct. bootm_start() sets the imagemap itself, keeping the invariant
in one function. The data-position and totalsize-alignment sequence is
shared between fit_image_get_data() and the storage path, which also
gives the latter the range check it lacked and settles the two spellings
of the alignment. Finally the two imagemap pointers compile only when
CONFIG_IMAGEMAP is on, reached through accessors that fold away when it
is off.

Patch 11 adds a new unit test Simon had asked for in his review.

The sandbox defconfig symbol ordering and the FIT_VERITY_NODENAME
rename that the v1 review also asked for are already in the tree, so
they are not separate patches in this posting.

One review point is not addressed here. Moving the imagemap out of the
global images struct and into an explicit parameter of
fit_image_get_data() means changing that function and fit_image_verify(),
both public, and their callers under arch/arm, arch/x86, arch/arm/mach-k3,
cmd/, common/ and tools/. It is also a design question rather than a
cleanup: the transparency of the verification path is exactly what lets
imagemap hook in without touching the FIT code. I would rather agree the
shape first than guess at it, so it is left out and proposed separately.

Tested on sandbox as for the first series: every commit builds, "ut
imagemap" passes 14/14 under "ut -r3", host tools build, the
documentation builds with no warnings, and a full test.py run matches
unmodified next. Built with CONFIG_IMAGEMAP=n as well, which patch 10
exists to keep working.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2: 
 - The sandbox defconfig symbol ordering and the FIT_VERITY_NODENAME
   rename that the v1 review also asked for are already in the tree, so
   the cover no longer counts them as patches in this series; they were
   not dropped between postings.
 - Patch 3: the guard rationale now says "same width" instead of
   "same rank"; on ILP32 loff_t outranks ulong and the subtraction
   stays signed, and it is the LP64 unsigned conversion that makes
   the guard essential.
 - Patch 7: the named path duplicates the caller's name directly
   instead of copying it through a fixed-size buffer that truncated
   it.
 - Patch 9: the helper is renamed fit_image_get_ext_data_offset() to
   match its siblings, fit_image_get_data() folds the embedded case
   into an early return, and the message explains the dropped
   signed-value guard.
 - Patch 10: adds bootm_info_set_imagemap(), keeps the bootm_info_*
   guards in step with bootm_imagemap(), keeps the device in a local
   in the storage path, and shares the release between the OS-GO and
   error paths in a helper.
 - Patch 11: the scenario comment no longer names the reviewer, the
   bare 64 and 256 become named constants, and the pointer
   comparison drops its casts.
 - Patches 1, 2, 4, 5, 6 and 8: no changes.

Daniel Golle (11):
  test: cmd_ut: keep the suite list alphabetical
  boot: bootm: derive the header probe size
  boot: imagemap: correct the lookup guard rationale
  test: boot: imagemap: declare locals at the top
  doc: imagemap: describe the API and phase gating
  doc: develop: sort the imagemap index entry
  boot: imagemap: name the device after its partition
  boot: bootm: set the imagemap from bootm_start()
  boot: fit: share the external-data offset calculation
  boot: build the imagemap pointers only when enabled
  test: boot: imagemap: caller-owned region not resized

 boot/bootm.c             |  55 ++++++------
 boot/image-fit.c         | 175 +++++++++++++++++++++------------------
 boot/imagemap.c          |  37 +++++++--
 doc/develop/imagemap.rst |  30 +++++--
 doc/develop/index.rst    |   2 +-
 include/bootm.h          |  31 +++++++
 include/image.h          |  31 +++++++
 test/boot/imagemap.c     | 121 +++++++++++++++++++++++----
 test/cmd_ut.c            |   4 +-
 9 files changed, 351 insertions(+), 135 deletions(-)


base-commit: e36b675ebc4c41f1f4187c46d363d581c371d538
-- 
2.56.0

[PATCH v2 01/11] test: cmd_ut: keep the suite list alphabetical

[Daniel Golle]

The imagemap suite was appended after setexpr in both the declaration
and the registration list, which are otherwise alphabetical.

Move it next to image_fdt.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2: No changes since v1.

 test/cmd_ut.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/cmd_ut.c b/test/cmd_ut.c
index fab55d5be8c..966cf44c511 100644
--- a/test/cmd_ut.c
+++ b/test/cmd_ut.c
@@ -64,6 +64,7 @@ SUITE_DECL(fit_verity);
 SUITE_DECL(font);
 SUITE_DECL(hush);
 SUITE_DECL(image_fdt);
+SUITE_DECL(imagemap);
 SUITE_DECL(lib);
 SUITE_DECL(loadm);
 SUITE_DECL(log);
@@ -74,7 +75,6 @@ SUITE_DECL(optee);
 SUITE_DECL(pci_mps);
 SUITE_DECL(seama);
 SUITE_DECL(setexpr);
-SUITE_DECL(imagemap);
 SUITE_DECL(upl);
 
 static struct suite suites[] = {
@@ -95,6 +95,7 @@ static struct suite suites[] = {
 	SUITE(font, "font command"),
 	SUITE(hush, "hush behaviour"),
 	SUITE(image_fdt, "image fdt parsing"),
+	SUITE(imagemap, "imagemap on-demand storage loading"),
 	SUITE(lib, "library functions"),
 	SUITE(loadm, "loadm command parameters and loading memory blob"),
 	SUITE(log, "logging functions"),
@@ -105,7 +106,6 @@ static struct suite suites[] = {
 	SUITE(pci_mps, "PCI Express Maximum Payload Size"),
 	SUITE(seama, "seama command parameters loading and decoding"),
 	SUITE(setexpr, "setexpr command"),
-	SUITE(imagemap, "imagemap on-demand storage loading"),
 	SUITE(upl, "Universal payload support"),
 };
 
-- 
2.56.0

[PATCH v2 02/11] boot: bootm: derive the header probe size

[Daniel Golle]

boot_get_kernel() maps a bare 64 bytes to identify the image format.
genimg_get_format() reads at most a 40-byte FDT header or an 8-byte
Android magic, but the legacy branch that follows verifies the whole
64-byte legacy header, so that is the size the mapping has to cover.

Say so with sizeof(struct legacy_img_hdr).

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2: No changes since v1.

 boot/bootm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/boot/bootm.c b/boot/bootm.c
index 92c8452a5d3..b9aa83c3328 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -158,7 +158,8 @@ static int boot_get_kernel(const char *addr_fit, struct bootm_headers *images,
 		 * parsed any #config / :subimage suffix so the FIT
 		 * selection variables are populated.
 		 */
-		buf = imagemap_map(images->imagemap, 0, 64);
+		buf = imagemap_map(images->imagemap, 0,
+				   sizeof(struct legacy_img_hdr));
 		if (IS_ERR(buf)) {
 			puts("Cannot read image header from storage\n");
 			return PTR_ERR(buf);
-- 
2.56.0

[PATCH v2 03/11] boot: imagemap: correct the lookup guard rationale

[Daniel Golle]

The comment on imagemap_lookup() attributes the need for its second
guard to ulong and loff_t sharing a rank on LP64. The rank wording is
wrong: loff_t is a signed long long, which outranks ulong on ILP32, so
the subtraction in (3) stays signed there and an underflowing offset
simply fails the size comparison. It is on LP64, where the two have
the same width but different signedness, that the usual arithmetic
conversions turn the subtraction unsigned and make it wrap, letting
every range appear to fit.

State the actual reason.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2: The "same rank" wording is corrected to "same width": on ILP32
    loff_t outranks ulong and the subtraction stays signed, on LP64 the
    usual arithmetic conversions turn it unsigned and it wraps.

 boot/imagemap.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/boot/imagemap.c b/boot/imagemap.c
index b004aa1644b..0a156e010ec 100644
--- a/boot/imagemap.c
+++ b/boot/imagemap.c
@@ -72,10 +72,11 @@ void *imagemap_lookup(struct udevice *dev, loff_t img_offset, ulong size)
 		 *
 		 *  2) img_offset - r->img_offset <= r->size
 		 *     The offset into the region does not exceed the
-		 *     region size.  This guard is essential: without it,
-		 *     the subtraction in (3) wraps to a huge value on
-		 *     LP64 where ulong and loff_t have the same rank
-		 *     and the arithmetic is performed as unsigned.
+		 *     region size.  This guard is essential: on LP64,
+		 *     ulong and loff_t have the same width, so the usual
+		 *     arithmetic conversions turn the subtraction in (3)
+		 *     unsigned and it wraps, making every range appear to
+		 *     fit.
 		 *
 		 *  3) size <= r->size - (img_offset - r->img_offset)
 		 *     The requested range fits in the remaining space.
-- 
2.56.0

[PATCH v2 04/11] test: boot: imagemap: declare locals at the top

[Daniel Golle]

Two tests declare variables away from the top of the function body: the
extend test wraps its last assertions in an anonymous block to scope p3
and p2_end, and the out-of-bounds test declares dst after a statement.
Neither shape is used elsewhere in U-Boot.

Hoist all four to the declaration block.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2: No changes since v1.

 test/boot/imagemap.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/test/boot/imagemap.c b/test/boot/imagemap.c
index de00a174470..c7289db3995 100644
--- a/test/boot/imagemap.c
+++ b/test/boot/imagemap.c
@@ -231,7 +231,8 @@ static int imagemap_test_map_extend(struct unit_test_state *uts)
 	struct imagemap_priv *priv;
 	struct mock_priv *mock;
 	u8 image[IMAGE_SIZE];
-	void *p1, *p2;
+	void *p1, *p2, *p3;
+	ulong p2_end;
 
 	for (int i = 0; i < IMAGE_SIZE; i++)
 		image[i] = (u8)(i & 0xff);
@@ -264,17 +265,15 @@ static int imagemap_test_map_extend(struct unit_test_state *uts)
 	 * With LMB allocation this cannot happen because each allocation
 	 * reserves distinct memory.
 	 */
-	{
-		void *p3 = imagemap_map(dev, 256, 128);
-		ulong p2_end = map_to_sysmem(p2) + 256;
-
-		ut_assert(!IS_ERR(p3));
-		ut_asserteq(2, priv->regions.count);
-		/* New region must not overlap the extended first region */
-		ut_assert(map_to_sysmem(p3) >= p2_end ||
-			  map_to_sysmem(p3) + 128 <= map_to_sysmem(p2));
-		ut_asserteq_mem(image + 256, p3, 128);
-	}
+	p3 = imagemap_map(dev, 256, 128);
+	p2_end = map_to_sysmem(p2) + 256;
+
+	ut_assert(!IS_ERR(p3));
+	ut_asserteq(2, priv->regions.count);
+	/* New region must not overlap the extended first region */
+	ut_assert(map_to_sysmem(p3) >= p2_end ||
+		  map_to_sysmem(p3) + 128 <= map_to_sysmem(p2));
+	ut_asserteq_mem(image + 256, p3, 128);
 
 	imagemap_cleanup(dev);
 
@@ -455,6 +454,7 @@ static int imagemap_test_read_oob(struct unit_test_state *uts)
 {
 	struct udevice *dev;
 	u8 image[IMAGE_SIZE];
+	u8 dst[64];
 	void *p;
 
 	ut_assertok(create_mock_loader(image, IMAGE_SIZE, &dev));
@@ -464,8 +464,6 @@ static int imagemap_test_read_oob(struct unit_test_state *uts)
 	ut_assert(IS_ERR(p));
 
 	/* map_to should also fail */
-	u8 dst[64];
-
 	p = imagemap_map_to(dev, IMAGE_SIZE - 32, 64, dst);
 	ut_assert(IS_ERR(p));
 
-- 
2.56.0

[PATCH v2 05/11] doc: imagemap: describe the API and phase gating

[Daniel Golle]

The imagemap_create() entry omits the devp out parameter the function
actually takes, the boot-integration list names boot_get_kernel() and
imagemap_map_to() without saying which caller a reader can reach, and
bootmeth_openwrt is described as the primary consumer although it is not
in tree.

Correct the signature, name the callers, and add the section on phase
gating that the commit adding this file already claimed was here.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2: No changes since v1.

 doc/develop/imagemap.rst | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/doc/develop/imagemap.rst b/doc/develop/imagemap.rst
index f1fea1bba78..66c0db9e8a8 100644
--- a/doc/develop/imagemap.rst
+++ b/doc/develop/imagemap.rst
@@ -13,9 +13,9 @@ Architecture Overview
 The framework is built around the ``UCLASS_IMAGEMAP`` device class
 (``include/imagemap.h``), which provides:
 
-``imagemap_create(dev, name, part)``
+``imagemap_create(dev, name, part, devp)``
    Create an imagemap device over a partition of block device *dev*,
-   selected by *name* or by index *part*.
+   selected by *name* or by index *part*, and return it through *devp*.
 
 ``imagemap_map(dev, offset, size)``
    Return a pointer to a buffer holding the requested region.
@@ -76,6 +76,16 @@ they do for the rest of U-Boot; on NOR a partition is read linearly and
 end-to-end integrity is provided by the FIT hash or signature, so no
 MTD-specific read path is needed in imagemap.
 
+Build Phases
+------------
+
+``CONFIG_IMAGEMAP`` builds only in full U-Boot.  Nothing in the xpl
+phases sets ``images->imagemap``, so SPL and TPL would carry the code
+without ever reaching it, and their FIT loading is unaffected.  The
+``struct spl_load_info`` reader and ``spl_load_region()`` that imagemap
+builds on are shared with SPL rather than duplicated, which is why they
+live in ``include/spl.h``.
+
 Boot Integration
 ----------------
 
@@ -85,16 +95,20 @@ The ``imagemap`` framework integrates with the boot pipeline through
 1. The imagemap device pointer is stored in ``struct bootm_info``
    and propagated to the global ``images`` structure.
 
-2. ``boot_get_kernel()`` uses ``imagemap_map()`` to read the image
-   header and FIT metadata without loading the full image into RAM.
+2. While finding the OS, ``imagemap_map()`` reads the image header and
+   FIT metadata without loading the full image into RAM.  This happens
+   in ``boot_get_kernel()``, an internal helper of ``bootm_find_os()``.
 
 3. ``fit_image_load()`` uses the on-demand storage path via
    ``images->imagemap`` to stream individual sub-images directly to
-   their final load addresses.
+   their final load addresses.  Its internal
+   ``fit_image_load_storage()`` calls ``imagemap_map_to()`` for an
+   uncompressed sub-image that has a load address, and
+   ``imagemap_map()`` otherwise.
 
 4. Before jumping to the OS, ``imagemap_cleanup()`` releases all
    allocated resources.
 
-The ``bootmeth_openwrt`` boot method is the primary consumer of this
-framework, using it to boot OpenWrt-style FIT firmware images stored
-directly on raw storage.
+The ``bootmeth_openwrt`` boot method will be the first in-tree consumer
+of this framework, using it to boot OpenWrt-style FIT firmware images
+stored directly on raw storage.
-- 
2.56.0

[PATCH v2 06/11] doc: develop: sort the imagemap index entry

[Daniel Golle]

The imagemap entry sits between bloblist and bootstd/index, where the
surrounding list is alphabetical.

Move it after global_data.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2: No changes since v1.

 doc/develop/index.rst | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/develop/index.rst b/doc/develop/index.rst
index 3b4cc2e28a7..cb172b66b12 100644
--- a/doc/develop/index.rst
+++ b/doc/develop/index.rst
@@ -31,7 +31,6 @@ Implementation
 
    directories
    bloblist
-   imagemap
    bootstd/index
    ci_testing
    commands
@@ -46,6 +45,7 @@ Implementation
    cedit
    event
    global_data
+   imagemap
    lmb
    logging
    makefiles
-- 
2.56.0

[PATCH v2 07/11] boot: imagemap: name the device after its partition

[Daniel Golle]

imagemap_create() names every device it binds by index "imagemap", so
two mappings taken on one block device appear as two identically named
children and the device tree does not say which partition each covers.
The name also comes straight from the caller, whose string has to outlive
the device for the driver-model pointer to stay valid.

Derive the name from the partition, by name where one was given and by
index otherwise, and hand driver model an allocated copy it releases on
unbind.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2: The named path no longer copies the caller's name through a
    fixed-size buffer, which truncated it at 31 characters; the name is
    duplicated directly and the buffer is only used for the
    "imagemap%d" index spelling.

 boot/imagemap.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/boot/imagemap.c b/boot/imagemap.c
index 0a156e010ec..d46c1430fb7 100644
--- a/boot/imagemap.c
+++ b/boot/imagemap.c
@@ -11,6 +11,7 @@
 #include <dm.h>
 #include <imagemap.h>
 #include <lmb.h>
+#include <malloc.h>
 #include <mapmem.h>
 #include <memalign.h>
 #include <part.h>
@@ -442,6 +443,9 @@ int imagemap_create(struct udevice *dev, const char *name,
 	struct imagemap_plat *plat;
 	struct disk_partition info;
 	struct udevice *imdev;
+	char buf[32];
+	char *devname;
+	const char *base;
 	int ret;
 
 	if (name && *name)
@@ -451,10 +455,28 @@ int imagemap_create(struct udevice *dev, const char *name,
 	if (ret < 0)
 		return ret;
 
-	ret = device_bind_driver(desc->bdev, "imagemap",
-				 name && *name ? name : "imagemap", &imdev);
-	if (ret)
+	/*
+	 * Name the device after the partition it covers, so that two mappings
+	 * on one block device stay distinct in the device tree.
+	 */
+	if (name && *name) {
+		base = name;
+	} else {
+		snprintf(buf, sizeof(buf), "imagemap%d", part);
+		base = buf;
+	}
+
+	/* device_bind_driver() keeps the pointer, so hand it one DM can own */
+	devname = strdup(base);
+	if (!devname)
+		return -ENOMEM;
+
+	ret = device_bind_driver(desc->bdev, "imagemap", devname, &imdev);
+	if (ret) {
+		free(devname);
 		return ret;
+	}
+	device_set_name_alloced(imdev);
 
 	plat = dev_get_plat(imdev);
 	plat->part_start = info.start;
-- 
2.56.0

[PATCH v2 08/11] boot: bootm: set the imagemap from bootm_start()

[Daniel Golle]

bootm_run_states() puts the imagemap back into the global images struct
after bootm_start() has zeroed it, which leaves the invariant split
across two functions: any later state that calls bootm_start() again
drops the pointer, and BOOTM_STATE_RESTART is already close to doing so.

Pass the imagemap to bootm_start() and let it set the field while it
initialises the rest of the struct.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2: No changes since v1.

 boot/bootm.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/boot/bootm.c b/boot/bootm.c
index b9aa83c3328..67df647d176 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -275,7 +275,7 @@ static int boot_get_kernel(const char *addr_fit, struct bootm_headers *images,
 	return 0;
 }
 
-static int bootm_start(void)
+static int bootm_start(struct udevice *imagemap)
 {
 	/*
 	 * Free dm-verity allocations from a prior boot attempt before
@@ -286,6 +286,7 @@ static int bootm_start(void)
 	fit_verity_free(&images);
 	memset((void *)&images, 0, sizeof(images));
 	images.verify = env_get_yesno("verify");
+	images.imagemap = imagemap;
 
 	bootstage_mark_name(BOOTSTAGE_ID_BOOTM_START, "bootm_start");
 	images.state = BOOTM_STATE_START;
@@ -1198,15 +1199,8 @@ int bootm_run_states(struct bootm_info *bmi, int states)
 	 * Work through the states and see how far we get. We stop on
 	 * any error.
 	 */
-	if (states & BOOTM_STATE_START) {
-		ret = bootm_start();
-		/*
-		 * bootm_start() zeroes the global images struct. Restore
-		 * the loader pointer so the storage-backed path works.
-		 */
-		if (IS_ENABLED(CONFIG_IMAGEMAP) && bmi->imagemap)
-			images->imagemap = bmi->imagemap;
-	}
+	if (states & BOOTM_STATE_START)
+		ret = bootm_start(bmi->imagemap);
 
 	if (!ret && (states & BOOTM_STATE_PRE_LOAD)) {
 		/* Pre-load verification is not applicable to storage boot */
-- 
2.56.0

[PATCH v2 09/11] boot: fit: share the external-data offset calculation

[Daniel Golle]

fit_image_load_storage() repeats the data-position / data-offset /
totalsize-alignment sequence that fit_image_get_data() performs, and
writes the result through an int pointer where the getters expect a u32.
The copy also omits the range check on the sum, so an offset that the
embedded path rejects is accepted here.

Extract fit_image_get_ext_data_offset() and call it from both. The
storage path gains the range check and both agree on one expression for
the alignment. The signed comparison of the offset and size is
meaningless once both are u32, so it is dropped in favour of the shared
range check the helper provides.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2:
 - Rename the helper to fit_image_get_ext_data_offset() to match the
   sibling fit_image_get_*() getters.
 - fit_image_get_data() folds the embedded case into an early return
   on -ENOENT, dropping the external_data local.
 - Note in the message that the signed-value guard in the storage
   path is dead once data_off and data_sz are u32.

 boot/image-fit.c | 166 ++++++++++++++++++++++++++---------------------
 1 file changed, 91 insertions(+), 75 deletions(-)

diff --git a/boot/image-fit.c b/boot/image-fit.c
index 6ab6c6c7da9..0367331c25d 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -1056,6 +1056,47 @@ int fit_image_get_data_size_unciphered(const void *fit, int noffset,
 	return 0;
 }
 
+/**
+ * fit_image_get_ext_data_offset - locate a sub-image's external data
+ * @fit: pointer to the FIT format image header
+ * @noffset: component image node offset
+ * @offsetp: holds the data offset from the start of the FIT, on success
+ *
+ * returns:
+ *     0, if the sub-image has external data
+ *     -ENOENT, if its data is embedded in the FIT
+ *     -EINVAL, if the recorded offset is out of range
+ */
+static int fit_image_get_ext_data_offset(const void *fit, int noffset, u32 *offsetp)
+{
+	size_t fdt_total_size_aligned;
+	u32 offset;
+
+	if (!fit_image_get_data_position(fit, noffset, &offset)) {
+		*offsetp = offset;
+
+		return 0;
+	}
+
+	if (fit_image_get_data_offset(fit, noffset, &offset))
+		return -ENOENT;
+
+	/*
+	 * data-offset counts from the end of the FDT, so that is the base for
+	 * every image; the sum has to stay within a u32.
+	 */
+	fdt_total_size_aligned = ALIGN(fdt_totalsize(fit), 4);
+	if (fdt_total_size_aligned > UINT32_MAX - offset) {
+		printf("Invalid external data offset: %u\n", offset);
+
+		return -EINVAL;
+	}
+
+	*offsetp = offset + fdt_total_size_aligned;
+
+	return 0;
+}
+
 /**
  * fit_image_get_data - get data and its size including
  *				 both embedded and external data
@@ -1075,82 +1116,63 @@ int fit_image_get_data_size_unciphered(const void *fit, int noffset,
 int fit_image_get_data(const void *fit, int noffset, const void **data,
 		       size_t *size)
 {
-	bool external_data = false;
 	int ret;
-	size_t fdt_total_size_aligned;
 	u32 offset;
 	u32 len;
 	uintptr_t max_offset;
 
-	if (!fit_image_get_data_position(fit, noffset, &offset)) {
-		external_data = true;
-	} else if (!fit_image_get_data_offset(fit, noffset, &offset)) {
-		/*
-		 * For FIT with external data, figure out where
-		 * the external images start. This is the base
-		 * for the data-offset properties in each image.
-		 */
-		fdt_total_size_aligned = ((fdt_totalsize(fit) + 3) & ~3);
-		/* The resulting offset cannot exceed UINT32_MAX */
-		if (fdt_total_size_aligned > UINT32_MAX - offset) {
-			printf("Invalid external data offset: %u\n", offset);
-			return -EINVAL;
-		}
-		offset += fdt_total_size_aligned;
-
-		external_data = true;
-	}
+	ret = fit_image_get_ext_data_offset(fit, noffset, &offset);
+	if (ret == -ENOENT)
+		return fit_image_get_emb_data(fit, noffset, data, size);
+	if (ret)
+		return ret;
 
-	if (external_data) {
-		debug("External Data\n");
+	debug("External Data\n");
 
-		max_offset = UINTPTR_MAX - (uintptr_t)fit;
-		/* Check that external data offset is within the addressable range */
-		if (offset > max_offset) {
-			printf("Invalid external data offset: %u\n", offset);
-			return -EINVAL;
-		}
+	max_offset = UINTPTR_MAX - (uintptr_t)fit;
+	/* Check that external data offset is within the addressable range */
+	if (offset > max_offset) {
+		printf("Invalid external data offset: %u\n", offset);
+		return -EINVAL;
+	}
 
-		ret = fit_image_get_data_size(fit, noffset, &len);
-		if (!ret) {
-			/*
-			 * For non-signed FIT images, we can only check that
-			 * (offset + len) doesn't exceed the addressable range.
-			 * For signed FITs, we can additionally check that
-			 * (offset + len) doesn't exceed the allowed FIT image
-			 * maximum size.
-			 */
-			if (len > max_offset - offset
-			/*
-			 * #if (not a runtime if) is required: FIT_SIGNATURE_MAX_SIZE
-			 * depends on FIT_SIGNATURE, so CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE)
-			 * is undefined when signing is disabled and referencing it
-			 * here would fail to compile.
-			 */
+	ret = fit_image_get_data_size(fit, noffset, &len);
+	if (!ret) {
+		/*
+		 * For non-signed FIT images, we can only check that
+		 * (offset + len) doesn't exceed the addressable range.
+		 * For signed FITs, we can additionally check that
+		 * (offset + len) doesn't exceed the allowed FIT image
+		 * maximum size.
+		 */
+		if (len > max_offset - offset
+		/*
+		 * #if (not a runtime if) is required: FIT_SIGNATURE_MAX_SIZE
+		 * depends on FIT_SIGNATURE, so CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE)
+		 * is undefined when signing is disabled and referencing it
+		 * here would fail to compile.
+		 */
 #if CONFIG_IS_ENABLED(FIT_SIGNATURE)
-			    || offset > CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE) ||
-			    len > CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE) - offset
+		    || offset > CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE) ||
+		    len > CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE) - offset
 #endif
-			) {
-				printf("FIT external data is out of bounds (offset=%u, size=%u)\n",
-				       offset, len);
-				return -EINVAL;
-			}
-			*data = fit + offset;
+		) {
+			printf("FIT external data is out of bounds (offset=%u, size=%u)\n",
+			       offset, len);
+			return -EINVAL;
+		}
+		*data = fit + offset;
 #if !defined(USE_HOSTCC) && CONFIG_IS_ENABLED(IMAGEMAP)
-			if (images.imagemap) {
-				void *mapped;
+		if (images.imagemap) {
+			void *mapped;
 
-				mapped = imagemap_lookup(images.imagemap,
-							 offset, len);
-				if (mapped)
-					*data = mapped;
-			}
-#endif
-			*size = len;
+			mapped = imagemap_lookup(images.imagemap,
+						 offset, len);
+			if (mapped)
+				*data = mapped;
 		}
-	} else {
-		ret = fit_image_get_emb_data(fit, noffset, data, size);
+#endif
+		*size = len;
 	}
 
 	return ret;
@@ -2185,12 +2207,12 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit,
 				  int noffset, enum fit_load_op load_op,
 				  ulong *datap, ulong *lenp, bool *early)
 {
-	int data_off = 0, data_sz = 0;
-	bool external = false;
+	u32 data_off = 0, data_sz = 0;
 	ulong img_load;
 	u8 img_comp = IH_COMP_NONE;
 	void *mapped;
 	void *dst;
+	int ret;
 
 	if (CONFIG_IS_ENABLED(FIT_VERITY)) {
 		u8 img_type;
@@ -2207,19 +2229,13 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit,
 		}
 	}
 
-	if (!fit_image_get_data_position(fit, noffset, &data_off)) {
-		external = true;
-	} else if (!fit_image_get_data_offset(fit, noffset, &data_off)) {
-		external = true;
-		data_off += ALIGN(fdt_totalsize(fit), 4);
-	}
+	ret = fit_image_get_ext_data_offset(fit, noffset, &data_off);
+	if (ret)
+		return ret == -ENOENT ? 0 : ret;
 
-	if (!external || fit_image_get_data_size(fit, noffset, &data_sz))
+	if (fit_image_get_data_size(fit, noffset, &data_sz))
 		return 0;
 
-	if (data_off < 0 || data_sz < 0)
-		return -EINVAL;
-
 	fit_image_get_comp(fit, noffset, &img_comp);
 
 	if (img_comp == IH_COMP_NONE && load_op != FIT_LOAD_IGNORED &&
-- 
2.56.0

[PATCH v2 10/11] boot: build the imagemap pointers only when enabled

[Daniel Golle]

struct bootm_headers and struct bootm_info carry an imagemap pointer
whatever the configuration, so every board pays for it with the feature
off, and each use site has to spell out an IS_ENABLED() test beside the
NULL check.

Compile the two fields under CONFIG_IMAGEMAP and reach them through
bootm_imagemap(), bootm_set_imagemap(), bootm_info_imagemap() and
bootm_info_set_imagemap(), which fold to NULL and to nothing when it
is off.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2:
 - Add bootm_info_set_imagemap() alongside the getter, so a future
   caller does not have to spell the ifdef itself.
 - The bootm_info_* accessors now carry the same !defined(USE_HOSTCC)
   guard as bootm_imagemap(); tools/image-host.c pulls in bootm.h.
 - fit_image_load_storage() keeps the device in a local instead of
   calling bootm_imagemap() twice.
 - The imagemap release in the OS-GO and error paths moves into the
   shared bootm_imagemap_cleanup() helper.

 boot/bootm.c     | 44 ++++++++++++++++++++++++++++----------------
 boot/image-fit.c | 13 +++++++------
 include/bootm.h  | 31 +++++++++++++++++++++++++++++++
 include/image.h  | 31 +++++++++++++++++++++++++++++++
 4 files changed, 97 insertions(+), 22 deletions(-)

diff --git a/boot/bootm.c b/boot/bootm.c
index 67df647d176..a15296c8b8d 100644
--- a/boot/bootm.c
+++ b/boot/bootm.c
@@ -151,14 +151,14 @@ static int boot_get_kernel(const char *addr_fit, struct bootm_headers *images,
 
 	/* check image type, for FIT images get FIT kernel node */
 	*os_data = *os_len = 0;
-	if (IS_ENABLED(CONFIG_IMAGEMAP) && images->imagemap) {
+	if (bootm_imagemap(images)) {
 		/*
 		 * Storage path: read enough bytes to detect the image
 		 * format. genimg_get_kernel_addr_fit() above still
 		 * parsed any #config / :subimage suffix so the FIT
 		 * selection variables are populated.
 		 */
-		buf = imagemap_map(images->imagemap, 0,
+		buf = imagemap_map(bootm_imagemap(images), 0,
 				   sizeof(struct legacy_img_hdr));
 		if (IS_ERR(buf)) {
 			puts("Cannot read image header from storage\n");
@@ -213,14 +213,14 @@ static int boot_get_kernel(const char *addr_fit, struct bootm_headers *images,
 #endif
 #if CONFIG_IS_ENABLED(FIT)
 	case IMAGE_FORMAT_FIT:
-		if (IS_ENABLED(CONFIG_IMAGEMAP) && images->imagemap) {
+		if (bootm_imagemap(images)) {
 			/*
 			 * Extend the mapping to cover the full FIT
 			 * FDT structure so all metadata is accessible.
 			 */
 			size_t fdt_sz = fdt_totalsize(buf);
 
-			buf = imagemap_map(images->imagemap, 0, fdt_sz);
+			buf = imagemap_map(bootm_imagemap(images), 0, fdt_sz);
 			if (IS_ERR(buf)) {
 				puts("Cannot read FIT header from storage\n");
 				return PTR_ERR(buf);
@@ -286,7 +286,7 @@ static int bootm_start(struct udevice *imagemap)
 	fit_verity_free(&images);
 	memset((void *)&images, 0, sizeof(images));
 	images.verify = env_get_yesno("verify");
-	images.imagemap = imagemap;
+	bootm_set_imagemap(&images, imagemap);
 
 	bootstage_mark_name(BOOTSTAGE_ID_BOOTM_START, "bootm_start");
 	images.state = BOOTM_STATE_START;
@@ -1186,6 +1186,24 @@ unmap_image:
 	return ret;
 }
 
+/**
+ * bootm_imagemap_cleanup() - release the on-demand storage loader
+ * @images: Boot images being used
+ *
+ * Tears down the imagemap device and clears the pointer, but only when one
+ * was in use. The two call sites (the OS-GO path and the error path) share
+ * this so the release logic lives in one place.
+ */
+static void bootm_imagemap_cleanup(struct bootm_headers *images)
+{
+	struct udevice *imagemap = bootm_imagemap(images);
+
+	if (imagemap) {
+		imagemap_cleanup(imagemap);
+		bootm_set_imagemap(images, NULL);
+	}
+}
+
 int bootm_run_states(struct bootm_info *bmi, int states)
 {
 	struct bootm_headers *images = bmi->images;
@@ -1200,11 +1218,11 @@ int bootm_run_states(struct bootm_info *bmi, int states)
 	 * any error.
 	 */
 	if (states & BOOTM_STATE_START)
-		ret = bootm_start(bmi->imagemap);
+		ret = bootm_start(bootm_info_imagemap(bmi));
 
 	if (!ret && (states & BOOTM_STATE_PRE_LOAD)) {
 		/* Pre-load verification is not applicable to storage boot */
-		if (!IS_ENABLED(CONFIG_IMAGEMAP) || !images->imagemap)
+		if (!bootm_imagemap(images))
 			ret = bootm_pre_load(bmi->addr_img);
 	}
 
@@ -1214,7 +1232,7 @@ int bootm_run_states(struct bootm_info *bmi, int states)
 	if (!ret && (states & BOOTM_STATE_FINDOTHER)) {
 		ulong img_addr;
 
-		if (IS_ENABLED(CONFIG_IMAGEMAP) && images->imagemap)
+		if (bootm_imagemap(images))
 			img_addr = images->os.start;
 		else
 			img_addr = bmi->addr_img ? hextoul(bmi->addr_img, NULL)
@@ -1318,10 +1336,7 @@ int bootm_run_states(struct bootm_info *bmi, int states)
 	/* Now run the OS! We hope this doesn't return */
 	if (!ret && (states & BOOTM_STATE_OS_GO)) {
 		/* Release storage backend before jumping - no return expected */
-		if (IS_ENABLED(CONFIG_IMAGEMAP) && images->imagemap) {
-			imagemap_cleanup(images->imagemap);
-			images->imagemap = NULL;
-		}
+		bootm_imagemap_cleanup(images);
 
 		ret = boot_selected_os(BOOTM_STATE_OS_GO, bmi, boot_fn);
 	}
@@ -1329,10 +1344,7 @@ int bootm_run_states(struct bootm_info *bmi, int states)
 	/* Deal with any fallout */
 err:
 	/* Clean up imagemap on error (not reached on successful boot) */
-	if (IS_ENABLED(CONFIG_IMAGEMAP) && images->imagemap) {
-		imagemap_cleanup(images->imagemap);
-		images->imagemap = NULL;
-	}
+	bootm_imagemap_cleanup(images);
 
 	if (iflag)
 		enable_interrupts();
diff --git a/boot/image-fit.c b/boot/image-fit.c
index 0367331c25d..d104367f91b 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -1163,10 +1163,10 @@ int fit_image_get_data(const void *fit, int noffset, const void **data,
 		}
 		*data = fit + offset;
 #if !defined(USE_HOSTCC) && CONFIG_IS_ENABLED(IMAGEMAP)
-		if (images.imagemap) {
+		if (bootm_imagemap(&images)) {
 			void *mapped;
 
-			mapped = imagemap_lookup(images.imagemap,
+			mapped = imagemap_lookup(bootm_imagemap(&images),
 						 offset, len);
 			if (mapped)
 				*data = mapped;
@@ -2212,6 +2212,7 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit,
 	u8 img_comp = IH_COMP_NONE;
 	void *mapped;
 	void *dst;
+	struct udevice *imagemap = bootm_imagemap(images);
 	int ret;
 
 	if (CONFIG_IS_ENABLED(FIT_VERITY)) {
@@ -2242,11 +2243,11 @@ static int fit_image_load_storage(struct bootm_headers *images, const void *fit,
 	    !fit_image_get_load(fit, noffset, &img_load)) {
 		dst = map_sysmem(img_load, data_sz);
 
-		mapped = imagemap_map_to(images->imagemap, data_off, data_sz,
+		mapped = imagemap_map_to(imagemap, data_off, data_sz,
 					 dst);
 		unmap_sysmem(dst);
 	} else {
-		mapped = imagemap_map(images->imagemap, data_off, data_sz);
+		mapped = imagemap_map(imagemap, data_off, data_sz);
 	}
 
 	return IS_ERR(mapped) ? PTR_ERR(mapped) : 0;
@@ -2361,7 +2362,7 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 	 * On-demand storage: pre-load external-data payloads into RAM (or leave
 	 * dm-verity filesystems on storage) before fit_image_select() verifies.
 	 */
-	if (CONFIG_IS_ENABLED(IMAGEMAP) && !tools_build() && images->imagemap) {
+	if (!tools_build() && bootm_imagemap(images)) {
 		bool early = false;
 
 		ret = fit_image_load_storage(images, fit, noffset, load_op,
@@ -2486,7 +2487,7 @@ int fit_image_load(struct bootm_headers *images, ulong addr,
 		}
 
 		/* A storage-backed image was read straight to its load address */
-		if (!images->imagemap || data != load)
+		if (!bootm_imagemap(images) || data != load)
 			printf("   Loading %s from 0x%08lx to 0x%08lx\n",
 			       prop_name, data, load);
 	} else {
diff --git a/include/bootm.h b/include/bootm.h
index d1aac3d44df..272583c8471 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -52,12 +52,43 @@ struct bootm_info {
 	const char *conf_fdt;
 	bool boot_progress;
 	struct bootm_headers *images;
+#if CONFIG_IS_ENABLED(IMAGEMAP)
 	struct udevice *imagemap;
+#endif
 	const char *cmd_name;
 	int argc;
 	char *const *argv;
 };
 
+/**
+ * bootm_info_imagemap() - Get the storage loader a boot was asked to use
+ *
+ * @bmi: Boot info
+ * Return: the imagemap device, or NULL to boot an image already in RAM
+ */
+static inline struct udevice *bootm_info_imagemap(struct bootm_info *bmi)
+{
+#if !defined(USE_HOSTCC) && CONFIG_IS_ENABLED(IMAGEMAP)
+	return bmi->imagemap;
+#else
+	return NULL;
+#endif
+}
+
+/**
+ * bootm_info_set_imagemap() - Record the storage loader a boot should use
+ *
+ * @bmi: Boot info
+ * @imagemap: The imagemap device, or NULL for an image already in RAM
+ */
+static inline void bootm_info_set_imagemap(struct bootm_info *bmi,
+					   struct udevice *imagemap)
+{
+#if !defined(USE_HOSTCC) && CONFIG_IS_ENABLED(IMAGEMAP)
+	bmi->imagemap = imagemap;
+#endif
+}
+
 /**
  * bootm_init() - Set up a bootm_info struct with useful defaults
  *
diff --git a/include/image.h b/include/image.h
index 5248ca98d41..14189581646 100644
--- a/include/image.h
+++ b/include/image.h
@@ -412,7 +412,9 @@ struct bootm_headers {
 
 	int		verify;		/* env_get("verify")[0] != 'n' */
 
+#if !defined(USE_HOSTCC) && CONFIG_IS_ENABLED(IMAGEMAP)
 	struct udevice *imagemap;	/* on-demand storage loader, or NULL */
+#endif
 
 #define BOOTM_STATE_START	0x00000001
 #define BOOTM_STATE_FINDOS	0x00000002
@@ -432,6 +434,35 @@ struct bootm_headers {
 
 extern struct bootm_headers images;
 
+/**
+ * bootm_imagemap() - Get the on-demand storage loader in use, if any
+ *
+ * @images: Images being booted
+ * Return: the imagemap device, or NULL when the image is already in RAM
+ */
+static inline struct udevice *bootm_imagemap(struct bootm_headers *images)
+{
+#if !defined(USE_HOSTCC) && CONFIG_IS_ENABLED(IMAGEMAP)
+	return images->imagemap;
+#else
+	return NULL;
+#endif
+}
+
+/**
+ * bootm_set_imagemap() - Record the on-demand storage loader to read through
+ *
+ * @images: Images being booted
+ * @imagemap: The imagemap device, or NULL for an image already in RAM
+ */
+static inline void bootm_set_imagemap(struct bootm_headers *images,
+				      struct udevice *imagemap)
+{
+#if !defined(USE_HOSTCC) && CONFIG_IS_ENABLED(IMAGEMAP)
+	images->imagemap = imagemap;
+#endif
+}
+
 /*
  * Some systems (for example LWMON) have very short watchdog periods;
  * we must make sure to split long operations like memmove() or
-- 
2.56.0

[PATCH v2 11/11] test: boot: imagemap: caller-owned region not resized

[Daniel Golle]

Add a unit test that locks down the fix in imagemap_map(): a larger
request against a region recorded by imagemap_map_to() (which the
caller owns and is not LMB-reserved) falls through to a fresh LMB
allocation instead of resizing the caller's memory.

The test reserves a caller-owned buffer inside the LMB-managed RAM,
records a 64-byte region there via imagemap_map_to() with a guard
pattern beyond the payload, then issues a 256-byte imagemap_map() at the
same base. It verifies the returned pointer differs from the caller's
buffer, the guard tail is unchanged, and the new region is LMB-reserved.

Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
v2:
 - Drop the reviewer's name from the scenario comment; the
   preceding paragraphs already explain it.
 - The bare 64 and 256 spellings become the record_size and
   request_size constants alongside dst_size, so they cannot drift.
 - Drop the pointless char * casts from the pointer comparison.

 test/boot/imagemap.c | 95 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 95 insertions(+)

diff --git a/test/boot/imagemap.c b/test/boot/imagemap.c
index c7289db3995..9024d42ea50 100644
--- a/test/boot/imagemap.c
+++ b/test/boot/imagemap.c
@@ -316,6 +316,101 @@ static int imagemap_test_map_to(struct unit_test_state *uts)
 
 IMAGEMAP_TEST(imagemap_test_map_to, UTF_DM);
 
+/*
+ * Test: map() must NOT resize a region the caller recorded via map_to().
+ *
+ * The caller reserves (owns) its own load address through the LMB, then
+ * asks imagemap_map_to() to load 64 bytes there and records it as a
+ * caller-owned region (lmb_reserved = false).  A later, larger request at
+ * the same base must fall through to a fresh LMB allocation, leaving the
+ * caller's memory untouched.
+ *
+ * Without the guard in imagemap_map() this path would lmb_free() the
+ * caller's reservation, re-reserve the same address and re-read in place,
+ * clobbering whatever the caller kept beyond the 64 requested bytes.
+ *
+ * dst must live inside the LMB-managed RAM (here: a caller-reserved
+ * buffer) so the buggy in-place re-read actually happens; a plain stack
+ * buffer sits outside that region and would mask the bug.
+ */
+static int imagemap_test_map_to_not_resized(struct unit_test_state *uts)
+{
+	struct udevice *dev;
+	struct imagemap_priv *priv;
+	struct mock_priv *mock;
+	u8 image[IMAGE_SIZE];
+	phys_addr_t dst_phys;
+	u8 *dst;
+	const ulong dst_size = 512;	/* caller-owned buffer */
+	const ulong record_size = 64;	/* region recorded via imagemap_map_to() */
+	const ulong request_size = 256;	/* larger request that must fall through */
+	u8 guard[dst_size - record_size];
+	struct imagemap_region *r;
+	void *p;
+
+	for (int i = 0; i < IMAGE_SIZE; i++)
+		image[i] = (u8)(i & 0xff);
+
+	ut_assertok(create_mock_loader(image, IMAGE_SIZE, &dev));
+	mock = get_mock_priv(dev);
+	priv = dev_get_uclass_priv(dev);
+
+	/* Caller reserves its own load address and owns the memory */
+	ut_assertok(lmb_alloc_mem(LMB_MEM_ALLOC_ANY, ARCH_DMA_MINALIGN,
+				  &dst_phys, dst_size, LMB_NONE));
+	dst = map_sysmem(dst_phys, dst_size);
+
+	/* Distinct guard pattern beyond the 64 requested payload bytes */
+	memset(guard, 0xa5, sizeof(guard));
+	memcpy(dst + record_size, guard, sizeof(guard));
+
+	/*
+	 * Caller records a 64-byte region at its chosen address; imagemap
+	 * must NOT claim an LMB reservation for it.
+	 */
+	p = imagemap_map_to(dev, 0, record_size, dst);
+	ut_assert(!IS_ERR(p));
+	ut_asserteq_ptr(dst, p);
+	ut_asserteq_mem(image, dst, record_size);
+	r = alist_get(&priv->regions, 0, struct imagemap_region);
+	ut_assert(r && !r->lmb_reserved);
+	ut_asserteq(1, priv->regions.count);
+
+	/*
+	 * Larger request at the same base: must fall through to a fresh
+	 * LMB allocation rather than resizing the caller's region.
+	 */
+	p = imagemap_map(dev, 0, request_size);
+	ut_assert(!IS_ERR(p));
+	/* Returned pointer must be a different buffer */
+	ut_assert(p != dst);
+	/* Caller's guard tail must remain intact */
+	ut_asserteq_mem(guard, dst + record_size, sizeof(guard));
+	/* The new region is LMB-reserved and holds the full 256 bytes */
+	ut_assert(lmb_is_reserved_flags(map_to_sysmem(p), LMB_NONE));
+	ut_asserteq_mem(image, p, request_size);
+
+	/*
+	 * The fall-through replaced the caller's record via
+	 * imagemap_record() (same img_offset); the table keeps one entry,
+	 * now LMB-reserved (the fresh allocation).
+	 */
+	ut_asserteq(1, priv->regions.count);
+	ut_assert(alist_get(&priv->regions, 0, struct imagemap_region)->lmb_reserved);
+
+	imagemap_cleanup(dev);
+
+	/*
+	 * The caller's own reservation was never taken over by imagemap and
+	 * is still ours to release.
+	 */
+	lmb_free(dst_phys, dst_size, LMB_NONE);
+
+	return 0;
+}
+
+IMAGEMAP_TEST(imagemap_test_map_to_not_resized, UTF_DM);
+
 /* Test: lookup() returns NULL for unmapped ranges */
 static int imagemap_test_lookup_miss(struct unit_test_state *uts)
 {
-- 
2.56.0

Re: [PATCH v2 03/11] boot: imagemap: correct the lookup guard rationale

[Simon Glass]

On 2026-10-21T14:08:59, Daniel Golle <daniel@makrotopia.org> wrote:
> boot: imagemap: correct the lookup guard rationale
>
> The comment on imagemap_lookup() attributes the need for its second
> guard to ulong and loff_t sharing a rank on LP64. The rank wording is
> wrong: loff_t is a signed long long, which outranks ulong on ILP32, so
> the subtraction in (3) stays signed there and an underflowing offset
> simply fails the size comparison. It is on LP64, where the two have
> the same width but different signedness, that the usual arithmetic
> conversions turn the subtraction unsigned and make it wrap, letting
> every range appear to fit.
>
> State the actual reason.
>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
>  boot/imagemap.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 09/11] boot: fit: share the external-data offset calculation

[Simon Glass]

On 2026-10-21T14:08:59, Daniel Golle <daniel@makrotopia.org> wrote:
> boot: fit: share the external-data offset calculation
>
> fit_image_load_storage() repeats the data-position / data-offset /
> totalsize-alignment sequence that fit_image_get_data() performs, and
> writes the result through an int pointer where the getters expect a u32.
> The copy also omits the range check on the sum, so an offset that the
> embedded path rejects is accepted here.
>
> Extract fit_image_get_ext_data_offset() and call it from both. The
> storage path gains the range check and both agree on one expression for
> the alignment. The signed comparison of the offset and size is
> meaningless once both are u32, so it is dropped in favour of the shared
> range check the helper provides.
>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
>  boot/image-fit.c | 166 ++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 91 insertions(+), 75 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 10/11] boot: build the imagemap pointers only when enabled

[Simon Glass]

On 2026-10-21T14:08:59, Daniel Golle <daniel@makrotopia.org> wrote:
> boot: build the imagemap pointers only when enabled
>
> struct bootm_headers and struct bootm_info carry an imagemap pointer
> whatever the configuration, so every board pays for it with the feature
> off, and each use site has to spell out an IS_ENABLED() test beside the
> NULL check.
>
> Compile the two fields under CONFIG_IMAGEMAP and reach them through
> bootm_imagemap(), bootm_set_imagemap(), bootm_info_imagemap() and
> bootm_info_set_imagemap(), which fold to NULL and to nothing when it
> is off.
>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
>  boot/bootm.c     | 44 ++++++++++++++++++++++++++++----------------
>  boot/image-fit.c | 13 +++++++------
>  include/bootm.h  | 31 +++++++++++++++++++++++++++++++
>  include/image.h  | 31 +++++++++++++++++++++++++++++++
>  4 files changed, 97 insertions(+), 22 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 11/11] test: boot: imagemap: caller-owned region not resized

[Simon Glass]

On 2026-10-21T14:08:59, Daniel Golle <daniel@makrotopia.org> wrote:
> test: boot: imagemap: caller-owned region not resized
>
> Add a unit test that locks down the fix in imagemap_map(): a larger
> request against a region recorded by imagemap_map_to() (which the
> caller owns and is not LMB-reserved) falls through to a fresh LMB
> allocation instead of resizing the caller's memory.
>
> The test reserves a caller-owned buffer inside the LMB-managed RAM,
> records a 64-byte region there via imagemap_map_to() with a guard
> pattern beyond the payload, then issues a 256-byte imagemap_map() at the
> same base. It verifies the returned pointer differs from the caller's
> buffer, the guard tail is unchanged, and the new region is LMB-reserved.
>
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>
>  test/boot/imagemap.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 95 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>