diff mbox series

[1/1] tools: mkimage: validate image references in FIT configurations

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

Commit Message

Aristo Chen May 5, 2025, 10:08 a.m. UTC
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(-)

Comments

Quentin Schulz May 13, 2025, 3:28 p.m. UTC | #1
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(&params);
>   
> -		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
Simon Glass May 14, 2025, 7:44 p.m. UTC | #2
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
Quentin Schulz May 15, 2025, 7:35 a.m. UTC | #3
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
Alexander Dahl May 15, 2025, 10:43 a.m. UTC | #4
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/
Aristo Chen May 15, 2025, 9:30 p.m. UTC | #5
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 mbox series

Patch

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(&params);
 
-		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) {