| Message ID | 20200717072825.371105-3-patrick.oppenlander@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [1/3] mkimage: fit: only process one cipher node | expand |
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
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
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");