* Patch proposal - mkimage: fit: Support signed conf 'auto' FITs
@ 2022-11-19 18:00 Pegorer Massimo
2022-11-23 2:09 ` Simon Glass
0 siblings, 1 reply; 12+ messages in thread
From: Pegorer Massimo @ 2022-11-19 18:00 UTC (permalink / raw)
To: u-boot; +Cc: Simon Glass, Sean Anderson
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 ++++++++++++++----
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: Patch proposal - mkimage: fit: Support signed conf 'auto' FITs
2022-11-19 18:00 Patch proposal - mkimage: fit: Support signed conf 'auto' FITs Pegorer Massimo
@ 2022-11-23 2:09 ` Simon Glass
2022-11-24 7:32 ` R: " Pegorer Massimo
2022-11-28 15:45 ` Sean Anderson
0 siblings, 2 replies; 12+ messages in thread
From: Simon Glass @ 2022-11-23 2:09 UTC (permalink / raw)
To: Pegorer Massimo; +Cc: u-boot, Sean Anderson
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 ++++++++++++++----
> 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.
Regards,
SImon
^ permalink raw reply [flat|nested] 12+ messages in thread
* R: Patch proposal - mkimage: fit: Support signed conf 'auto' FITs
2022-11-23 2:09 ` Simon Glass
@ 2022-11-24 7:32 ` Pegorer Massimo
2022-11-28 15:45 ` Sean Anderson
1 sibling, 0 replies; 12+ messages in thread
From: Pegorer Massimo @ 2022-11-24 7:32 UTC (permalink / raw)
To: Simon Glass; +Cc: u-boot, Sean Anderson
Hi Simon,
> Da: Simon Glass <sjg@chromium.org>
> Inviato: mercoledì 23 novembre 2022 03:09
>
> 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 ++++++++++++++----
> > 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Patch proposal - mkimage: fit: Support signed conf 'auto' FITs
2022-11-23 2:09 ` Simon Glass
2022-11-24 7:32 ` R: " Pegorer Massimo
@ 2022-11-28 15:45 ` Sean Anderson
2022-12-04 21:16 ` Simon Glass
2022-12-09 16:09 ` Pegorer Massimo
1 sibling, 2 replies; 12+ messages in thread
From: Sean Anderson @ 2022-11-28 15:45 UTC (permalink / raw)
To: Simon Glass, Pegorer Massimo; +Cc: u-boot
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Patch proposal - mkimage: fit: Support signed conf 'auto' FITs
2022-11-28 15:45 ` Sean Anderson
@ 2022-12-04 21:16 ` Simon Glass
2022-12-09 15:47 ` R: " Pegorer Massimo
2022-12-09 16:09 ` Pegorer Massimo
1 sibling, 1 reply; 12+ messages in thread
From: Simon Glass @ 2022-12-04 21:16 UTC (permalink / raw)
To: Sean Anderson; +Cc: Pegorer Massimo, u-boot
()Hi Sean,
On Tue, 29 Nov 2022 at 04:45, Sean Anderson <sean.anderson@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@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...
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")
^ permalink raw reply [flat|nested] 12+ messages in thread
* R: Patch proposal - mkimage: fit: Support signed conf 'auto' FITs
2022-12-04 21:16 ` Simon Glass
@ 2022-12-09 15:47 ` Pegorer Massimo
0 siblings, 0 replies; 12+ messages in thread
From: Pegorer Massimo @ 2022-12-09 15:47 UTC (permalink / raw)
To: Simon Glass, Sean Anderson; +Cc: u-boot
Hi,
> Da: Simon Glass <sjg@chromium.org>
> Inviato: domenica 4 dicembre 2022 22:17
>
> ()Hi Sean,
>
> On Tue, 29 Nov 2022 at 04:45, Sean Anderson <sean.anderson@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@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 ++++++++++++++----
[...]
> > >> --
> > >> 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.
I agree, '-r' makes sense only with '-K <dtb>'. Therefore, it is preferrable to
allow to specify in a different way what and how to sign in the auto-FIT:
consider someone would like to create signed auto-FIT without the need to
add the key to any FTD.
From a semantic point of view, not using '-r' would be clearer. Otherwise,
we would force an overload of '-r' current meaning, which is "when public
key data are added to the dtb file, include also the "required" property".
The point here is that we have two actions - 1. add hash and/or signatures
to images and configurations in a FIT; 2. add public key data, with or without
"required" property, in a FTD; - which are "independent" but require being
"coordinated" to have a coherent and meaningful final result.
> 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.
I think we can keep current behaviour (image signing) when '-f auto' is used
with signing parameters, and the suggested '-f auto-conf' (with mandatory
signing options) to have an auto-FIT with signed configurations. Or swap the
default, if preferred (for the mix and match issue, and not complaining with
backward compatibility):
- '-f auto' + signing params for auto-FIT with signed configurations
- '-f auto-signimg' + signing params for auto-FIT with signed images
> 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.
I would not add more flexibility, as in case people can draw their required FIT
structure writing an ad-hoc ITS and invoking mkimage with '-f <file.its>'. By the
way we are discussing about auto FIT, which is just a single (kernel) image plus
one ore more dtbs plus a ramdisk.
There is one more interesting case: usage of mkimage just to add public key to
a dtb (with or without "required" property), without signing anything. E.g.:
mkimage -f auto -K <dtb> [-r] -d /dev/null -k ... -g ... -o ... etc...
Also for this case the '-f auto-conf' seems fine, without dependency on '-r'.
> Also this patch should have a test.
>
> Regards,
> Simon
>
> [1] 87b0af9317c ("mkimage: Support signing 'auto' FITs")
Finally I am going to propose a first patch that will support the following cases:
1. - creates crc nodes in images, no signing of configurations (original behaviour)
syntax: '-f auto' without signing options
2. - sign images with <algo>, no signing of configurations [1]
syntax: '-f auto -k ... -g ... -o <algo>'
3. - creates sha1 nodes in images, sign configurations with <algo>
syntax: '-f auto-conf -k ... -g ... -o <algo>'
Regards,
Massimo
^ permalink raw reply [flat|nested] 12+ messages in thread
* R: Patch proposal - mkimage: fit: Support signed conf 'auto' FITs
2022-11-28 15:45 ` Sean Anderson
2022-12-04 21:16 ` Simon Glass
@ 2022-12-09 16:09 ` Pegorer Massimo
2022-12-11 14:54 ` [PATCH] mkimage: fit: Support signed configurations in " Pegorer Massimo
1 sibling, 1 reply; 12+ messages in thread
From: Pegorer Massimo @ 2022-12-09 16:09 UTC (permalink / raw)
To: Sean Anderson; +Cc: u-boot, Simon Glass
Hi Sean,
> Da: Sean Anderson <sean.anderson@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@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.
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] mkimage: fit: Support signed configurations in 'auto' FITs
2022-12-09 16:09 ` Pegorer Massimo
@ 2022-12-11 14:54 ` Pegorer Massimo
2022-12-15 21:16 ` Simon Glass
0 siblings, 1 reply; 12+ messages in thread
From: Pegorer Massimo @ 2022-12-11 14:54 UTC (permalink / raw)
To: Simon Glass, Sean Anderson, u-boot
Hi,
The patch follows, as per discussion in email thread "Patch proposal
- mkimage: fit: Support signed conf 'auto' FITs". Let me know if you
prefer something to be changed, or patch to be split in several
commits.
I have updated the man page with description of the new feature and
examples. Also fixed some wrong or misleading information.
===
mkimage: fit: Support signed configurations in 'auto' FITs
Extend support for signing in auto-generated (-f auto) FIT. Previously,
it was possible to get signed 'images' subnodes in the FIT using
options -g and -o together with -f auto. This patch allows signing
'configurations' subnodes instead of 'images' ones (which are hashed),
using option -f auto-conf instead of -f auto. Adding also -K <dtb> and
-r options, will add public key to <dtb> file with required = "conf"
property.
Summary:
-f auto => FIT with crc32 images
-f auto -g ... -o ... => FIT with signed images
-f auto-conf -g ... -o ... => FIT with sha1 images and signed confs
Example: FIT with kernel, two device tree files, and signed
configurations; public key (needed to verify signatures) is
added to u-boot.dtb with required = "conf" property.
mkimage -f auto-conf -A arm -O linux -T kernel -C none -a 43e00000 \
-e 0 -d vmlinuz -b /path/to/first.dtb -b /path/to/second.dtb \
-k /folder/with/key-files -g keyname -o sha256,rsa4096 \
-K u-boot.dtb -r kernel.itb
Example: Add public key with required = "conf" property to u-boot.dtb
without needing to sign anything. This will also create a useless FIT
named unused.itb.
mkimage -f auto-conf -d /dev/null -k /folder/with/key-files \
-g keyname -o sha256,rsa4096 -K u-boot.dtb -r unused.itb
Signed-off-by: Massimo Pegorer <massimo.pegorer@vimar.com>
---
doc/mkimage.1 | 119 ++++++++++++++++++++++++++++++++--------------
tools/fit_image.c | 75 +++++++++++++++++++----------
tools/imagetool.h | 10 +++-
tools/mkimage.c | 23 +++++++--
4 files changed, 160 insertions(+), 67 deletions(-)
diff --git a/doc/mkimage.1 b/doc/mkimage.1
index 353ea8b2f7..d8727ec73c 100644
--- a/doc/mkimage.1
+++ b/doc/mkimage.1
@@ -22,7 +22,8 @@ mkimage \- generate images for U-Boot
.SY mkimage
.RI [ option\~ .\|.\|.\&]
.BI \-f\~ image-tree-source-file\c
-.RB | auto
+.RB | auto\c
+.RB | auto-conf
.I image-file-name
.YS
.
@@ -296,9 +297,9 @@ FIT. See
for details on using external data.
.
.TP
-\fB\-f \fIimage-tree-source-file\fR | \fBauto
+\fB\-f \fIimage-tree-source-file\fR | \fBauto\fR | \fBauto-conf
.TQ
-\fB\-\-fit \fIimage-tree-source-file\fR | \fBauto
+\fB\-\-fit \fIimage-tree-source-file\fR | \fBauto\fR | \fBauto-conf
Image tree source file that describes the structure and contents of the
FIT image.
.IP
@@ -317,7 +318,25 @@ and
options may be used to specify the image to include in the FIT and its
attributes. No
.I image-tree-source-file
-is required.
+is required. The
+.BR \-g ,
+.BR \-o ,
+and
+.B \-k
+or
+.B \-G
+options may be used to get \(oqimages\(cq signed subnodes in the generated
+auto FIT. Instead, to get \(oqconfigurations\(cq signed subnodes and
+\(oqimages\(cq hashed subnodes, pass
+.BR "\-f auto-conf".
+In this case
+.BR \-g ,
+.BR \-o ,
+and
+.B \-k
+or
+.B \-G
+are mandatory options.
.
.TP
.B \-F
@@ -348,16 +367,16 @@ for use with signing, and a certificate
necessary when embedding it into another device tree using
.BR \-K .
.I name
-defaults to the value of the signature node's \(oqkey-name-hint\(cq property,
-but may be overridden using
-.BR \-g .
+is the value of the signature node's \(oqkey-name-hint\(cq property.
.
.TP
.BI \-G " key-file"
.TQ
.BI \-\-key\-file " key-file"
Specifies the private key file to use when signing. This option may be used
-instead of \-k.
+instead of \-k. Useful when the private key file basename does not match
+\(oqkey-name-hint\(cq value. But note that it may lead to unexpected results
+when used together with -K and/or -k options.
.
.TP
.BI \-K " key-destination"
@@ -373,49 +392,50 @@ CONFIG_OF_CONTROL in U-Boot.
.BI \-g " key-name-hint"
.TQ
.BI \-\-key\-name\-hint " key-name-hint"
-Overrides the signature node's \(oqkey-name-hint\(cq property. This is
-especially useful when signing an image with
-.BR "\-f auto" .
-This is the
-.I name
-part of the key. The directory part is set by
-.BR \-k .
-This option also indicates that the images included in the FIT should be signed.
-If this option is specified, then
+Specifies the value of signature node \(oqkey-name-hint\(cq property for
+an automatically generated FIT image. It makes sense only when used with
+.B "\-f auto"
+or
+.BR "\-f auto-conf".
+This option also indicates that the images or configurations included in
+the FIT should be signed. If this option is specified, then
.B \-o
must be specified as well.
.
.TP
-.BI \-o " crypto" , checksum
+.BI \-o " checksum" , crypto
.TQ
-.BI \-\-algo " crypto" , checksum
-Specifies the algorithm to be used for signing a FIT image. The default is
-taken from the signature node's \(oqalgo\(cq property.
+.BI \-\-algo " checksum" , crypto
+Specifies the algorithm to be used for signing a FIT image, overriding value
+taken from the signature node \(oqalgo\(cq property in the
+.IR image-tree-source-file .
+It is mandatory for automatically generated FIT.
+.IP
The valid values for
-.I crypto
+.I checksum
are:
.RS
.IP
.TS
lb.
-rsa2048
-rsa3072
-rsa4096
-ecdsa256
+sha1
+sha256
+sha384
+sha512
.TE
.RE
.IP
The valid values for
-.I checksum
-are
+.I crypto
+are:
.RS
.IP
.TS
lb.
-sha1
-sha256
-sha384
-sha512
+rsa2048
+rsa3072
+rsa4096
+ecdsa256
.TE
.RE
.
@@ -423,9 +443,13 @@ sha512
.B \-r
.TQ
.B \-\-key\-required
-Specifies that keys used to sign the FIT are required. This means that they
-must be verified for the image to boot. Without this option, the verification
-will be optional (useful for testing but not for release).
+Specifies that keys used to sign the FIT are required. This means that images
+or configurations signatures must be verified before using them (i.e. to
+boot). Without this option, the verification will be optional (useful for
+testing but not for release). It makes sense only when used with
+.BR \-K.
+When both, images and configurations, are signed, \(oqrequired\(cq property
+value will be "conf".
.
.TP
.BI \-N " engine"
@@ -716,7 +740,7 @@ skipping those for which keys cannot be found. Also add a comment.
.EE
.RE
.P
-Add public keys to u\-boot.dtb without needing a FIT to sign. This will also
+Add public key to u\-boot.dtb without needing a FIT to sign. This will also
create a FIT containing an images node with no data named unused.itb.
.RS
.P
@@ -726,6 +750,16 @@ create a FIT containing an images node with no data named unused.itb.
.EE
.RE
.P
+Add public key with required = "conf" property to u\-boot.dtb without needing
+a FIT to sign. This will also create a useless FIT named unused.itb.
+.RS
+.P
+.EX
+\fBmkimage \-f auto-conf \-d /dev/null \-k /public/signing\-keys \-g dev \\
+ \-o sha256,rsa2048 \-K u\-boot.dtb -r unused.itb
+.EE
+.RE
+.P
Update an existing FIT image, signing it with additional keys.
Add corresponding public keys into u\-boot.dtb. This will resign all images
with keys that are available in the new directory. Images that request signing
@@ -768,6 +802,19 @@ file is required.
\-d vmlinuz \-k /secret/signing\-keys \-g dev \-o sha256,rsa2048 kernel.itb
.EE
.RE
+.P
+Create a FIT image containing a kernel and some device tree files, signing
+each configuration, using automatic mode. Moreover, the public key needed to
+verify signatures is added to u\-boot.dtb with required = "conf" property.
+.RS
+.P
+.EX
+\fBmkimage \-f auto-conf \-A arm \-O linux \-T kernel \-C none \-a 43e00000 \\
+ \-e 0 \-d vmlinuz \-b /path/to/file\-1.dtb \-b /path/to/file\-2.dtb \\
+ \-k /folder/with/signing\-keys \-g dev \-o sha256,rsa2048 \\
+ \-K u\-boot.dtb -r kernel.itb
+.EE
+.RE
.
.SH SEE ALSO
.BR dtc (1),
diff --git a/tools/fit_image.c b/tools/fit_image.c
index 923a9755b7..d15a377779 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -199,36 +199,59 @@ static void get_basename(char *str, int size, const char *fname)
}
/**
- * add_hash_node() - Add a hash or signature node
+ * fit_add_hash_or_sign() - Add a hash or signature node
*
* @params: Image parameters
* @fdt: Device tree to add to (in sequential-write mode)
+ * @is_images_subnode: 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.
- *
- * Return: 0 on success, or -1 on failure
+ * 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.
*/
-static int add_hash_node(struct image_tool_params *params, void *fdt)
+static void fit_add_hash_or_sign(struct image_tool_params *params, void *fdt,
+ bool is_images_subnode)
{
- if (params->keyname) {
- if (!params->algo_name) {
- fprintf(stderr,
- "%s: Algorithm name must be specified\n",
- params->cmdname);
- return -1;
+ const char *hash_algo = "crc32";
+ bool do_hash = false;
+ bool do_sign = false;
+
+ switch (params->auto_fit) {
+ case AF_OFF:
+ break;
+ case AF_HASHED_IMG:
+ do_hash = is_images_subnode;
+ break;
+ case AF_SIGNED_IMG:
+ do_sign = is_images_subnode;
+ break;
+ case AF_SIGNED_CONF:
+ if (is_images_subnode) {
+ do_hash = true;
+ hash_algo = "sha1";
+ } else {
+ do_sign = true;
}
+ break;
+ default:
+ fprintf(stderr,
+ "%s: Unsupported auto FIT mode %u\n",
+ params->cmdname, params->auto_fit);
+ break;
+ }
+
+ if (do_hash) {
+ fdt_begin_node(fdt, FIT_HASH_NODENAME);
+ fdt_property_string(fdt, FIT_ALGO_PROP, hash_algo);
+ fdt_end_node(fdt);
+ }
- fdt_begin_node(fdt, "signature-1");
+ if (do_sign) {
+ fdt_begin_node(fdt, FIT_SIG_NODENAME);
fdt_property_string(fdt, FIT_ALGO_PROP, params->algo_name);
fdt_property_string(fdt, FIT_KEY_HINT, params->keyname);
- } else {
- fdt_begin_node(fdt, "hash-1");
- fdt_property_string(fdt, FIT_ALGO_PROP, "crc32");
+ fdt_end_node(fdt);
}
-
- fdt_end_node(fdt);
- return 0;
}
/**
@@ -269,9 +292,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);
- if (ret)
- return ret;
+ fit_add_hash_or_sign(params, fdt, true);
fdt_end_node(fdt);
/* Now the device tree files if available */
@@ -294,7 +315,7 @@ 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);
+ fit_add_hash_or_sign(params, fdt, true);
if (ret)
return ret;
fdt_end_node(fdt);
@@ -314,7 +335,7 @@ 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);
+ fit_add_hash_or_sign(params, fdt, true);
if (ret)
return ret;
fdt_end_node(fdt);
@@ -366,6 +387,7 @@ 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);
+ fit_add_hash_or_sign(params, fdt, false);
fdt_end_node(fdt);
}
@@ -378,6 +400,7 @@ 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");
+ fit_add_hash_or_sign(params, fdt, false);
fdt_end_node(fdt);
}
@@ -721,7 +744,7 @@ static int fit_handle_file(struct image_tool_params *params)
sprintf (tmpfile, "%s%s", params->imagefile, MKIMAGE_TMPFILE_SUFFIX);
/* We either compile the source file, or use the existing FIT image */
- if (params->auto_its) {
+ if (params->auto_fit) {
if (fit_build(params, tmpfile)) {
fprintf(stderr, "%s: failed to build FIT\n",
params->cmdname);
@@ -905,7 +928,7 @@ static int fit_extract_contents(void *ptr, struct image_tool_params *params)
static int fit_check_params(struct image_tool_params *params)
{
- if (params->auto_its)
+ if (params->auto_fit)
return 0;
return ((params->dflag && params->fflag) ||
(params->fflag && params->lflag) ||
diff --git a/tools/imagetool.h b/tools/imagetool.h
index ca7c2e48ba..fdceea46c0 100644
--- a/tools/imagetool.h
+++ b/tools/imagetool.h
@@ -39,6 +39,14 @@ struct content_info {
const char *fname;
};
+/* FIT auto generation modes */
+enum af_mode {
+ AF_OFF = 0, /* Needs .its or existing FIT to be provided */
+ AF_HASHED_IMG, /* Auto FIT with crc32 hashed images subnodes */
+ AF_SIGNED_IMG, /* Auto FIT with signed images subnodes */
+ AF_SIGNED_CONF, /* Auto FIT with sha1 images and signed configs */
+};
+
/*
* This structure defines all such variables those are initialized by
* mkimage and dumpimage main core and need to be referred by image
@@ -79,7 +87,7 @@ struct image_tool_params {
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 */
- bool auto_its; /* Automatically create the .its file */
+ enum af_mode auto_fit; /* Automatically create the FIT */
int fit_image_type; /* Image type to put into the FIT */
char *fit_ramdisk; /* Ramdisk file to include */
struct content_info *content_head; /* List of files to include */
diff --git a/tools/mkimage.c b/tools/mkimage.c
index 30c6df7708..75b72420b9 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -104,7 +104,7 @@ static void usage(const char *msg)
" -v ==> verbose\n",
params.cmdname);
fprintf(stderr,
- " %s [-D dtc_options] [-f fit-image.its|-f auto|-F] [-b <dtb> [-b <dtb>]] [-E] [-B size] [-i <ramdisk.cpio.gz>] fit-image\n"
+ " %s [-D dtc_options] [-f fit-image.its|-f auto|-f auto-conf|-F] [-b <dtb> [-b <dtb>]] [-E] [-B size] [-i <ramdisk.cpio.gz>] fit-image\n"
" <dtb> file is used with -f auto, it may occur multiple times.\n",
params.cmdname);
fprintf(stderr,
@@ -271,7 +271,10 @@ static void process_args(int argc, char **argv)
break;
case 'f':
datafile = optarg;
- params.auto_its = !strcmp(datafile, "auto");
+ if (!strcmp(datafile, "auto"))
+ params.auto_fit = AF_HASHED_IMG;
+ else if (!strcmp(datafile, "auto-conf"))
+ params.auto_fit = AF_SIGNED_CONF;
/* fallthrough */
case 'F':
/*
@@ -283,6 +286,7 @@ static void process_args(int argc, char **argv)
break;
case 'g':
params.keyname = optarg;
+ break;
case 'G':
params.keyfile = optarg;
break;
@@ -370,6 +374,17 @@ static void process_args(int argc, char **argv)
if (optind < argc)
params.imagefile = argv[optind];
+ if (params.auto_fit == AF_SIGNED_CONF) {
+ if (!params.keyname || !params.algo_name)
+ usage("Auto FIT with signed configs requires -f auto-conf "
+ "-g <key name hint> -o <algorithm>");
+ } else if (params.auto_fit == AF_HASHED_IMG && params.keyname) {
+ params.auto_fit = AF_SIGNED_IMG;
+ if (!params.algo_name)
+ usage("Auto FIT with signed images requires -f auto "
+ "-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
@@ -377,8 +392,8 @@ static void process_args(int argc, char **argv)
*/
if (params.type == IH_TYPE_FLATDT) {
params.fit_image_type = type ? type : IH_TYPE_KERNEL;
- /* For auto_its, datafile is always 'auto' */
- if (!params.auto_its)
+ /* For auto-FIT, datafile has to be provided with -d */
+ if (!params.auto_fit)
params.datafile = datafile;
else if (!params.datafile)
usage("Missing data file for auto-FIT (use -d)");
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] mkimage: fit: Support signed configurations in 'auto' FITs
2022-12-11 14:54 ` [PATCH] mkimage: fit: Support signed configurations in " Pegorer Massimo
@ 2022-12-15 21:16 ` Simon Glass
0 siblings, 0 replies; 12+ messages in thread
From: Simon Glass @ 2022-12-15 21:16 UTC (permalink / raw)
To: Pegorer Massimo; +Cc: Sean Anderson, u-boot
Hi Pegorer,
On Sun, 11 Dec 2022 at 06:54, Pegorer Massimo <Massimo.Pegorer@vimar.com> wrote:
>
> Hi,
>
> The patch follows, as per discussion in email thread "Patch proposal
> - mkimage: fit: Support signed conf 'auto' FITs". Let me know if you
> prefer something to be changed, or patch to be split in several
> commits.
>
> I have updated the man page with description of the new feature and
> examples. Also fixed some wrong or misleading information.
>
> ===
Use:
Commit-notes:
notes go here
END
(assuming you are using patman)
We don't want the message above to appear in the commit log.
>
> mkimage: fit: Support signed configurations in 'auto' FITs
>
> Extend support for signing in auto-generated (-f auto) FIT. Previously,
> it was possible to get signed 'images' subnodes in the FIT using
> options -g and -o together with -f auto. This patch allows signing
> 'configurations' subnodes instead of 'images' ones (which are hashed),
> using option -f auto-conf instead of -f auto. Adding also -K <dtb> and
> -r options, will add public key to <dtb> file with required = "conf"
> property.
>
> Summary:
> -f auto => FIT with crc32 images
> -f auto -g ... -o ... => FIT with signed images
> -f auto-conf -g ... -o ... => FIT with sha1 images and signed confs
>
> Example: FIT with kernel, two device tree files, and signed
> configurations; public key (needed to verify signatures) is
> added to u-boot.dtb with required = "conf" property.
>
> mkimage -f auto-conf -A arm -O linux -T kernel -C none -a 43e00000 \
> -e 0 -d vmlinuz -b /path/to/first.dtb -b /path/to/second.dtb \
> -k /folder/with/key-files -g keyname -o sha256,rsa4096 \
> -K u-boot.dtb -r kernel.itb
>
> Example: Add public key with required = "conf" property to u-boot.dtb
> without needing to sign anything. This will also create a useless FIT
> named unused.itb.
>
> mkimage -f auto-conf -d /dev/null -k /folder/with/key-files \
> -g keyname -o sha256,rsa4096 -K u-boot.dtb -r unused.itb
>
> Signed-off-by: Massimo Pegorer <massimo.pegorer@vimar.com>
> ---
> doc/mkimage.1 | 119 ++++++++++++++++++++++++++++++++--------------
> tools/fit_image.c | 75 +++++++++++++++++++----------
> tools/imagetool.h | 10 +++-
> tools/mkimage.c | 23 +++++++--
> 4 files changed, 160 insertions(+), 67 deletions(-)
Looks good, but it does need a test, please. See test/py/tests/fit.py
for an example
https://u-boot.readthedocs.io/en/latest/develop/py_testing.html
Regards,
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mkimage: fit: Support signed configurations in 'auto' FITs
2023-01-05 9:31 Massimo Pegorer
2023-01-13 18:00 ` Simon Glass
@ 2023-01-27 19:07 ` Tom Rini
1 sibling, 0 replies; 12+ messages in thread
From: Tom Rini @ 2023-01-27 19:07 UTC (permalink / raw)
To: Massimo Pegorer
Cc: u-boot, Heinrich Schuchardt, Jan Kiszka, Jessica Clarke,
Pali Rohár, Samuel Holland, Sean Anderson, Simon Glass,
Stefan Eichenberger
[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]
On Thu, Jan 05, 2023 at 10:31:09AM +0100, Massimo Pegorer wrote:
> Extend support for signing in auto-generated (-f auto) FIT. Previously,
> it was possible to get signed 'images' subnodes in the FIT using
> options -g and -o together with -f auto. This patch allows signing
> 'configurations' subnodes instead of 'images' ones (which are hashed),
> using option -f auto-conf instead of -f auto. Adding also -K <dtb> and
> -r options, will add public key to <dtb> file with required = "conf"
> property.
>
> Summary:
> -f auto => FIT with crc32 images
> -f auto -g ... -o ... => FIT with signed images
> -f auto-conf -g ... -o ... => FIT with sha1 images and signed confs
>
> Example: FIT with kernel, two device tree files, and signed
> configurations; public key (needed to verify signatures) is
> added to u-boot.dtb with required = "conf" property.
>
> mkimage -f auto-conf -A arm -O linux -T kernel -C none -a 43e00000 \
> -e 0 -d vmlinuz -b /path/to/first.dtb -b /path/to/second.dtb \
> -k /folder/with/key-files -g keyname -o sha256,rsa4096 \
> -K u-boot.dtb -r kernel.itb
>
> Example: Add public key with required = "conf" property to u-boot.dtb
> without needing to sign anything. This will also create a useless FIT
> named unused.itb.
>
> mkimage -f auto-conf -d /dev/null -k /folder/with/key-files \
> -g keyname -o sha256,rsa4096 -K u-boot.dtb -r unused.itb
>
> Signed-off-by: Massimo Pegorer <massimo.pegorer@vimar.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
Applied to u-boot/master, thanks!
--
Tom
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mkimage: fit: Support signed configurations in 'auto' FITs
2023-01-05 9:31 Massimo Pegorer
@ 2023-01-13 18:00 ` Simon Glass
2023-01-27 19:07 ` Tom Rini
1 sibling, 0 replies; 12+ messages in thread
From: Simon Glass @ 2023-01-13 18:00 UTC (permalink / raw)
To: Massimo Pegorer
Cc: u-boot, Heinrich Schuchardt, Jan Kiszka, Jessica Clarke,
Pali Rohár, Samuel Holland, Sean Anderson,
Stefan Eichenberger
On Thu, 5 Jan 2023 at 02:31, Massimo Pegorer <massimo.pegorer@vimar.com> wrote:
>
> Extend support for signing in auto-generated (-f auto) FIT. Previously,
> it was possible to get signed 'images' subnodes in the FIT using
> options -g and -o together with -f auto. This patch allows signing
> 'configurations' subnodes instead of 'images' ones (which are hashed),
> using option -f auto-conf instead of -f auto. Adding also -K <dtb> and
> -r options, will add public key to <dtb> file with required = "conf"
> property.
>
> Summary:
> -f auto => FIT with crc32 images
> -f auto -g ... -o ... => FIT with signed images
> -f auto-conf -g ... -o ... => FIT with sha1 images and signed confs
>
> Example: FIT with kernel, two device tree files, and signed
> configurations; public key (needed to verify signatures) is
> added to u-boot.dtb with required = "conf" property.
>
> mkimage -f auto-conf -A arm -O linux -T kernel -C none -a 43e00000 \
> -e 0 -d vmlinuz -b /path/to/first.dtb -b /path/to/second.dtb \
> -k /folder/with/key-files -g keyname -o sha256,rsa4096 \
> -K u-boot.dtb -r kernel.itb
>
> Example: Add public key with required = "conf" property to u-boot.dtb
> without needing to sign anything. This will also create a useless FIT
> named unused.itb.
>
> mkimage -f auto-conf -d /dev/null -k /folder/with/key-files \
> -g keyname -o sha256,rsa4096 -K u-boot.dtb -r unused.itb
>
> Signed-off-by: Massimo Pegorer <massimo.pegorer@vimar.com>
>
> ---
> The commit includes: patch for adding the new feature to mkimage tool;
> updated man page, with description of the new feature and examples,
> plus fixes to wrong/misleading information; test for all of the three
> flavours of auto-FIT (crc32 images, signed images, sha1 hashed images
> and signed configurations).
>
> doc/mkimage.1 | 119 +++++++++++-----
> test/py/tests/test_fit_auto_signed.py | 195 ++++++++++++++++++++++++++
> tools/fit_image.c | 75 ++++++----
> tools/imagetool.h | 10 +-
> tools/mkimage.c | 21 ++-
> 5 files changed, 353 insertions(+), 67 deletions(-)
> create mode 100644 test/py/tests/test_fit_auto_signed.py
Reviewed-by: Simon Glass <sjg@chromium.org>
We currently avoid using the fdt library in tools/dtoc in tests but
perhaps this policy needs to be changed, as this patch shows.
One option would be to create a new tools/u_boot_lib directory with
the shared functions currently in tools/patman etc., then allow use of
that in tests.
Regards,
Simon
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] mkimage: fit: Support signed configurations in 'auto' FITs
@ 2023-01-05 9:31 Massimo Pegorer
2023-01-13 18:00 ` Simon Glass
2023-01-27 19:07 ` Tom Rini
0 siblings, 2 replies; 12+ messages in thread
From: Massimo Pegorer @ 2023-01-05 9:31 UTC (permalink / raw)
To: u-boot
Cc: Massimo Pegorer, Heinrich Schuchardt, Jan Kiszka, Jessica Clarke,
Pali Rohár, Samuel Holland, Sean Anderson, Simon Glass,
Stefan Eichenberger
Extend support for signing in auto-generated (-f auto) FIT. Previously,
it was possible to get signed 'images' subnodes in the FIT using
options -g and -o together with -f auto. This patch allows signing
'configurations' subnodes instead of 'images' ones (which are hashed),
using option -f auto-conf instead of -f auto. Adding also -K <dtb> and
-r options, will add public key to <dtb> file with required = "conf"
property.
Summary:
-f auto => FIT with crc32 images
-f auto -g ... -o ... => FIT with signed images
-f auto-conf -g ... -o ... => FIT with sha1 images and signed confs
Example: FIT with kernel, two device tree files, and signed
configurations; public key (needed to verify signatures) is
added to u-boot.dtb with required = "conf" property.
mkimage -f auto-conf -A arm -O linux -T kernel -C none -a 43e00000 \
-e 0 -d vmlinuz -b /path/to/first.dtb -b /path/to/second.dtb \
-k /folder/with/key-files -g keyname -o sha256,rsa4096 \
-K u-boot.dtb -r kernel.itb
Example: Add public key with required = "conf" property to u-boot.dtb
without needing to sign anything. This will also create a useless FIT
named unused.itb.
mkimage -f auto-conf -d /dev/null -k /folder/with/key-files \
-g keyname -o sha256,rsa4096 -K u-boot.dtb -r unused.itb
Signed-off-by: Massimo Pegorer <massimo.pegorer@vimar.com>
---
The commit includes: patch for adding the new feature to mkimage tool;
updated man page, with description of the new feature and examples,
plus fixes to wrong/misleading information; test for all of the three
flavours of auto-FIT (crc32 images, signed images, sha1 hashed images
and signed configurations).
doc/mkimage.1 | 119 +++++++++++-----
test/py/tests/test_fit_auto_signed.py | 195 ++++++++++++++++++++++++++
tools/fit_image.c | 75 ++++++----
tools/imagetool.h | 10 +-
tools/mkimage.c | 21 ++-
5 files changed, 353 insertions(+), 67 deletions(-)
create mode 100644 test/py/tests/test_fit_auto_signed.py
diff --git a/doc/mkimage.1 b/doc/mkimage.1
index 353ea8b2f7..d8727ec73c 100644
--- a/doc/mkimage.1
+++ b/doc/mkimage.1
@@ -22,7 +22,8 @@ mkimage \- generate images for U-Boot
.SY mkimage
.RI [ option\~ .\|.\|.\&]
.BI \-f\~ image-tree-source-file\c
-.RB | auto
+.RB | auto\c
+.RB | auto-conf
.I image-file-name
.YS
.
@@ -296,9 +297,9 @@ FIT. See
for details on using external data.
.
.TP
-\fB\-f \fIimage-tree-source-file\fR | \fBauto
+\fB\-f \fIimage-tree-source-file\fR | \fBauto\fR | \fBauto-conf
.TQ
-\fB\-\-fit \fIimage-tree-source-file\fR | \fBauto
+\fB\-\-fit \fIimage-tree-source-file\fR | \fBauto\fR | \fBauto-conf
Image tree source file that describes the structure and contents of the
FIT image.
.IP
@@ -317,7 +318,25 @@ and
options may be used to specify the image to include in the FIT and its
attributes. No
.I image-tree-source-file
-is required.
+is required. The
+.BR \-g ,
+.BR \-o ,
+and
+.B \-k
+or
+.B \-G
+options may be used to get \(oqimages\(cq signed subnodes in the generated
+auto FIT. Instead, to get \(oqconfigurations\(cq signed subnodes and
+\(oqimages\(cq hashed subnodes, pass
+.BR "\-f auto-conf".
+In this case
+.BR \-g ,
+.BR \-o ,
+and
+.B \-k
+or
+.B \-G
+are mandatory options.
.
.TP
.B \-F
@@ -348,16 +367,16 @@ for use with signing, and a certificate
necessary when embedding it into another device tree using
.BR \-K .
.I name
-defaults to the value of the signature node's \(oqkey-name-hint\(cq property,
-but may be overridden using
-.BR \-g .
+is the value of the signature node's \(oqkey-name-hint\(cq property.
.
.TP
.BI \-G " key-file"
.TQ
.BI \-\-key\-file " key-file"
Specifies the private key file to use when signing. This option may be used
-instead of \-k.
+instead of \-k. Useful when the private key file basename does not match
+\(oqkey-name-hint\(cq value. But note that it may lead to unexpected results
+when used together with -K and/or -k options.
.
.TP
.BI \-K " key-destination"
@@ -373,49 +392,50 @@ CONFIG_OF_CONTROL in U-Boot.
.BI \-g " key-name-hint"
.TQ
.BI \-\-key\-name\-hint " key-name-hint"
-Overrides the signature node's \(oqkey-name-hint\(cq property. This is
-especially useful when signing an image with
-.BR "\-f auto" .
-This is the
-.I name
-part of the key. The directory part is set by
-.BR \-k .
-This option also indicates that the images included in the FIT should be signed.
-If this option is specified, then
+Specifies the value of signature node \(oqkey-name-hint\(cq property for
+an automatically generated FIT image. It makes sense only when used with
+.B "\-f auto"
+or
+.BR "\-f auto-conf".
+This option also indicates that the images or configurations included in
+the FIT should be signed. If this option is specified, then
.B \-o
must be specified as well.
.
.TP
-.BI \-o " crypto" , checksum
+.BI \-o " checksum" , crypto
.TQ
-.BI \-\-algo " crypto" , checksum
-Specifies the algorithm to be used for signing a FIT image. The default is
-taken from the signature node's \(oqalgo\(cq property.
+.BI \-\-algo " checksum" , crypto
+Specifies the algorithm to be used for signing a FIT image, overriding value
+taken from the signature node \(oqalgo\(cq property in the
+.IR image-tree-source-file .
+It is mandatory for automatically generated FIT.
+.IP
The valid values for
-.I crypto
+.I checksum
are:
.RS
.IP
.TS
lb.
-rsa2048
-rsa3072
-rsa4096
-ecdsa256
+sha1
+sha256
+sha384
+sha512
.TE
.RE
.IP
The valid values for
-.I checksum
-are
+.I crypto
+are:
.RS
.IP
.TS
lb.
-sha1
-sha256
-sha384
-sha512
+rsa2048
+rsa3072
+rsa4096
+ecdsa256
.TE
.RE
.
@@ -423,9 +443,13 @@ sha512
.B \-r
.TQ
.B \-\-key\-required
-Specifies that keys used to sign the FIT are required. This means that they
-must be verified for the image to boot. Without this option, the verification
-will be optional (useful for testing but not for release).
+Specifies that keys used to sign the FIT are required. This means that images
+or configurations signatures must be verified before using them (i.e. to
+boot). Without this option, the verification will be optional (useful for
+testing but not for release). It makes sense only when used with
+.BR \-K.
+When both, images and configurations, are signed, \(oqrequired\(cq property
+value will be "conf".
.
.TP
.BI \-N " engine"
@@ -716,7 +740,7 @@ skipping those for which keys cannot be found. Also add a comment.
.EE
.RE
.P
-Add public keys to u\-boot.dtb without needing a FIT to sign. This will also
+Add public key to u\-boot.dtb without needing a FIT to sign. This will also
create a FIT containing an images node with no data named unused.itb.
.RS
.P
@@ -726,6 +750,16 @@ create a FIT containing an images node with no data named unused.itb.
.EE
.RE
.P
+Add public key with required = "conf" property to u\-boot.dtb without needing
+a FIT to sign. This will also create a useless FIT named unused.itb.
+.RS
+.P
+.EX
+\fBmkimage \-f auto-conf \-d /dev/null \-k /public/signing\-keys \-g dev \\
+ \-o sha256,rsa2048 \-K u\-boot.dtb -r unused.itb
+.EE
+.RE
+.P
Update an existing FIT image, signing it with additional keys.
Add corresponding public keys into u\-boot.dtb. This will resign all images
with keys that are available in the new directory. Images that request signing
@@ -768,6 +802,19 @@ file is required.
\-d vmlinuz \-k /secret/signing\-keys \-g dev \-o sha256,rsa2048 kernel.itb
.EE
.RE
+.P
+Create a FIT image containing a kernel and some device tree files, signing
+each configuration, using automatic mode. Moreover, the public key needed to
+verify signatures is added to u\-boot.dtb with required = "conf" property.
+.RS
+.P
+.EX
+\fBmkimage \-f auto-conf \-A arm \-O linux \-T kernel \-C none \-a 43e00000 \\
+ \-e 0 \-d vmlinuz \-b /path/to/file\-1.dtb \-b /path/to/file\-2.dtb \\
+ \-k /folder/with/signing\-keys \-g dev \-o sha256,rsa2048 \\
+ \-K u\-boot.dtb -r kernel.itb
+.EE
+.RE
.
.SH SEE ALSO
.BR dtc (1),
diff --git a/test/py/tests/test_fit_auto_signed.py b/test/py/tests/test_fit_auto_signed.py
new file mode 100644
index 0000000000..9ea3351619
--- /dev/null
+++ b/test/py/tests/test_fit_auto_signed.py
@@ -0,0 +1,195 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (c) 2022 Massimo Pegorer
+
+"""
+Test that mkimage generates auto-FIT with signatures and/or hashes as expected.
+
+The mkimage tool can create auto generated (i.e. without an ITS file
+provided as input) FIT in three different flavours: with crc32 checksums
+of 'images' subnodes; with signatures of 'images' subnodes; with sha1
+hashes of 'images' subnodes and signatures of 'configurations' subnodes.
+This test verifies that auto-FIT are generated as expected, in all of
+the three flavours, including check of hashes and signatures (except for
+configurations ones).
+
+The test does not run the sandbox. It only checks the host tool mkimage.
+"""
+
+import os
+import pytest
+import u_boot_utils as util
+import binascii
+from Cryptodome.Hash import SHA1
+from Cryptodome.Hash import SHA256
+from Cryptodome.PublicKey import RSA
+from Cryptodome.Signature import pkcs1_15
+
+class SignedFitHelper(object):
+ """Helper to manipulate a FIT with signed/hashed images/configs."""
+ def __init__(self, cons, file_name):
+ self.fit = file_name
+ self.cons = cons
+ self.images_nodes = set()
+ self.confgs_nodes = set()
+
+ def __fdt_list(self, path):
+ return util.run_and_log(self.cons,
+ f'fdtget -l {self.fit} {path}')
+
+ def __fdt_get_string(self, node, prop):
+ return util.run_and_log(self.cons,
+ f'fdtget -ts {self.fit} {node} {prop}')
+
+ def __fdt_get_binary(self, node, prop):
+ numbers = util.run_and_log(self.cons,
+ f'fdtget -tbi {self.fit} {node} {prop}')
+
+ bignum = bytearray()
+ for little_num in numbers.split():
+ bignum.append(int(little_num))
+
+ return bignum
+
+ def build_nodes_sets(self):
+ """Fill sets with FIT images and configurations subnodes."""
+ for node in self.__fdt_list('/images').split():
+ subnode = f'/images/{node}'
+ self.images_nodes.add(subnode)
+
+ for node in self.__fdt_list('/configurations').split():
+ subnode = f'/configurations/{node}'
+ self.confgs_nodes.add(subnode)
+
+ return len(self.images_nodes) + len(self.confgs_nodes)
+
+ def check_fit_crc32_images(self):
+ """Test that all images in the set are hashed as expected.
+
+ Each image must have an hash with algo=crc32 and hash value must match
+ the one calculated over image data.
+ """
+ for node in self.images_nodes:
+ algo = self.__fdt_get_string(f'{node}/hash', 'algo')
+ assert algo == "crc32\n", "Missing expected crc32 image hash!"
+
+ raw_crc32 = self.__fdt_get_binary(f'{node}/hash', 'value')
+ raw_bin = self.__fdt_get_binary(node, 'data')
+ assert raw_crc32 == (binascii.crc32(raw_bin) &
+ 0xffffffff).to_bytes(4, 'big'), "Wrong crc32 hash!"
+
+ def check_fit_signed_images(self, key_name, sign_algo, verifier):
+ """Test that all images in the set are signed as expected.
+
+ Each image must have a signature with: key-name-hint matching key_name
+ argument; algo matching sign_algo argument; value matching the one
+ calculated over image data using verifier argument.
+ """
+ for node in self.images_nodes:
+ hint = self.__fdt_get_string(f'{node}/signature', 'key-name-hint')
+ assert hint == key_name + "\n", "Missing expected key name hint!"
+ algo = self.__fdt_get_string(f'{node}/signature', 'algo')
+ assert algo == sign_algo + "\n", "Missing expected signature algo!"
+
+ raw_sig = self.__fdt_get_binary(f'{node}/signature', 'value')
+ raw_bin = self.__fdt_get_binary(node, 'data')
+ verifier.verify(SHA256.new(raw_bin), bytes(raw_sig))
+
+ def check_fit_signed_confgs(self, key_name, sign_algo):
+ """Test that all configs are signed, and images hashed, as expected.
+
+ Each image must have an hash with algo=sha1 and hash value must match
+ the one calculated over image data. Each configuration must have a
+ signature with key-name-hint matching key_name argument and algo
+ matching sign_algo argument.
+ TODO: configurations signature checking.
+ """
+ for node in self.images_nodes:
+ algo = self.__fdt_get_string(f'{node}/hash', 'algo')
+ assert algo == "sha1\n", "Missing expected sha1 image hash!"
+
+ raw_hash = self.__fdt_get_binary(f'{node}/hash', 'value')
+ raw_bin = self.__fdt_get_binary(node, 'data')
+ assert raw_hash == SHA1.new(raw_bin).digest(), "Wrong sha1 hash!"
+
+ for node in self.confgs_nodes:
+ hint = self.__fdt_get_string(f'{node}/signature', 'key-name-hint')
+ assert hint == key_name + "\n", "Missing expected key name hint!"
+ algo = self.__fdt_get_string(f'{node}/signature', 'algo')
+ assert algo == sign_algo + "\n", "Missing expected signature algo!"
+
+
+@pytest.mark.buildconfigspec('fit_signature')
+@pytest.mark.requiredtool('fdtget')
+def test_fit_auto_signed(u_boot_console):
+ """Test that mkimage generates auto-FIT with signatures/hashes as expected.
+
+ The mkimage tool can create auto generated (i.e. without an ITS file
+ provided as input) FIT in three different flavours: with crc32 checksums
+ of 'images' subnodes; with signatures of 'images' subnodes; with sha1
+ hashes of 'images' subnodes and signatures of 'configurations' subnodes.
+ This test verifies that auto-FIT are generated as expected, in all of
+ the three flavours, including check of hashes and signatures (except for
+ configurations ones).
+
+ The test does not run the sandbox. It only checks the host tool mkimage.
+ """
+ cons = u_boot_console
+ mkimage = cons.config.build_dir + '/tools/mkimage'
+ tempdir = os.path.join(cons.config.result_dir, 'auto_fit')
+ os.makedirs(tempdir, exist_ok=True)
+ kernel_file = f'{tempdir}/vmlinuz'
+ dt1_file = f'{tempdir}/dt-1.dtb'
+ dt2_file = f'{tempdir}/dt-2.dtb'
+ key_name = 'sign-key'
+ sign_algo = 'sha256,rsa4096'
+ key_file = f'{tempdir}/{key_name}.key'
+ fit_file = f'{tempdir}/test.fit'
+
+ # Create a fake kernel image and two dtb files with random data
+ with open(kernel_file, 'wb') as fd:
+ fd.write(os.urandom(512))
+
+ with open(dt1_file, 'wb') as fd:
+ fd.write(os.urandom(256))
+
+ with open(dt2_file, 'wb') as fd:
+ fd.write(os.urandom(256))
+
+ # Create 4096 RSA key and write to file to be read by mkimage
+ key = RSA.generate(bits=4096)
+ verifier = pkcs1_15.new(key)
+
+ with open(key_file, 'w') as fd:
+ fd.write(str(key.export_key(format='PEM').decode('ascii')))
+
+ b_args = " -d" + kernel_file + " -b" + dt1_file + " -b" + dt2_file
+ s_args = " -k" + tempdir + " -g" + key_name + " -o" + sign_algo
+
+ # 1 - Create auto FIT with images crc32 checksum, and verify it
+ util.run_and_log(cons, mkimage + ' -fauto' + b_args + " " + fit_file)
+
+ fit = SignedFitHelper(cons, fit_file)
+ if fit.build_nodes_sets() == 0:
+ raise ValueError('FIT-1 has no "/image" nor "/configuration" nodes')
+
+ fit.check_fit_crc32_images()
+
+ # 2 - Create auto FIT with signed images, and verify it
+ util.run_and_log(cons, mkimage + ' -fauto' + b_args + s_args + " " +
+ fit_file)
+
+ fit = SignedFitHelper(cons, fit_file)
+ if fit.build_nodes_sets() == 0:
+ raise ValueError('FIT-2 has no "/image" nor "/configuration" nodes')
+
+ fit.check_fit_signed_images(key_name, sign_algo, verifier)
+
+ # 3 - Create auto FIT with signed configs and hashed images, and verify it
+ util.run_and_log(cons, mkimage + ' -fauto-conf' + b_args + s_args + " " +
+ fit_file)
+
+ fit = SignedFitHelper(cons, fit_file)
+ if fit.build_nodes_sets() == 0:
+ raise ValueError('FIT-3 has no "/image" nor "/configuration" nodes')
+
+ fit.check_fit_signed_confgs(key_name, sign_algo)
diff --git a/tools/fit_image.c b/tools/fit_image.c
index 923a9755b7..d15a377779 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -199,36 +199,59 @@ static void get_basename(char *str, int size, const char *fname)
}
/**
- * add_hash_node() - Add a hash or signature node
+ * fit_add_hash_or_sign() - Add a hash or signature node
*
* @params: Image parameters
* @fdt: Device tree to add to (in sequential-write mode)
+ * @is_images_subnode: 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.
- *
- * Return: 0 on success, or -1 on failure
+ * 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.
*/
-static int add_hash_node(struct image_tool_params *params, void *fdt)
+static void fit_add_hash_or_sign(struct image_tool_params *params, void *fdt,
+ bool is_images_subnode)
{
- if (params->keyname) {
- if (!params->algo_name) {
- fprintf(stderr,
- "%s: Algorithm name must be specified\n",
- params->cmdname);
- return -1;
+ const char *hash_algo = "crc32";
+ bool do_hash = false;
+ bool do_sign = false;
+
+ switch (params->auto_fit) {
+ case AF_OFF:
+ break;
+ case AF_HASHED_IMG:
+ do_hash = is_images_subnode;
+ break;
+ case AF_SIGNED_IMG:
+ do_sign = is_images_subnode;
+ break;
+ case AF_SIGNED_CONF:
+ if (is_images_subnode) {
+ do_hash = true;
+ hash_algo = "sha1";
+ } else {
+ do_sign = true;
}
+ break;
+ default:
+ fprintf(stderr,
+ "%s: Unsupported auto FIT mode %u\n",
+ params->cmdname, params->auto_fit);
+ break;
+ }
+
+ if (do_hash) {
+ fdt_begin_node(fdt, FIT_HASH_NODENAME);
+ fdt_property_string(fdt, FIT_ALGO_PROP, hash_algo);
+ fdt_end_node(fdt);
+ }
- fdt_begin_node(fdt, "signature-1");
+ if (do_sign) {
+ fdt_begin_node(fdt, FIT_SIG_NODENAME);
fdt_property_string(fdt, FIT_ALGO_PROP, params->algo_name);
fdt_property_string(fdt, FIT_KEY_HINT, params->keyname);
- } else {
- fdt_begin_node(fdt, "hash-1");
- fdt_property_string(fdt, FIT_ALGO_PROP, "crc32");
+ fdt_end_node(fdt);
}
-
- fdt_end_node(fdt);
- return 0;
}
/**
@@ -269,9 +292,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);
- if (ret)
- return ret;
+ fit_add_hash_or_sign(params, fdt, true);
fdt_end_node(fdt);
/* Now the device tree files if available */
@@ -294,7 +315,7 @@ 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);
+ fit_add_hash_or_sign(params, fdt, true);
if (ret)
return ret;
fdt_end_node(fdt);
@@ -314,7 +335,7 @@ 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);
+ fit_add_hash_or_sign(params, fdt, true);
if (ret)
return ret;
fdt_end_node(fdt);
@@ -366,6 +387,7 @@ 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);
+ fit_add_hash_or_sign(params, fdt, false);
fdt_end_node(fdt);
}
@@ -378,6 +400,7 @@ 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");
+ fit_add_hash_or_sign(params, fdt, false);
fdt_end_node(fdt);
}
@@ -721,7 +744,7 @@ static int fit_handle_file(struct image_tool_params *params)
sprintf (tmpfile, "%s%s", params->imagefile, MKIMAGE_TMPFILE_SUFFIX);
/* We either compile the source file, or use the existing FIT image */
- if (params->auto_its) {
+ if (params->auto_fit) {
if (fit_build(params, tmpfile)) {
fprintf(stderr, "%s: failed to build FIT\n",
params->cmdname);
@@ -905,7 +928,7 @@ static int fit_extract_contents(void *ptr, struct image_tool_params *params)
static int fit_check_params(struct image_tool_params *params)
{
- if (params->auto_its)
+ if (params->auto_fit)
return 0;
return ((params->dflag && params->fflag) ||
(params->fflag && params->lflag) ||
diff --git a/tools/imagetool.h b/tools/imagetool.h
index ca7c2e48ba..fdceea46c0 100644
--- a/tools/imagetool.h
+++ b/tools/imagetool.h
@@ -39,6 +39,14 @@ struct content_info {
const char *fname;
};
+/* FIT auto generation modes */
+enum af_mode {
+ AF_OFF = 0, /* Needs .its or existing FIT to be provided */
+ AF_HASHED_IMG, /* Auto FIT with crc32 hashed images subnodes */
+ AF_SIGNED_IMG, /* Auto FIT with signed images subnodes */
+ AF_SIGNED_CONF, /* Auto FIT with sha1 images and signed configs */
+};
+
/*
* This structure defines all such variables those are initialized by
* mkimage and dumpimage main core and need to be referred by image
@@ -79,7 +87,7 @@ struct image_tool_params {
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 */
- bool auto_its; /* Automatically create the .its file */
+ enum af_mode auto_fit; /* Automatically create the FIT */
int fit_image_type; /* Image type to put into the FIT */
char *fit_ramdisk; /* Ramdisk file to include */
struct content_info *content_head; /* List of files to include */
diff --git a/tools/mkimage.c b/tools/mkimage.c
index 30c6df7708..59f3f1597e 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -104,7 +104,7 @@ static void usage(const char *msg)
" -v ==> verbose\n",
params.cmdname);
fprintf(stderr,
- " %s [-D dtc_options] [-f fit-image.its|-f auto|-F] [-b <dtb> [-b <dtb>]] [-E] [-B size] [-i <ramdisk.cpio.gz>] fit-image\n"
+ " %s [-D dtc_options] [-f fit-image.its|-f auto|-f auto-conf|-F] [-b <dtb> [-b <dtb>]] [-E] [-B size] [-i <ramdisk.cpio.gz>] fit-image\n"
" <dtb> file is used with -f auto, it may occur multiple times.\n",
params.cmdname);
fprintf(stderr,
@@ -271,7 +271,10 @@ static void process_args(int argc, char **argv)
break;
case 'f':
datafile = optarg;
- params.auto_its = !strcmp(datafile, "auto");
+ if (!strcmp(datafile, "auto"))
+ params.auto_fit = AF_HASHED_IMG;
+ else if (!strcmp(datafile, "auto-conf"))
+ params.auto_fit = AF_SIGNED_CONF;
/* fallthrough */
case 'F':
/*
@@ -283,6 +286,7 @@ static void process_args(int argc, char **argv)
break;
case 'g':
params.keyname = optarg;
+ break;
case 'G':
params.keyfile = optarg;
break;
@@ -370,6 +374,15 @@ static void process_args(int argc, char **argv)
if (optind < argc)
params.imagefile = argv[optind];
+ if (params.auto_fit == AF_SIGNED_CONF) {
+ if (!params.keyname || !params.algo_name)
+ usage("Missing key/algo for auto-FIT with signed configs (use -g -o)");
+ } else if (params.auto_fit == AF_HASHED_IMG && params.keyname) {
+ params.auto_fit = AF_SIGNED_IMG;
+ if (!params.algo_name)
+ usage("Missing algorithm for auto-FIT with signed images (use -g)");
+ }
+
/*
* 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
@@ -377,8 +390,8 @@ static void process_args(int argc, char **argv)
*/
if (params.type == IH_TYPE_FLATDT) {
params.fit_image_type = type ? type : IH_TYPE_KERNEL;
- /* For auto_its, datafile is always 'auto' */
- if (!params.auto_its)
+ /* For auto-FIT, datafile has to be provided with -d */
+ if (!params.auto_fit)
params.datafile = datafile;
else if (!params.datafile)
usage("Missing data file for auto-FIT (use -d)");
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-01-27 19:16 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-19 18:00 Patch proposal - mkimage: fit: Support signed conf 'auto' FITs Pegorer Massimo
2022-11-23 2:09 ` Simon Glass
2022-11-24 7:32 ` R: " Pegorer Massimo
2022-11-28 15:45 ` Sean Anderson
2022-12-04 21:16 ` Simon Glass
2022-12-09 15:47 ` R: " Pegorer Massimo
2022-12-09 16:09 ` Pegorer Massimo
2022-12-11 14:54 ` [PATCH] mkimage: fit: Support signed configurations in " Pegorer Massimo
2022-12-15 21:16 ` Simon Glass
2023-01-05 9:31 Massimo Pegorer
2023-01-13 18:00 ` Simon Glass
2023-01-27 19:07 ` Tom Rini
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.