Hi Simon,
> Da: Simon Glass <s...@chromium.org>
> Inviato: mercoledì 23 novembre 2022 03:09
>
> Hi Pegorer,
>
> On Sat, 19 Nov 2022 at 11:01, Pegorer Massimo
> <massimo.pego...@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.pego...@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.pego...@vimar.com>
> > ---
> > tools/fit_image.c | 25 +++++++++++++++++--------
> > tools/mkimage.c | 18 ++++++++++++++----
> > 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"))
> > + params.require_keys = 1;
> > + else if (!strcmp(optarg, "conf"))
> > + params.require_keys = 2;
> > + 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.
Yes, seems better. Do you think I can move the image_tool_params.auto_its
from bool to int type, to carry more than just a boolean value, or do you
prefer a new 'flag' to be added to image_tool_params? The first is my
preferred one if nobody complain.
Best regards,
Massimo
> Regards,
> SImon
