From c520266f9a796f5f447f5e3b1da04a874500290a Mon Sep 17 00:00:00 2001 From: Patrick Oppenlander Date: Thu, 30 Jul 2020 14:22:13 +1000 Subject: [PATCH 1/5] mkimage: fit: only process one cipher node 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 Reviewed-by: Philippe Reynes --- tools/image-host.c | 57 ++++++++++++++++++---------------------------- 1 file changed, 22 insertions(+), 35 deletions(-) diff --git a/tools/image-host.c b/tools/image-host.c index 9a83b7f675..dd7ecc4b60 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,20 @@ 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, + FIT_CIPHER_NODENAME); + 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); } /** From 04aeebb131081698204ad38bd8aba7140cd3ba22 Mon Sep 17 00:00:00 2001 From: Patrick Oppenlander Date: Thu, 30 Jul 2020 14:22:14 +1000 Subject: [PATCH 2/5] mkimage: fit: handle FDT_ERR_NOSPACE when ciphering Also replace fdt_delprop/fdt_setprop with fdt_setprop as fdt_setprop can replace an existing property value. Signed-off-by: Patrick Oppenlander Reviewed-by: Philippe Reynes --- 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 dd7ecc4b60..b4603c5f01 100644 --- a/tools/image-host.c +++ b/tools/image-host.c @@ -399,23 +399,24 @@ 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); - if (ret) { - printf("Can't remove data (err = %d)\n", ret); - goto out; - } - - /* Add ciphered data */ + /* Replace data with ciphered data */ ret = fdt_setprop(fit, image_noffset, FIT_DATA_PROP, data_ciphered, data_ciphered_len); + if (ret == -FDT_ERR_NOSPACE) { + ret = -ENOSPC; + goto out; + } if (ret) { - printf("Can't add ciphered data (err = %d)\n", ret); + printf("Can't replace data with ciphered data (err = %d)\n", ret); goto out; } /* 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 add unciphered data size (err = %d)\n", ret); goto out; From b33e5cc18263d438d11bb9a728b4117cc560cae4 Mon Sep 17 00:00:00 2001 From: Patrick Oppenlander Date: Thu, 30 Jul 2020 14:22:15 +1000 Subject: [PATCH 3/5] mkimage: fit: don't cipher ciphered data Previously, mkimage -F could be run multiple times causing already ciphered image data to be ciphered again. Signed-off-by: Patrick Oppenlander Reviewed-by: Philippe Reynes --- tools/image-host.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tools/image-host.c b/tools/image-host.c index b4603c5f01..e5417beee5 100644 --- a/tools/image-host.c +++ b/tools/image-host.c @@ -482,7 +482,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 +497,19 @@ int fit_image_cipher_data(const char *keydir, void *keydest, return -1; } + /* + * Don't cipher ciphered data. + * + * If the data-size-unciphered property is present the data for this + * image is already encrypted. This is important as 'mkimage -F' can be + * run multiple times on a FIT image. + */ + 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, From ef40129c33396d90a42e10f4a772390ac5b2ba05 Mon Sep 17 00:00:00 2001 From: Patrick Oppenlander Date: Thu, 30 Jul 2020 14:30:47 +1000 Subject: [PATCH 4/5] mkimage: fit: include image cipher in configuration signature This patch addresses issue #2 for signed configurations. -----8<----- Including the image cipher properties in the configuration signature prevents an attacker from modifying cipher, key or iv properties. Signed-off-by: Patrick Oppenlander Reviewed-by: Philippe Reynes --- tools/image-host.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tools/image-host.c b/tools/image-host.c index e5417beee5..3d52593e36 100644 --- a/tools/image-host.c +++ b/tools/image-host.c @@ -744,6 +744,23 @@ static int fit_config_get_hash_list(void *fit, int conf_noffset, return -ENOMSG; } + /* Add this image's cipher node if present */ + noffset = fdt_subnode_offset(fit, image_noffset, + FIT_CIPHER_NODENAME); + if (noffset != -FDT_ERR_NOTFOUND) { + if (noffset < 0) { + printf("Failed to get cipher node in configuration '%s/%s' image '%s': %s\n", + conf_name, sig_name, iname, + fdt_strerror(noffset)); + return -EIO; + } + ret = fdt_get_path(fit, noffset, path, sizeof(path)); + if (ret < 0) + goto err_path; + if (strlist_add(node_inc, path)) + goto err_mem; + } + image_count++; } From c995d854efcbf54ebd99a41f9a3c918340f16376 Mon Sep 17 00:00:00 2001 From: Patrick Oppenlander Date: Thu, 30 Jul 2020 14:31:48 +1000 Subject: [PATCH 5/5] mkimage: fit: fix import of external data The external data is located after the mmapped FDT pointed to by 'old_fdt', not in the newly created FDT we are importing into at 'fdt'. Signed-off-by: Patrick Oppenlander --- tools/fit_image.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/fit_image.c b/tools/fit_image.c index f7d2f56029..06faeda7c2 100644 --- a/tools/fit_image.c +++ b/tools/fit_image.c @@ -606,8 +606,8 @@ static int fit_import_data(struct image_tool_params *params, const char *fname) continue; debug("Importing data size %x\n", len); - ret = fdt_setprop(fdt, node, "data", fdt + data_base + buf_ptr, - len); + ret = fdt_setprop(fdt, node, "data", + old_fdt + data_base + buf_ptr, len); if (ret) { debug("%s: Failed to write property: %s\n", __func__, fdt_strerror(ret));