[resent,3/3] mkimage: Allow to specify the signature algorithm on the command line

From: Jan Kiszka <jan.kiszka@siemens.com>

This permits to prepare FIT image description that do not hard-code the
final choice of the signature algorithm, possibly requiring the user to
patch the sources.

When -o <algo> is specified, this information is used in favor of the
'algo' property in the signature node. Furthermore, that property is set
accordingly when writing the image.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 doc/mkimage.1      |  5 +++++
 include/image.h    |  3 ++-
 tools/fit_image.c  |  3 ++-
 tools/image-host.c | 48 +++++++++++++++++++++++++++-------------------
 tools/imagetool.h  |  1 +
 tools/mkimage.c    |  5 ++++-
 6 files changed, 42 insertions(+), 23 deletions(-)

Hi Jan,

On Fri, 14 Jan 2022 at 02:21, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> This permits to prepare FIT image description that do not hard-code the
> final choice of the signature algorithm, possibly requiring the user to
> patch the sources.
>
> When -o <algo> is specified, this information is used in favor of the
> 'algo' property in the signature node. Furthermore, that property is set
> accordingly when writing the image.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  doc/mkimage.1      |  5 +++++
>  include/image.h    |  3 ++-
>  tools/fit_image.c  |  3 ++-
>  tools/image-host.c | 48 +++++++++++++++++++++++++++-------------------
>  tools/imagetool.h  |  1 +
>  tools/mkimage.c    |  5 ++++-
>  6 files changed, 42 insertions(+), 23 deletions(-)

Please add a test to test_vboot for this case.

>
> diff --git a/doc/mkimage.1 b/doc/mkimage.1
> index fea5288784..0734bd36a1 100644
> --- a/doc/mkimage.1
> +++ b/doc/mkimage.1
> @@ -155,6 +155,11 @@ the corresponding public key is written into this file for for run-time
>  verification. Typically the file here is the device tree binary used by
>  CONFIG_OF_CONTROL in U-Boot.
>
> +.TP
> +.BI "\-o [" "signing algorithm" "]"
> +Specifies the algorithm to be used for signing a FIT image. The default is
> +taken from the target signature nodes 'algo' properties.

What does 'target' mean in this case?

Perhaps 'taken from the signature node's 'algo' properties' ?

> +
>  .TP
>  .BI "\-p [" "external position" "]"
>  Place external data at a static external position. See \-E. Instead of writing
> diff --git a/include/image.h b/include/image.h
> index 16ccc5b10f..4a7e9bc9a1 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1031,6 +1031,7 @@ int fit_cipher_data(const char *keydir, void *keydest, void *fit,
>   * @require_keys: Mark all keys as 'required'
>   * @engine_id: Engine to use for signing
>   * @cmdname:   Command name used when reporting errors
> + * @algo_name: Algorithm name, or NULL if to be read from FIT
>   *
>   * Adds hash values for all component images in the FIT blob.
>   * Hashes are calculated for all component images which have hash subnodes
> @@ -1045,7 +1046,7 @@ int fit_cipher_data(const char *keydir, void *keydest, void *fit,
>  int fit_add_verification_data(const char *keydir, const char *keyfile,
>                               void *keydest, void *fit, const char *comment,
>                               int require_keys, const char *engine_id,
> -                             const char *cmdname);
> +                             const char *cmdname, const char *algo_name);
>
>  int fit_image_verify_with_data(const void *fit, int image_noffset,
>                                const void *data, size_t size);
> diff --git a/tools/fit_image.c b/tools/fit_image.c
> index f4f372ba62..428ddcf881 100644
> --- a/tools/fit_image.c
> +++ b/tools/fit_image.c
> @@ -73,7 +73,8 @@ static int fit_add_file_data(struct image_tool_params *params, size_t size_inc,
>                                                 params->comment,
>                                                 params->require_keys,
>                                                 params->engine_id,
> -                                               params->cmdname);
> +                                               params->cmdname,
> +                                               params->algo_name);
>         }
>
>         if (dest_blob) {
> diff --git a/tools/image-host.c b/tools/image-host.c
> index a027155f3c..d2e67a06aa 100644
> --- a/tools/image-host.c
> +++ b/tools/image-host.c
> @@ -107,7 +107,7 @@ static int fit_image_process_hash(void *fit, const char *image_name,
>   */
>  static int fit_image_write_sig(void *fit, int noffset, uint8_t *value,
>                 int value_len, const char *comment, const char *region_prop,
> -               int region_proplen, const char *cmdname)
> +               int region_proplen, const char *cmdname, const char *algo_name)
>  {
>         int string_size;
>         int ret;
> @@ -150,6 +150,8 @@ static int fit_image_write_sig(void *fit, int noffset, uint8_t *value,
>                                           strdata, sizeof(strdata));
>                 }
>         }
> +       if (algo_name && !ret)
> +               ret = fdt_setprop_string(fit, noffset, "algo", algo_name);
>
>         return ret;
>  }
> @@ -157,17 +159,18 @@ static int fit_image_write_sig(void *fit, int noffset, uint8_t *value,
>  static int fit_image_setup_sig(struct image_sign_info *info,
>                 const char *keydir, const char *keyfile, void *fit,
>                 const char *image_name, int noffset, const char *require_keys,
> -               const char *engine_id)
> +               const char *engine_id, const char *algo_name)
>  {
>         const char *node_name;
> -       const char *algo_name;
>         const char *padding_name;
>
>         node_name = fit_get_name(fit, noffset, NULL);
> -       if (fit_image_hash_get_algo(fit, noffset, &algo_name)) {
> -               printf("Can't get algo property for '%s' signature node in '%s' image node\n",
> -                      node_name, image_name);
> -               return -1;
> +       if (!algo_name) {
> +               if (fit_image_hash_get_algo(fit, noffset, &algo_name)) {
> +                       printf("Can't get algo property for '%s' signature node in '%s' image node\n",
> +                              node_name, image_name);
> +                       return -1;
> +               }
>         }
>
>         padding_name = fdt_getprop(fit, noffset, "padding", NULL);
> @@ -215,7 +218,7 @@ static int fit_image_process_sig(const char *keydir, const char *keyfile,
>                 void *keydest, void *fit, const char *image_name,
>                 int noffset, const void *data, size_t size,
>                 const char *comment, int require_keys, const char *engine_id,
> -               const char *cmdname)
> +               const char *cmdname, const char *algo_name)
>  {
>         struct image_sign_info info;
>         struct image_region region;
> @@ -226,7 +229,7 @@ static int fit_image_process_sig(const char *keydir, const char *keyfile,
>
>         if (fit_image_setup_sig(&info, keydir, keyfile, fit, image_name,
>                                 noffset, require_keys ? "image" : NULL,
> -                               engine_id))
> +                               engine_id, algo_name))
>                 return -1;
>
>         node_name = fit_get_name(fit, noffset, NULL);
> @@ -244,7 +247,7 @@ static int fit_image_process_sig(const char *keydir, const char *keyfile,
>         }
>
>         ret = fit_image_write_sig(fit, noffset, value, value_len, comment,
> -                       NULL, 0, cmdname);
> +                       NULL, 0, cmdname, algo_name);
>         if (ret) {
>                 if (ret == -FDT_ERR_NOSPACE)
>                         return -ENOSPC;
> @@ -606,7 +609,7 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
>  int fit_image_add_verification_data(const char *keydir, const char *keyfile,
>                 void *keydest, void *fit, int image_noffset,
>                 const char *comment, int require_keys, const char *engine_id,
> -               const char *cmdname)
> +               const char *cmdname, const char* algo_name)
>  {
>         const char *image_name;
>         const void *data;
> @@ -643,7 +646,8 @@ int fit_image_add_verification_data(const char *keydir, const char *keyfile,
>                                 strlen(FIT_SIG_NODENAME))) {
>                         ret = fit_image_process_sig(keydir, keyfile, keydest,
>                                 fit, image_name, noffset, data, size,
> -                               comment, require_keys, engine_id, cmdname);
> +                               comment, require_keys, engine_id, cmdname,
> +                               algo_name);
>                 }
>                 if (ret)
>                         return ret;
> @@ -927,7 +931,8 @@ static int fit_config_get_data(void *fit, int conf_noffset, int noffset,
>  static int fit_config_process_sig(const char *keydir, const char *keyfile,
>                 void *keydest,  void *fit, const char *conf_name,
>                 int conf_noffset, int noffset, const char *comment,
> -               int require_keys, const char *engine_id, const char *cmdname)
> +               int require_keys, const char *engine_id, const char *cmdname,
> +               const char *algo_name)
>  {
>         struct image_sign_info info;
>         const char *node_name;
> @@ -945,7 +950,8 @@ static int fit_config_process_sig(const char *keydir, const char *keyfile,
>                 return -1;
>
>         if (fit_image_setup_sig(&info, keydir, keyfile, fit, conf_name, noffset,
> -                               require_keys ? "conf" : NULL, engine_id))
> +                               require_keys ? "conf" : NULL, engine_id,
> +                               algo_name))
>                 return -1;
>
>         ret = info.crypto->sign(&info, region, region_count, &value,
> @@ -962,7 +968,8 @@ static int fit_config_process_sig(const char *keydir, const char *keyfile,
>         }
>
>         ret = fit_image_write_sig(fit, noffset, value, value_len, comment,
> -                               region_prop, region_proplen, cmdname);
> +                                 region_prop, region_proplen, cmdname,
> +                                 algo_name);
>         if (ret) {
>                 if (ret == -FDT_ERR_NOSPACE)
>                         return -ENOSPC;
> @@ -992,7 +999,7 @@ static int fit_config_process_sig(const char *keydir, const char *keyfile,
>  static int fit_config_add_verification_data(const char *keydir,
>                 const char *keyfile, void *keydest, void *fit, int conf_noffset,
>                 const char *comment, int require_keys, const char *engine_id,
> -               const char *cmdname)
> +               const char *cmdname, const char *algo_name)
>  {
>         const char *conf_name;
>         int noffset;
> @@ -1011,7 +1018,7 @@ static int fit_config_add_verification_data(const char *keydir,
>                              strlen(FIT_SIG_NODENAME))) {
>                         ret = fit_config_process_sig(keydir, keyfile, keydest,
>                                 fit, conf_name, conf_noffset, noffset, comment,
> -                               require_keys, engine_id, cmdname);
> +                               require_keys, engine_id, cmdname, algo_name);
>                 }
>                 if (ret)
>                         return ret;
> @@ -1058,7 +1065,7 @@ int fit_cipher_data(const char *keydir, void *keydest, void *fit,
>  int fit_add_verification_data(const char *keydir, const char *keyfile,
>                               void *keydest, void *fit, const char *comment,
>                               int require_keys, const char *engine_id,
> -                             const char *cmdname)
> +                             const char *cmdname, const char *algo_name)
>  {
>         int images_noffset, confs_noffset;
>         int noffset;
> @@ -1082,7 +1089,7 @@ int fit_add_verification_data(const char *keydir, const char *keyfile,
>                  */
>                 ret = fit_image_add_verification_data(keydir, keyfile, keydest,
>                                 fit, noffset, comment, require_keys, engine_id,
> -                               cmdname);
> +                               cmdname, algo_name);
>                 if (ret)
>                         return ret;
>         }
> @@ -1106,7 +1113,8 @@ int fit_add_verification_data(const char *keydir, const char *keyfile,
>                 ret = fit_config_add_verification_data(keydir, keyfile, keydest,
>                                                        fit, noffset, comment,
>                                                        require_keys,
> -                                                      engine_id, cmdname);
> +                                                      engine_id, cmdname,
> +                                                      algo_name);
>                 if (ret)
>                         return ret;
>         }
> diff --git a/tools/imagetool.h b/tools/imagetool.h
> index e229a34ffc..d71027317f 100644
> --- a/tools/imagetool.h
> +++ b/tools/imagetool.h
> @@ -69,6 +69,7 @@ struct image_tool_params {
>         const char *keydest;    /* Destination .dtb for public key */
>         const char *keyfile;    /* Filename of private or public key */
>         const char *comment;    /* Comment to add to signature node */
> +       const char *algo_name;  /* Algorithm name to use hashing/signing */

NULL to use the one in the .its ?


>         int require_keys;       /* 1 to mark signing keys as 'required' */
>         int file_size;          /* Total size of output file */
>         int orig_file_size;     /* Original size for file before padding */
> diff --git a/tools/mkimage.c b/tools/mkimage.c
> index a4844d0f18..ddb79331a6 100644
> --- a/tools/mkimage.c
> +++ b/tools/mkimage.c
> @@ -154,7 +154,7 @@ static void process_args(int argc, char **argv)
>         int opt;
>
>         while ((opt = getopt(argc, argv,
> -                  "a:A:b:B:c:C:d:D:e:Ef:FG:k:i:K:ln:N:p:O:rR:qstT:vVx")) != -1) {
> +                  "a:A:b:B:c:C:d:D:e:Ef:FG:k:i:K:ln:N:p:o:O:rR:qstT:vVx")) != -1) {
>                 switch (opt) {
>                 case 'a':
>                         params.addr = strtoull(optarg, &ptr, 16);
> @@ -250,6 +250,9 @@ static void process_args(int argc, char **argv)
>                 case 'N':
>                         params.engine_id = optarg;
>                         break;
> +               case 'o':
> +                       params.algo_name = optarg;
> +                       break;
>                 case 'O':
>                         params.os = genimg_get_os_id(optarg);
>                         if (params.os < 0) {
> --
> 2.31.1
>

On Fri, Jan 14, 2022 at 10:21:19AM +0100, Jan Kiszka wrote:

> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> This permits to prepare FIT image description that do not hard-code the
> final choice of the signature algorithm, possibly requiring the user to
> patch the sources.
> 
> When -o <algo> is specified, this information is used in favor of the
> 'algo' property in the signature node. Furthermore, that property is set
> accordingly when writing the image.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

Applied to u-boot/master, thanks!