Hi Aristo,
On 5/5/25 12:08 PM, Aristo Chen wrote:
When parsing a FIT image source (ITS), mkimage does not currently check
whether the image names referenced in the /configurations section (e.g.
"kernel", "fdt", "ramdisk", "loadables") actually exist in the /images
node.
This patch introduces a validation step during FIT import that iterates
over each configuration and verifies that all referenced image names are
defined under /images. If a missing image is detected, an appropriate
error is reported and mkimage exits with FDT_ERR_NOTFOUND.
Wondering if
fdt = &fdt_1, &fdt_2;
for example would have saved us from this issue since this would be
enforced by dtc requiring a label to exist. Ship has sailed already
though :)
This ensures that configuration integrity is validated at build time.
Signed-off-by: Aristo Chen <aristo.c...@canonical.com>
---
tools/fit_image.c | 48 +++++++++++++++++++++++++++++++++++++++++++++--
tools/mkimage.c | 7 ++++++-
2 files changed, 52 insertions(+), 3 deletions(-)
diff --git a/tools/fit_image.c b/tools/fit_image.c
index caed8d5f901..02b9c9b5855 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -627,6 +627,7 @@ static int fit_import_data(struct image_tool_params
*params, const char *fname)
struct stat sbuf;
int ret;
int images;
+ int confs;
int node;
fd = mmap_fdt(params->cmdname, fname, 0, &old_fdt, &sbuf, false, false);
@@ -695,6 +696,49 @@ static int fit_import_data(struct image_tool_params
*params, const char *fname)
}
}
+ confs = fdt_path_offset(fdt, FIT_CONFS_PATH);
+
+ for (node = fdt_first_subnode(fdt, confs);
+ node >= 0;
+ node = fdt_next_subnode(fdt, node)) {
This should be fdt_for_each_subnode instead?
+ static const char * const props[] = { FIT_KERNEL_PROP,
+ FIT_RAMDISK_PROP,
+ FIT_FDT_PROP,
+ FIT_LOADABLE_PROP };
+
Why only those?
https://fitspec.osfw.foundation/#optional-properties specifies "fpga",
"script" as unit names. https://fitspec.osfw.foundation/#id6 specifies
"firmware" as a unit name. So I'm assuming we should be checking those
as well?
Reading the code, it's not entirely clear (to me) whether FIT_SETUP_PROP
is about a unit name.
+ for (int i = 0; i < ARRAY_SIZE(props); i++) {
+ const char *list;
+ int len = 0;
+ int offset = 0;
+
+ list = fdt_getprop(fdt, node, props[i], &len);
+ if (!list)
+ continue;
+
+ // Some properties (like loadables) are stringlists
Aren't they all stringlists?
Simply get the stringlist length with fdt_stringlist_count() and then
iterate over all fdt_stringlist_get() instead of handcrafting some iterator?
+ while (offset < len) {
+ const char *img_name = list + offset;
+ char img_path[256];
+
+ offset += strlen(img_name) + 1;
+ snprintf(img_path, sizeof(img_path), "%s/%s",
FIT_IMAGES_PATH,
+ img_name);
+
+ int img = fdt_path_offset(fdt, img_path);
I believe you could save the snprintf by doing the following
- get the offset to the /images node with fdt_path_offset(fdt,
FIT_IMAGES_PATH)
- use that offset with fdt_subnode_offset(fdt, images_offset, img_name)
You only need to access the /images node once, outside of all loops.
+
+ if (img < 0) {
+ const char *conf_name =
fdt_get_name(fdt, node, NULL);
+
+ fprintf(stderr,
+ "Error: configuration '%s'
references undefined image '%s'\n",
+ conf_name, img_name);
Would be nice to tell in which property as well since we already have
that info.
+ ret = FDT_ERR_NOTFOUND;
+ goto err_munmap;
+ }
+ }
+ }
+ }
+
munmap(old_fdt, sbuf.st_size);
/* Close the old fd so we can re-use it. */
@@ -750,7 +794,7 @@ static int fit_handle_file(struct image_tool_params *params)
char bakfile[MKIMAGE_MAX_TMPFILE_LEN + 4] = {0};
char cmd[MKIMAGE_MAX_DTC_CMDLINE_LEN];
size_t size_inc;
- int ret;
+ int ret = EXIT_FAILURE;
/* Flattened Image Tree (FIT) format handling */
debug ("FIT format handling\n");
@@ -854,7 +898,7 @@ static int fit_handle_file(struct image_tool_params *params)
err_system:
unlink(tmpfile);
unlink(bakfile);
- return -1;
+ return ret;
This propagates more than simply the FDT_ERR_NOTFOUND added in this
patch. Not sure this is entirely safe to do? Please check that all
possible return codes of all functions whose return code is stored in
ret are negative.
Maybe we should have this as a separate commit too, seems like we could
benefit from it regardless of the first part of the patch being merged.
}
/**
diff --git a/tools/mkimage.c b/tools/mkimage.c
index 2954626a283..361711c53b2 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -519,8 +519,13 @@ int main(int argc, char **argv)
*/
retval = tparams->fflag_handle(¶ms);
- if (retval != EXIT_SUCCESS)
+ if (retval != EXIT_SUCCESS) {
+ if (retval == FDT_ERR_NOTFOUND) {
+ // Already printed error, exit cleanly
+ exit(EXIT_FAILURE);
Considering we only have one U_BOOT_IMAGE_TYPE with a fflag_handle, I
guess this is "fine", but this seems very enthusiastic :)
Cheers,
Quentin
