
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