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

[Philippe REYNES <philippe.reynes@softathome.com>]

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