Patch proposal - mkimage: fit: Support signed conf 'auto' FITs
Simon Glass
sjg at chromium.org
Sun Dec 4 22:16:57 CET 2022
()Hi Sean,
On Tue, 29 Nov 2022 at 04:45, Sean Anderson <sean.anderson at seco.com> wrote:
>
> 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.
>
> >> 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...
Note that -F can be used to sign a FIT later, after it is created.
That option does not allow the creation of new configurations, though,
so I don't think we need to worry about that angle.
We should try to support not using -r so that the signatures are added
but not required. After all, the -r flag actually affects the
verification data in U-Boot's FDT, not the FIT.
fit_image_setup_sig() is called with a string arg for require_keys,
similar to what is used here, but I think that is a different thing.
So overall I think that the image of enabling the feature in this patch is that:
- a 'signature' subnode is added to each configuration (or image) in
add_hash_or_sig_node()
- the crc32 in the image nodes changes to a sha
- the key may or may not be required
These sound like things that should only be done during the initial
FIT creation, with '-f auto', not in later signature addition with -F.
The current creation of signatures in the image nodes[1] seems a bit
odd to me, but I suppose it makes sense if the goal is just to sign
images. When signing configs we want to hash the images, not sign
them, so perhaps signing of images with '-f auto' should be dropped? I
don't mind either way, though.
So I do think that '-f auto-conf' is the right thing to do here.
Alternatively down the road we could add one more flag which controls
the operation of '-f auto', with respect to image nodes and config
nodes:
-S <image>,<config>
so:
- (empty) - creates crc nodes in images, no signing of configurations
- sha256 - creates sha256 nodes in images
- sha1,rsa2048 - creates sha1 nodes in images, signs configurations with rsa2048
But I'm not sure how flexible we want this. Keeping it simple along
the lines of this patch seems good to me.
Also this patch should have a test.
Regards,
Simon
[1] 87b0af9317c ("mkimage: Support signing 'auto' FITs")
More information about the U-Boot
mailing list