Patch proposal - mkimage: fit: Support signed conf 'auto' FITs
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
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;elseusage("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
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 bespecified\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' FITwith 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;elseusage("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
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"?
elseusage("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
()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"?
elseusage("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")
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
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 bespecified\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' FITwith 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.
elseusage("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
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)");
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
participants (3)
-
Pegorer Massimo -
Sean Anderson -
Simon Glass