R: Patch proposal - mkimage: fit: Support signed conf 'auto' FITs
Pegorer Massimo
Massimo.Pegorer at vimar.com
Fri Dec 9 17:09:48 CET 2022
Hi Sean,
> Da: Sean Anderson <sean.anderson at seco.com>
> Inviato: lunedì 28 novembre 2022 16:46
>
> On 11/22/22 21:09, Simon Glass wrote:
> > Hi Pegorer,
> >
> > On Sat, 19 Nov 2022 at 11:01, Pegorer Massimo
> <Massimo.Pegorer at 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 at 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 at vimar.com>
> >> ---
> >> tools/fit_image.c | 25 +++++++++++++++++--------
> >> tools/mkimage.c | 18 ++++++++++++++----
>
> Remember to update the man page for your next revision.
Yes, of course. This was just a preliminary patch to share the idea.
> >> 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.
Current behaviour is to sign images and not configurations. Of
course, I think signed configurations would be a preferable default.
> >> + params.require_keys = 1;
> >> + else if (!strcmp(optarg, "conf"))
> >> + params.require_keys = 2;
>
> Please use an enum instead of 1/2/etc.
Yes, of course.
> Can we also support "both"?
That's an interesting, but as per doc/uImage.FIT/signature.txt:
" - required: If present this indicates that the key must be verified for the
image / configuration to be considered valid. Only required keys are
normally verified by the FIT image booting algorithm. Valid values are
"image" to force verification of all images, and "conf" to force verification
of the selected configuration (which then relies on hashes in the images to
verify those)."
By the way it is quite a limitation.
> >> + 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
See the my comments on the other reply. Thanks.
Regards,
Massimo
More information about the U-Boot
mailing list