Re: Patch proposal - mkimage: fit: Support signed conf 'auto' FITs

[Sean Anderson <sean.anderson@seco.com>]

On 11/22/22 21:09, Simon Glass wrote:
> Hi Pegorer,
> 
> On Sat, 19 Nov 2022 at 11:01, Pegorer Massimo <Massimo.Pegorer@vimar.com> wrote:
>>
>> Commit 87b0af9317cb4105f3f29cb0a4c28c7cd87ea65f added support for signing auto-generated (mkimage -f auto) FIT. Unfortunately, this signs 'images' subnodes but not 'configurations' ones. Following patch is a proposal to support also 'configurations' signing + 'images' hashing, as an alternative to 'images' signing, with 'auto' FIT. For this purpose, a new optional argument is added to mkimage '-r' option: any other better idea?
>>
>> =====
>>
>> From 8c8c8f421d541cc2eccb50490a57e86b81dc8df2 Mon Sep 17 00:00:00 2001
>> From: Massimo Pegorer <massimo.pegorer@vimar.com>
>> Date: Sat, 19 Nov 2022 16:25:58 +0100
>> Subject: [PATCH] mkimage: fit: Support signed conf 'auto' FITs
>>
>> Extend support for signing in auto-generated FITs. Previously, it was
>> possible to sign 'images' subnodes in auto FIT, but not 'configurations'
>> subnodes. Consequently, usage with -K and -r options (i.e. write keys as
>> 'required' in a .dtb file) resulted in adding a signature node with
>> required = "image" property in the dtb.
>>
>> This patch allows usage of an optional argument with -r option to select
>> which subnodes, 'images' ones or 'configurations' ones, have to be signed
>> (in the second case 'images' subnodes are hashed): with '-r' or '-rimage'
>> the firsts are signed, while with '-rconf' the seconds; argument values
>> different than 'image' and 'conf' are invalid.
>>
>> Example to write a key with required = "conf" attribute into a dtb file:
>>
>> mkimage -f auto -rconf -d /dev/null -K <dtb-file> -o <algo> \
>>         -g <key-name-hint> -k <path-to-key-file> <dummy-itb-file>
>>
>> Signed-off-by: Massimo Pegorer <massimo.pegorer@vimar.com>
>> ---
>>  tools/fit_image.c | 25 +++++++++++++++++--------
>>  tools/mkimage.c   | 18 ++++++++++++++----

Remember to update the man page for your next revision.

>>  2 files changed, 31 insertions(+), 12 deletions(-)
>>
>> diff --git a/tools/fit_image.c b/tools/fit_image.c
>> index 923a9755b7..c78d83d509 100644
>> --- a/tools/fit_image.c
>> +++ b/tools/fit_image.c
>> @@ -199,19 +199,22 @@ static void get_basename(char *str, int size, const char *fname)
>>  }
>>
>>  /**
>> - * add_hash_node() - Add a hash or signature node
>> + * add_hash_or_sign_node() - Add a hash or signature node
>>   *
>>   * @params: Image parameters
>>   * @fdt: Device tree to add to (in sequential-write mode)
>> + * @do_add_hash: true to add hash even if key name hint is provided
>>   *
>> - * If there is a key name hint, try to sign the images. Otherwise, just add a
>> - * CRC.
>> + * If do_add_hash is false (default) and there is a key name hint, try to add
>> + * a sign node to parent. Otherwise, just add a CRC. Rationale: if conf have
>> + * to be signed, image/dt have to be hashed even if there is a key name hint.
>>   *
>>   * Return: 0 on success, or -1 on failure
>>   */
>> -static int add_hash_node(struct image_tool_params *params, void *fdt)
>> +static int add_hash_or_sig_node(struct image_tool_params *params, void *fdt,
>> +                               bool do_add_hash)
>>  {
>> -       if (params->keyname) {
>> +       if (!do_add_hash && params->keyname) {
>>                 if (!params->algo_name) {
>>                         fprintf(stderr,
>>                                 "%s: Algorithm name must be specified\n",
>> @@ -269,7 +272,7 @@ static int fit_write_images(struct image_tool_params *params, char *fdt)
>>         ret = fdt_property_file(params, fdt, FIT_DATA_PROP, params->datafile);
>>         if (ret)
>>                 return ret;
>> -       ret = add_hash_node(params, fdt);
>> +       ret = add_hash_or_sig_node(params, fdt, (params->require_keys == 2));
>>         if (ret)
>>                 return ret;
>>         fdt_end_node(fdt);
>> @@ -294,7 +297,8 @@ static int fit_write_images(struct image_tool_params *params, char *fdt)
>>                                     genimg_get_arch_short_name(params->arch));
>>                 fdt_property_string(fdt, FIT_COMP_PROP,
>>                                     genimg_get_comp_short_name(IH_COMP_NONE));
>> -               ret = add_hash_node(params, fdt);
>> +               ret = add_hash_or_sig_node(params, fdt,
>> +                                          (params->require_keys == 2));
>>                 if (ret)
>>                         return ret;
>>                 fdt_end_node(fdt);
>> @@ -314,7 +318,8 @@ static int fit_write_images(struct image_tool_params *params, char *fdt)
>>                                         params->fit_ramdisk);
>>                 if (ret)
>>                         return ret;
>> -               ret = add_hash_node(params, fdt);
>> +               ret = add_hash_or_sig_node(params, fdt,
>> +                                          (params->require_keys == 2));
>>                 if (ret)
>>                         return ret;
>>                 fdt_end_node(fdt);
>> @@ -366,6 +371,8 @@ static void fit_write_configs(struct image_tool_params *params, char *fdt)
>>
>>                 snprintf(str, sizeof(str), FIT_FDT_PROP "-%d", upto);
>>                 fdt_property_string(fdt, FIT_FDT_PROP, str);
>> +               if (params->require_keys == 2)
>> +                       add_hash_or_sig_node(params, fdt, false);
>>                 fdt_end_node(fdt);
>>         }
>>
>> @@ -378,6 +385,8 @@ static void fit_write_configs(struct image_tool_params *params, char *fdt)
>>                 if (params->fit_ramdisk)
>>                         fdt_property_string(fdt, FIT_RAMDISK_PROP,
>>                                             FIT_RAMDISK_PROP "-1");
>> +               if (params->require_keys == 2)
>> +                       add_hash_or_sig_node(params, fdt, false);
>>
>>                 fdt_end_node(fdt);
>>         }
>> diff --git a/tools/mkimage.c b/tools/mkimage.c
>> index 30c6df7708..4d4f128b54 100644
>> --- a/tools/mkimage.c
>> +++ b/tools/mkimage.c
>> @@ -125,7 +125,7 @@ static void usage(const char *msg)
>>                 "          -c => add comment in signature node\n"
>>                 "          -F => re-sign existing FIT image\n"
>>                 "          -p => place external data at a static position\n"
>> -               "          -r => mark keys used as 'required' in dtb\n"
>> +               "          -r => mark keys used as 'required' in dtb (-rconf for 'auto' FIT with signed config)\n"
>>                 "          -N => openssl engine to use for signing\n"
>>                 "          -o => algorithm to use for signing\n");
>>  #else
>> @@ -159,7 +159,7 @@ static int add_content(int type, const char *fname)
>>  }
>>
>>  static const char optstring[] =
>> -       "a:A:b:B:c:C:d:D:e:Ef:Fg:G:i:k:K:ln:N:o:O:p:qrR:stT:vVx";
>> +       "a:A:b:B:c:C:d:D:e:Ef:Fg:G:i:k:K:ln:N:o:O:p:qr::R:stT:vVx";
>>
>>  static const struct option longopts[] = {
>>         { "load-address", required_argument, NULL, 'a' },
>> @@ -187,7 +187,7 @@ static const struct option longopts[] = {
>>         { "os", required_argument, NULL, 'O' },
>>         { "position", required_argument, NULL, 'p' },
>>         { "quiet", no_argument, NULL, 'q' },
>> -       { "key-required", no_argument, NULL, 'r' },
>> +       { "key-required", optional_argument, NULL, 'r' },
>>         { "secondary-config", required_argument, NULL, 'R' },
>>         { "no-copy", no_argument, NULL, 's' },
>>         { "touch", no_argument, NULL, 't' },
>> @@ -326,7 +326,12 @@ static void process_args(int argc, char **argv)
>>                         params.quiet = 1;
>>                         break;
>>                 case 'r':
>> -                       params.require_keys = 1;
>> +                       if (!optarg || !strcmp(optarg, "image"))

The default should be "conf", as that is the current behavior.

>> +                               params.require_keys = 1;
>> +                       else if (!strcmp(optarg, "conf"))
>> +                               params.require_keys = 2;

Please use an enum instead of 1/2/etc.

Can we also support "both"?

>> +                       else
>> +                               usage("Invalid key-required option argument");
>>                         break;
>>                 case 'R':
>>                         /*
>> @@ -370,6 +375,11 @@ static void process_args(int argc, char **argv)
>>         if (optind < argc)
>>                 params.imagefile = argv[optind];
>>
>> +       if (params.require_keys == 2)
>> +               if (!params.auto_its || !params.keyname || !params.algo_name)
>> +                       usage("Auto FIT with signed config requires -f auto "
>> +                               "-rconf -g <key name hint> -o <algorithm>");
>> +
>>         /*
>>          * For auto-generated FIT images we need to know the image type to put
>>          * in the FIT, which is separate from the file's image type (which
>> --
>> 2.34.1
>>
> 
> I think this is a reasonable feature, but how about using '-f
> auto-conf' as the way to select this feature? The '-r' argument is
> intended to indicate that the keys are required to be verified.

I think that extending -r with an argument is reasonable here. There's no way to
specify required = "image" either...

--Sean