| Message ID | 20250505100852.9364-1-aristo.chen@canonical.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/1] tools: mkimage: validate image references in FIT configurations | expand |
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.chen@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
Hi, On Tue, 13 May 2025 at 17:28, Quentin Schulz <quentin.schulz@cherry.de> wrote: > > 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. > > One option (for boards which use Binman) would be to put these sorts of checks in there, since it might be easier to code? But having it in mkimage as well seems reasonable to me. Regards, Simon
Hi Simon, On 5/14/25 9:44 PM, Simon Glass wrote: > Hi, > > On Tue, 13 May 2025 at 17:28, Quentin Schulz <quentin.schulz@cherry.de> wrote: >> >> 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. >>> > > One option (for boards which use Binman) would be to put these sorts > of checks in there, since it might be easier to code? But having it in > mkimage as well seems reasonable to me. > mkimage can be used to generate images from outside of U-Boot. I believe this is what Yocto does for example to generate kernel FIT (see UBOOT_MKIMAGE and others), so having an additional check in mkimage is probably a good idea. Cheers, Quentin > Regards, > Simon
Hei hei, Am Thu, May 15, 2025 at 09:35:34AM +0200 schrieb Quentin Schulz: > Hi Simon, > > On 5/14/25 9:44 PM, Simon Glass wrote: > > Hi, > > > > On Tue, 13 May 2025 at 17:28, Quentin Schulz <quentin.schulz@cherry.de> wrote: > > > > > > 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. > > > > > > > > One option (for boards which use Binman) would be to put these sorts > > of checks in there, since it might be easier to code? But having it in > > mkimage as well seems reasonable to me. > > > > mkimage can be used to generate images from outside of U-Boot. I believe > this is what Yocto does for example to generate kernel FIT (see > UBOOT_MKIMAGE and others), so having an additional check in mkimage is > probably a good idea. Not sure about Yocto, but ptxdist [1] does exactly this: using mkimage to build signed FIT images, even if U-Boot is not used as bootloader. (ptxdist also uses mkimage when optionally packing U-Boot scripts. Those need to be put into an image so U-Boot can execute it.) Greets Alex [1] https://www.ptxdist.org/
Hi all, Alexander Dahl <ada@thorsis.com> 於 2025年5月15日 週四 下午12:44寫道： > > Hei hei, > > Am Thu, May 15, 2025 at 09:35:34AM +0200 schrieb Quentin Schulz: > > Hi Simon, > > > > On 5/14/25 9:44 PM, Simon Glass wrote: > > > Hi, > > > > > > On Tue, 13 May 2025 at 17:28, Quentin Schulz <quentin.schulz@cherry.de> wrote: > > > > > > > > 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. > > > > > > > > > > > One option (for boards which use Binman) would be to put these sorts > > > of checks in there, since it might be easier to code? But having it in > > > mkimage as well seems reasonable to me. > > > > > > > mkimage can be used to generate images from outside of U-Boot. I believe > > this is what Yocto does for example to generate kernel FIT (see > > UBOOT_MKIMAGE and others), so having an additional check in mkimage is > > probably a good idea. > > Not sure about Yocto, but ptxdist [1] does exactly this: using mkimage > to build signed FIT images, even if U-Boot is not used as bootloader. > > (ptxdist also uses mkimage when optionally packing U-Boot scripts. > Those need to be put into an image so U-Boot can execute it.) > > Greets > Alex > > [1] https://www.ptxdist.org/ Thank you very much for the reviews and all the helpful suggestions! Previously, I verified my patch using `./test/py/test.py -ra --bd sandbox --build`, and AFAICT everything looked fine. However, I just ran `make tests` and noticed that my patch causes some binman related test failures. I'll take some time to investigate the root cause before sending out the v2 patch. Best regards, Aristo
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)) { + static const char * const props[] = { FIT_KERNEL_PROP, + FIT_RAMDISK_PROP, + FIT_FDT_PROP, + FIT_LOADABLE_PROP }; + + 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 + 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); + + 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); + 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; } /** 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); + } usage("Bad parameters for FIT image type"); + } } if (params.lflag || params.fflag) {
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. This ensures that configuration integrity is validated at build time. Signed-off-by: Aristo Chen <aristo.chen@canonical.com> --- tools/fit_image.c | 48 +++++++++++++++++++++++++++++++++++++++++++++-- tools/mkimage.c | 7 ++++++- 2 files changed, 52 insertions(+), 3 deletions(-)