[PATCH 1/3] mkimage: fit: only process one cipher node

[PATCH 1/3] mkimage: fit: only process one cipher node

[patrick.oppenlander at gmail.com]

From: Patrick Oppenlander <patrick.oppenlander@gmail.com>

Previously mkimage would process any node matching the regex cipher.*
and apply the ciphers to the image data in the order they appeared in
the FDT. This meant that data could be inadvertently ciphered multiple
times.

Switch to processing a single cipher node which exactly matches
FIT_CIPHER_NODENAME.

Signed-off-by: Patrick Oppenlander <patrick.oppenlander@gmail.com>
---
 tools/image-host.c | 56 +++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 35 deletions(-)

diff --git a/tools/image-host.c b/tools/image-host.c
index 9a83b7f675..8fa1b9aba7 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -323,15 +323,15 @@ err:
 static int fit_image_setup_cipher(struct image_cipher_info *info,
 				  const char *keydir, void *fit,
 				  const char *image_name, int image_noffset,
-				  const char *node_name, int noffset)
+				  int noffset)
 {
 	char *algo_name;
 	char filename[128];
 	int ret = -1;
 
 	if (fit_image_cipher_get_algo(fit, noffset, &algo_name)) {
-		printf("Can't get algo name for cipher '%s' in image '%s'\n",
-		       node_name, image_name);
+		printf("Can't get algo name for cipher in image '%s'\n",
+		       image_name);
 		goto out;
 	}
 
@@ -340,16 +340,16 @@ static int fit_image_setup_cipher(struct image_cipher_info *info,
 	/* Read the key name */
 	info->keyname = fdt_getprop(fit, noffset, FIT_KEY_HINT, NULL);
 	if (!info->keyname) {
-		printf("Can't get key name for cipher '%s' in image '%s'\n",
-		       node_name, image_name);
+		printf("Can't get key name for cipher in image '%s'\n",
+		       image_name);
 		goto out;
 	}
 
 	/* Read the IV name */
 	info->ivname = fdt_getprop(fit, noffset, "iv-name-hint", NULL);
 	if (!info->ivname) {
-		printf("Can't get iv name for cipher '%s' in image '%s'\n",
-		       node_name, image_name);
+		printf("Can't get iv name for cipher in image '%s'\n",
+		       image_name);
 		goto out;
 	}
 
@@ -428,8 +428,7 @@ int fit_image_write_cipher(void *fit, int image_noffset, int noffset,
 static int
 fit_image_process_cipher(const char *keydir, void *keydest, void *fit,
 			 const char *image_name, int image_noffset,
-			 const char *node_name, int node_noffset,
-			 const void *data, size_t size,
+			 int node_noffset, const void *data, size_t size,
 			 const char *cmdname)
 {
 	struct image_cipher_info info;
@@ -440,7 +439,7 @@ fit_image_process_cipher(const char *keydir, void *keydest, void *fit,
 	memset(&info, 0, sizeof(info));
 
 	ret = fit_image_setup_cipher(&info, keydir, fit, image_name,
-				     image_noffset, node_name, node_noffset);
+				     image_noffset, node_noffset);
 	if (ret)
 		goto out;
 
@@ -482,7 +481,7 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
 	const char *image_name;
 	const void *data;
 	size_t size;
-	int node_noffset;
+	int cipher_node_offset;
 
 	/* Get image name */
 	image_name = fit_get_name(fit, image_noffset, NULL);
@@ -497,32 +496,19 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
 		return -1;
 	}
 
-	/* Process all hash subnodes of the component image node */
-	for (node_noffset = fdt_first_subnode(fit, image_noffset);
-	     node_noffset >= 0;
-	     node_noffset = fdt_next_subnode(fit, node_noffset)) {
-		const char *node_name;
-		int ret = 0;
-
-		node_name = fit_get_name(fit, node_noffset, NULL);
-		if (!node_name) {
-			printf("Can't get node name\n");
-			return -1;
-		}
 
-		if (IMAGE_ENABLE_ENCRYPT && keydir &&
-		    !strncmp(node_name, FIT_CIPHER_NODENAME,
-			     strlen(FIT_CIPHER_NODENAME)))
-			ret = fit_image_process_cipher(keydir, keydest,
-						       fit, image_name,
-						       image_noffset,
-						       node_name, node_noffset,
-						       data, size, cmdname);
-		if (ret)
-			return ret;
+	/* Process cipher node if present */
+	cipher_node_offset = fdt_subnode_offset(fit, image_noffset, "cipher");
+	if (cipher_node_offset == -FDT_ERR_NOTFOUND)
+		return 0;
+	if (cipher_node_offset < 0) {
+		printf("Failure getting cipher node\n");
+		return -1;
 	}
-
-	return 0;
+	if (!IMAGE_ENABLE_ENCRYPT || !keydir)
+		return 0;
+	return fit_image_process_cipher(keydir, keydest, fit, image_name,
+		image_noffset, cipher_node_offset, data, size, cmdname);
 }
 
 /**
-- 
2.27.0

[PATCH 2/3] mkimage: fit: handle FDT_ERR_NOSPACE when ciphering

[patrick.oppenlander at gmail.com]

From: Patrick Oppenlander <patrick.oppenlander@gmail.com>

This meant that the order of operations had to change. If we replace the
data property first then fail to add the data-size-unciphered property
the data will be ciphered again when retrying.

Signed-off-by: Patrick Oppenlander <patrick.oppenlander@gmail.com>
---
 tools/image-host.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/tools/image-host.c b/tools/image-host.c
index 8fa1b9aba7..87ef79ef53 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -399,25 +399,26 @@ int fit_image_write_cipher(void *fit, int image_noffset, int noffset,
 {
 	int ret = -1;
 
-	/* Remove unciphered data */
-	ret = fdt_delprop(fit, image_noffset, FIT_DATA_PROP);
+	/* add non ciphered data size */
+	ret = fdt_setprop_u32(fit, image_noffset, "data-size-unciphered", size);
+	if (ret == -FDT_ERR_NOSPACE) {
+		ret = -ENOSPC;
+		goto out;
+	}
 	if (ret) {
-		printf("Can't remove data (err = %d)\n", ret);
+		printf("Can't add unciphered data size (err = %d)\n", ret);
 		goto out;
 	}
 
 	/* Add ciphered data */
 	ret = fdt_setprop(fit, image_noffset, FIT_DATA_PROP,
 			  data_ciphered, data_ciphered_len);
-	if (ret) {
-		printf("Can't add ciphered data (err = %d)\n", ret);
+	if (ret == -FDT_ERR_NOSPACE) {
+		ret = -ENOSPC;
 		goto out;
 	}
-
-	/* add non ciphered data size */
-	ret = fdt_setprop_u32(fit, image_noffset, "data-size-unciphered", size);
 	if (ret) {
-		printf("Can't add unciphered data size (err = %d)\n", ret);
+		printf("Can't add ciphered data (err = %d)\n", ret);
 		goto out;
 	}
 
-- 
2.27.0

[PATCH 3/3] mkimage: fit: don't cipher ciphered data

[patrick.oppenlander at gmail.com]

From: Patrick Oppenlander <patrick.oppenlander@gmail.com>

Previously, mkimage -F could be run multiple times causing already
ciphered image data to be ciphered again.

Signed-off-by: Patrick Oppenlander <patrick.oppenlander@gmail.com>
---
 tools/image-host.c | 47 +++++++++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 15 deletions(-)

diff --git a/tools/image-host.c b/tools/image-host.c
index 87ef79ef53..12de9b5ec0 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -397,33 +397,43 @@ int fit_image_write_cipher(void *fit, int image_noffset, int noffset,
 			   const void *data, size_t size,
 			   unsigned char *data_ciphered, int data_ciphered_len)
 {
-	int ret = -1;
+	/*
+	 * fit_image_cipher_data() uses the presence of the data-size-unciphered
+	 * property as a sentinel to detect whether the data for this image is
+	 * already encrypted. This is important as:
+	 * - 'mkimage -F' can be run multiple times on a FIT image
+	 * - This function is in a retry loop to handle ENOSPC
+	 */
 
-	/* add non ciphered data size */
+	int ret;
+
+	/* Add unciphered data size */
 	ret = fdt_setprop_u32(fit, image_noffset, "data-size-unciphered", size);
-	if (ret == -FDT_ERR_NOSPACE) {
-		ret = -ENOSPC;
-		goto out;
-	}
+	if (ret == -FDT_ERR_NOSPACE)
+		return -ENOSPC;
 	if (ret) {
 		printf("Can't add unciphered data size (err = %d)\n", ret);
-		goto out;
+		return -EIO;
 	}
 
-	/* Add ciphered data */
+	/* Replace contents of data property with data_ciphered */
 	ret = fdt_setprop(fit, image_noffset, FIT_DATA_PROP,
 			  data_ciphered, data_ciphered_len);
 	if (ret == -FDT_ERR_NOSPACE) {
-		ret = -ENOSPC;
-		goto out;
+		/* Remove data-size-unciphered; data is not ciphered */
+		ret = fdt_delprop(fit, image_noffset, "data-size-unciphered");
+		if (ret) {
+			printf("Can't remove unciphered data size (err = %d)\n", ret);
+			return -EIO;
+		}
+		return -ENOSPC;
 	}
 	if (ret) {
-		printf("Can't add ciphered data (err = %d)\n", ret);
-		goto out;
+		printf("Can't replace data with ciphered data (err = %d)\n", ret);
+		return -EIO;
 	}
 
- out:
-	return ret;
+	return 0;
 }
 
 static int
@@ -482,7 +492,7 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
 	const char *image_name;
 	const void *data;
 	size_t size;
-	int cipher_node_offset;
+	int cipher_node_offset, len;
 
 	/* Get image name */
 	image_name = fit_get_name(fit, image_noffset, NULL);
@@ -497,6 +507,13 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
 		return -1;
 	}
 
+	/* Don't cipher ciphered data */
+	if (fdt_getprop(fit, image_noffset, "data-size-unciphered", &len))
+		return 0;
+	if (len != -FDT_ERR_NOTFOUND) {
+		printf("Failure testing for data-size-unciphered\n");
+		return -1;
+	}
 
 	/* Process cipher node if present */
 	cipher_node_offset = fdt_subnode_offset(fit, image_noffset, "cipher");
-- 
2.27.0

[PATCH 1/3] mkimage: fit: only process one cipher node

[Simon Glass]

Hi Patrick,

On Fri, 17 Jul 2020 at 05:30, <patrick.oppenlander@gmail.com> wrote:
>
> From: Patrick Oppenlander <patrick.oppenlander@gmail.com>
>
> Previously mkimage would process any node matching the regex cipher.*
> and apply the ciphers to the image data in the order they appeared in
> the FDT. This meant that data could be inadvertently ciphered multiple
> times.
>
> Switch to processing a single cipher node which exactly matches
> FIT_CIPHER_NODENAME.
>
> Signed-off-by: Patrick Oppenlander <patrick.oppenlander@gmail.com>
> ---
>  tools/image-host.c | 56 +++++++++++++++++-----------------------------
>  1 file changed, 21 insertions(+), 35 deletions(-)

+Philippe Reynes for a review on these three patches too.

>
> diff --git a/tools/image-host.c b/tools/image-host.c
> index 9a83b7f675..8fa1b9aba7 100644
> --- a/tools/image-host.c
> +++ b/tools/image-host.c
> @@ -323,15 +323,15 @@ err:
>  static int fit_image_setup_cipher(struct image_cipher_info *info,
>                                   const char *keydir, void *fit,
>                                   const char *image_name, int image_noffset,
> -                                 const char *node_name, int noffset)
> +                                 int noffset)
>  {
>         char *algo_name;
>         char filename[128];
>         int ret = -1;
>
>         if (fit_image_cipher_get_algo(fit, noffset, &algo_name)) {
> -               printf("Can't get algo name for cipher '%s' in image '%s'\n",
> -                      node_name, image_name);
> +               printf("Can't get algo name for cipher in image '%s'\n",
> +                      image_name);
>                 goto out;
>         }
>
> @@ -340,16 +340,16 @@ static int fit_image_setup_cipher(struct image_cipher_info *info,
>         /* Read the key name */
>         info->keyname = fdt_getprop(fit, noffset, FIT_KEY_HINT, NULL);
>         if (!info->keyname) {
> -               printf("Can't get key name for cipher '%s' in image '%s'\n",
> -                      node_name, image_name);
> +               printf("Can't get key name for cipher in image '%s'\n",
> +                      image_name);
>                 goto out;
>         }
>
>         /* Read the IV name */
>         info->ivname = fdt_getprop(fit, noffset, "iv-name-hint", NULL);
>         if (!info->ivname) {
> -               printf("Can't get iv name for cipher '%s' in image '%s'\n",
> -                      node_name, image_name);
> +               printf("Can't get iv name for cipher in image '%s'\n",
> +                      image_name);
>                 goto out;
>         }
>
> @@ -428,8 +428,7 @@ int fit_image_write_cipher(void *fit, int image_noffset, int noffset,
>  static int
>  fit_image_process_cipher(const char *keydir, void *keydest, void *fit,
>                          const char *image_name, int image_noffset,
> -                        const char *node_name, int node_noffset,
> -                        const void *data, size_t size,
> +                        int node_noffset, const void *data, size_t size,
>                          const char *cmdname)
>  {
>         struct image_cipher_info info;
> @@ -440,7 +439,7 @@ fit_image_process_cipher(const char *keydir, void *keydest, void *fit,
>         memset(&info, 0, sizeof(info));
>
>         ret = fit_image_setup_cipher(&info, keydir, fit, image_name,
> -                                    image_noffset, node_name, node_noffset);
> +                                    image_noffset, node_noffset);
>         if (ret)
>                 goto out;
>
> @@ -482,7 +481,7 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
>         const char *image_name;
>         const void *data;
>         size_t size;
> -       int node_noffset;
> +       int cipher_node_offset;
>
>         /* Get image name */
>         image_name = fit_get_name(fit, image_noffset, NULL);
> @@ -497,32 +496,19 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
>                 return -1;
>         }
>
> -       /* Process all hash subnodes of the component image node */
> -       for (node_noffset = fdt_first_subnode(fit, image_noffset);
> -            node_noffset >= 0;
> -            node_noffset = fdt_next_subnode(fit, node_noffset)) {
> -               const char *node_name;
> -               int ret = 0;
> -
> -               node_name = fit_get_name(fit, node_noffset, NULL);
> -               if (!node_name) {
> -                       printf("Can't get node name\n");
> -                       return -1;
> -               }
>
> -               if (IMAGE_ENABLE_ENCRYPT && keydir &&
> -                   !strncmp(node_name, FIT_CIPHER_NODENAME,
> -                            strlen(FIT_CIPHER_NODENAME)))
> -                       ret = fit_image_process_cipher(keydir, keydest,
> -                                                      fit, image_name,
> -                                                      image_noffset,
> -                                                      node_name, node_noffset,
> -                                                      data, size, cmdname);
> -               if (ret)
> -                       return ret;
> +       /* Process cipher node if present */
> +       cipher_node_offset = fdt_subnode_offset(fit, image_noffset, "cipher");
> +       if (cipher_node_offset == -FDT_ERR_NOTFOUND)
> +               return 0;
> +       if (cipher_node_offset < 0) {
> +               printf("Failure getting cipher node\n");
> +               return -1;
>         }
> -
> -       return 0;
> +       if (!IMAGE_ENABLE_ENCRYPT || !keydir)
> +               return 0;
> +       return fit_image_process_cipher(keydir, keydest, fit, image_name,
> +               image_noffset, cipher_node_offset, data, size, cmdname);
>  }
>
>  /**
> --
> 2.27.0
>

[PATCH 1/3] mkimage: fit: only process one cipher node

[Philippe REYNES]

Hi Patrick,

> From: Patrick Oppenlander <patrick.oppenlander@gmail.com>
> 
> Previously mkimage would process any node matching the regex cipher.*
> and apply the ciphers to the image data in the order they appeared in
> the FDT. This meant that data could be inadvertently ciphered multiple
> times.
> 
> Switch to processing a single cipher node which exactly matches
> FIT_CIPHER_NODENAME.
> 
> Signed-off-by: Patrick Oppenlander <patrick.oppenlander@gmail.com>
> ---
> tools/image-host.c | 56 +++++++++++++++++-----------------------------
> 1 file changed, 21 insertions(+), 35 deletions(-)
> 
> diff --git a/tools/image-host.c b/tools/image-host.c
> index 9a83b7f675..8fa1b9aba7 100644
> --- a/tools/image-host.c
> +++ b/tools/image-host.c
> @@ -323,15 +323,15 @@ err:
> static int fit_image_setup_cipher(struct image_cipher_info *info,
> const char *keydir, void *fit,
> const char *image_name, int image_noffset,
> - const char *node_name, int noffset)
> + int noffset)
> {
> char *algo_name;
> char filename[128];
> int ret = -1;
> 
> if (fit_image_cipher_get_algo(fit, noffset, &algo_name)) {
> - printf("Can't get algo name for cipher '%s' in image '%s'\n",
> - node_name, image_name);
> + printf("Can't get algo name for cipher in image '%s'\n",
> + image_name);
> goto out;
> }
> 
> @@ -340,16 +340,16 @@ static int fit_image_setup_cipher(struct image_cipher_info
> *info,
> /* Read the key name */
> info->keyname = fdt_getprop(fit, noffset, FIT_KEY_HINT, NULL);
> if (!info->keyname) {
> - printf("Can't get key name for cipher '%s' in image '%s'\n",
> - node_name, image_name);
> + printf("Can't get key name for cipher in image '%s'\n",
> + image_name);
> goto out;
> }
> 
> /* Read the IV name */
> info->ivname = fdt_getprop(fit, noffset, "iv-name-hint", NULL);
> if (!info->ivname) {
> - printf("Can't get iv name for cipher '%s' in image '%s'\n",
> - node_name, image_name);
> + printf("Can't get iv name for cipher in image '%s'\n",
> + image_name);
> goto out;
> }
> 
> @@ -428,8 +428,7 @@ int fit_image_write_cipher(void *fit, int image_noffset, int
> noffset,
> static int
> fit_image_process_cipher(const char *keydir, void *keydest, void *fit,
> const char *image_name, int image_noffset,
> - const char *node_name, int node_noffset,
> - const void *data, size_t size,
> + int node_noffset, const void *data, size_t size,
> const char *cmdname)
> {
> struct image_cipher_info info;
> @@ -440,7 +439,7 @@ fit_image_process_cipher(const char *keydir, void *keydest,
> void *fit,
> memset(&info, 0, sizeof(info));
> 
> ret = fit_image_setup_cipher(&info, keydir, fit, image_name,
> - image_noffset, node_name, node_noffset);
> + image_noffset, node_noffset);
> if (ret)
> goto out;
> 
> @@ -482,7 +481,7 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
> const char *image_name;
> const void *data;
> size_t size;
> - int node_noffset;
> + int cipher_node_offset;
> 
> /* Get image name */
> image_name = fit_get_name(fit, image_noffset, NULL);
> @@ -497,32 +496,19 @@ int fit_image_cipher_data(const char *keydir, void
> *keydest,
> return -1;
> }
> 
> - /* Process all hash subnodes of the component image node */
> - for (node_noffset = fdt_first_subnode(fit, image_noffset);
> - node_noffset >= 0;
> - node_noffset = fdt_next_subnode(fit, node_noffset)) {
> - const char *node_name;
> - int ret = 0;
> -
> - node_name = fit_get_name(fit, node_noffset, NULL);
> - if (!node_name) {
> - printf("Can't get node name\n");
> - return -1;
> - }
> 
> - if (IMAGE_ENABLE_ENCRYPT && keydir &&
> - !strncmp(node_name, FIT_CIPHER_NODENAME,
> - strlen(FIT_CIPHER_NODENAME)))
> - ret = fit_image_process_cipher(keydir, keydest,
> - fit, image_name,
> - image_noffset,
> - node_name, node_noffset,
> - data, size, cmdname);
> - if (ret)
> - return ret;
> + /* Process cipher node if present */
> + cipher_node_offset = fdt_subnode_offset(fit, image_noffset, "cipher");

use FIT_CIPHER_NODENAME instead of hardcoded "cipher"

> + if (cipher_node_offset == -FDT_ERR_NOTFOUND)
> + return 0;
> + if (cipher_node_offset < 0) {
> + printf("Failure getting cipher node\n");
> + return -1;
> }
> -
> - return 0;
> + if (!IMAGE_ENABLE_ENCRYPT || !keydir)
> + return 0;
> + return fit_image_process_cipher(keydir, keydest, fit, image_name,
> + image_noffset, cipher_node_offset, data, size, cmdname);
> }
> 
> /**
> --
> 2.27.0


Other than this little remark:

Reviewed-by: Philippe Reynes <philippe.reynes@softathome.com>

Regards,
Philippe

[PATCH 2/3] mkimage: fit: handle FDT_ERR_NOSPACE when ciphering

[Philippe REYNES]

Hi Patrick

> From: Patrick Oppenlander <patrick.oppenlander@gmail.com>
> 
> This meant that the order of operations had to change. If we replace the
> data property first then fail to add the data-size-unciphered property
> the data will be ciphered again when retrying.


This patch is good, but I disagree with the comment. It is not mandatory
to change the order of operation because when signing/ciphering we always
start from "fresh" file.

This "trick" is done in the function fit_handle_file(...)

Just before the loop, the tmpfile is rename in bakfile

	sprintf(bakfile, "%s%s", tmpfile, ".bak");
	rename(tmpfile, bakfile);

And in the loop, the first operation is to copy bakfile to tmpfile:

	for (size_inc = 0; size_inc < 64 * 1024; size_inc += 1024) {
		if (copyfile(bakfile, tmpfile) < 0) {
			printf("Can't copy %s to %s\n", bakfile, tmpfile);
			ret = -EIO;
			break;
		}
		ret = fit_add_file_data(params, size_inc, tmpfile);
		if (!ret || ret != -ENOSPC)
			break;
	}

So I think that we always cipher with unciphered data.


> Signed-off-by: Patrick Oppenlander <patrick.oppenlander@gmail.com>
> ---
> tools/image-host.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/image-host.c b/tools/image-host.c
> index 8fa1b9aba7..87ef79ef53 100644
> --- a/tools/image-host.c
> +++ b/tools/image-host.c
> @@ -399,25 +399,26 @@ int fit_image_write_cipher(void *fit, int image_noffset,
> int noffset,
> {
> int ret = -1;
> 
> - /* Remove unciphered data */
> - ret = fdt_delprop(fit, image_noffset, FIT_DATA_PROP);
> + /* add non ciphered data size */
> + ret = fdt_setprop_u32(fit, image_noffset, "data-size-unciphered", size);
> + if (ret == -FDT_ERR_NOSPACE) {
> + ret = -ENOSPC;
> + goto out;
> + }
> if (ret) {
> - printf("Can't remove data (err = %d)\n", ret);
> + printf("Can't add unciphered data size (err = %d)\n", ret);
> goto out;
> }
> 
> /* Add ciphered data */
> ret = fdt_setprop(fit, image_noffset, FIT_DATA_PROP,
> data_ciphered, data_ciphered_len);
> - if (ret) {
> - printf("Can't add ciphered data (err = %d)\n", ret);
> + if (ret == -FDT_ERR_NOSPACE) {
> + ret = -ENOSPC;
> goto out;
> }
> -
> - /* add non ciphered data size */
> - ret = fdt_setprop_u32(fit, image_noffset, "data-size-unciphered", size);
> if (ret) {
> - printf("Can't add unciphered data size (err = %d)\n", ret);
> + printf("Can't add ciphered data (err = %d)\n", ret);
> goto out;
> }
> 
> --
> 2.27.0


Regards,
Philippe

[PATCH 3/3] mkimage: fit: don't cipher ciphered data

[Philippe REYNES]

Hi Patrick,


> From: Patrick Oppenlander <patrick.oppenlander@gmail.com>
> 
> Previously, mkimage -F could be run multiple times causing already
> ciphered image data to be ciphered again.


Good catch, mkimage -F cipher data that are already ciphered,
generating a broken FIT image.


> Signed-off-by: Patrick Oppenlander <patrick.oppenlander@gmail.com>
> ---
> tools/image-host.c | 47 +++++++++++++++++++++++++++++++---------------
> 1 file changed, 32 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/image-host.c b/tools/image-host.c
> index 87ef79ef53..12de9b5ec0 100644
> --- a/tools/image-host.c
> +++ b/tools/image-host.c
> @@ -397,33 +397,43 @@ int fit_image_write_cipher(void *fit, int image_noffset,
> int noffset,
> const void *data, size_t size,
> unsigned char *data_ciphered, int data_ciphered_len)
> {
> - int ret = -1;
> + /*
> + * fit_image_cipher_data() uses the presence of the data-size-unciphered
> + * property as a sentinel to detect whether the data for this image is
> + * already encrypted. This is important as:
> + * - 'mkimage -F' can be run multiple times on a FIT image
> + * - This function is in a retry loop to handle ENOSPC
> + */
> 
> - /* add non ciphered data size */
> + int ret;
> +
> + /* Add unciphered data size */
> ret = fdt_setprop_u32(fit, image_noffset, "data-size-unciphered", size);
> - if (ret == -FDT_ERR_NOSPACE) {
> - ret = -ENOSPC;
> - goto out;
> - }
> + if (ret == -FDT_ERR_NOSPACE)
> + return -ENOSPC;
> if (ret) {
> printf("Can't add unciphered data size (err = %d)\n", ret);
> - goto out;
> + return -EIO;
> }
> 
> - /* Add ciphered data */
> + /* Replace contents of data property with data_ciphered */
> ret = fdt_setprop(fit, image_noffset, FIT_DATA_PROP,
> data_ciphered, data_ciphered_len);
> if (ret == -FDT_ERR_NOSPACE) {
> - ret = -ENOSPC;
> - goto out;
> + /* Remove data-size-unciphered; data is not ciphered */
> + ret = fdt_delprop(fit, image_noffset, "data-size-unciphered");
> + if (ret) {
> + printf("Can't remove unciphered data size (err = %d)\n", ret);
> + return -EIO;
> + }
> + return -ENOSPC;
> }
> if (ret) {
> - printf("Can't add ciphered data (err = %d)\n", ret);
> - goto out;
> + printf("Can't replace data with ciphered data (err = %d)\n", ret);
> + return -EIO;
> }
> 
> - out:
> - return ret;
> + return 0;
> }

As for the second patch, I think that the loop is not an issue because
it always start with "fresh/clean" value (using a backup file).

So I am not sure that changes in this function are needed.


> static int
> @@ -482,7 +492,7 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
> const char *image_name;
> const void *data;
> size_t size;
> - int cipher_node_offset;
> + int cipher_node_offset, len;
> 
> /* Get image name */
> image_name = fit_get_name(fit, image_noffset, NULL);
> @@ -497,6 +507,13 @@ int fit_image_cipher_data(const char *keydir, void
> *keydest,
> return -1;
> }
> 
> + /* Don't cipher ciphered data */
> + if (fdt_getprop(fit, image_noffset, "data-size-unciphered", &len))
> + return 0;
> + if (len != -FDT_ERR_NOTFOUND) {
> + printf("Failure testing for data-size-unciphered\n");
> + return -1;
> + }

From my point of view, it fixes an issue. But I see this solution more
as "workaround" than a clean solution.

As it fixes a real issue, we may start with it and then try to found a
"clean" solution.


> /* Process cipher node if present */
> cipher_node_offset = fdt_subnode_offset(fit, image_noffset, "cipher");
> --
> 2.27.0


Regards,
Philippe

[PATCH 2/3] mkimage: fit: handle FDT_ERR_NOSPACE when ciphering

[Patrick Oppenlander]

On Thu, Jul 30, 2020 at 1:02 AM Philippe REYNES
<philippe.reynes@softathome.com> wrote:
>
> Hi Patrick
>
> > From: Patrick Oppenlander <patrick.oppenlander@gmail.com>
> >
> > This meant that the order of operations had to change. If we replace the
> > data property first then fail to add the data-size-unciphered property
> > the data will be ciphered again when retrying.
>
>
> This patch is good, but I disagree with the comment. It is not mandatory
> to change the order of operation because when signing/ciphering we always
> start from "fresh" file.
>
> This "trick" is done in the function fit_handle_file(...)
>
> Just before the loop, the tmpfile is rename in bakfile
>
>         sprintf(bakfile, "%s%s", tmpfile, ".bak");
>         rename(tmpfile, bakfile);
>
> And in the loop, the first operation is to copy bakfile to tmpfile:
>
>         for (size_inc = 0; size_inc < 64 * 1024; size_inc += 1024) {
>                 if (copyfile(bakfile, tmpfile) < 0) {
>                         printf("Can't copy %s to %s\n", bakfile, tmpfile);
>                         ret = -EIO;
>                         break;
>                 }
>                 ret = fit_add_file_data(params, size_inc, tmpfile);
>                 if (!ret || ret != -ENOSPC)
>                         break;
>         }
>
> So I think that we always cipher with unciphered data.
>

Hi Philip,

I don't think that is the case if you use "mkimage -F". The input can
already be ciphered.

Patrick

[PATCH 3/3] mkimage: fit: don't cipher ciphered data

[Patrick Oppenlander]

On Thu, Jul 30, 2020 at 3:17 AM Philippe REYNES
<philippe.reynes@softathome.com> wrote:
>
> As for the second patch, I think that the loop is not an issue because
> it always start with "fresh/clean" value (using a backup file).
>
> So I am not sure that changes in this function are needed.
>

OK, I overlooked this.

I will resubmit a simplified patch series.

>
> > static int
> > @@ -482,7 +492,7 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
> > const char *image_name;
> > const void *data;
> > size_t size;
> > - int cipher_node_offset;
> > + int cipher_node_offset, len;
> >
> > /* Get image name */
> > image_name = fit_get_name(fit, image_noffset, NULL);
> > @@ -497,6 +507,13 @@ int fit_image_cipher_data(const char *keydir, void
> > *keydest,
> > return -1;
> > }
> >
> > + /* Don't cipher ciphered data */
> > + if (fdt_getprop(fit, image_noffset, "data-size-unciphered", &len))
> > + return 0;
> > + if (len != -FDT_ERR_NOTFOUND) {
> > + printf("Failure testing for data-size-unciphered\n");
> > + return -1;
> > + }
>
> From my point of view, it fixes an issue. But I see this solution more
> as "workaround" than a clean solution.
>
> As it fixes a real issue, we may start with it and then try to found a
> "clean" solution.
>

True, it's not ideal. But at least it fixes the bug :)

Thanks for the review,

Patrick