[PATCH 0/3] mkimage: allow to specify signing algorithm

[PATCH 0/3] mkimage: allow to specify signing algorithm

[Jan Kiszka]

Another step to decouple the FIT image specification from the actual
signing: With these changes, the signature nodes can leave out an algo
property, mkimage will initialize that as well while signing. This way,
in-tree FIT source files can be prepared for gaining signatures without
defining the key type or size upfront, forcing users to patch the code
to change that.

Patch 1 is preparatory for this, patch 2 a drive-by cleanup.

A better solution would actually be if the algorithm was derived from
the provided key. But the underlying crypto layer seems to be rather
unprepared for that.

Jan

Jan Kiszka (3):
  image-fit: Make string of algo parameter constant
  mkimage: Drop unused OPT_STRING constant
  mkimage: Allow to specify the signature algorithm on the command line

 boot/image-fit-sig.c |  2 +-
 boot/image-fit.c     |  8 +++----
 doc/mkimage.1        |  5 +++++
 include/image.h      |  5 +++--
 tools/fit_image.c    |  3 ++-
 tools/image-host.c   | 50 +++++++++++++++++++++++++-------------------
 tools/imagetool.h    |  1 +
 tools/mkimage.c      |  6 ++++--
 8 files changed, 49 insertions(+), 31 deletions(-)

-- 
2.31.1

[PATCH 1/3] image-fit: Make string of algo parameter constant

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

Modifications would be invalid.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 boot/image-fit-sig.c | 2 +-
 boot/image-fit.c     | 8 ++++----
 include/image.h      | 2 +-
 tools/image-host.c   | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/boot/image-fit-sig.c b/boot/image-fit-sig.c
index 63e5423c92..47d7633568 100644
--- a/boot/image-fit-sig.c
+++ b/boot/image-fit-sig.c
@@ -67,7 +67,7 @@ static int fit_image_setup_verify(struct image_sign_info *info,
 				  const void *fit, int noffset,
 				  int required_keynode, char **err_msgp)
 {
-	char *algo_name;
+	const char *algo_name;
 	const char *padding_name;
 
 	if (fdt_totalsize(fit) > CONFIG_VAL(FIT_SIGNATURE_MAX_SIZE)) {
diff --git a/boot/image-fit.c b/boot/image-fit.c
index 33b4a46028..3be6fe6eb9 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -191,7 +191,7 @@ static void fit_image_print_data(const void *fit, int noffset, const char *p,
 	const char *keyname;
 	uint8_t *value;
 	int value_len;
-	char *algo;
+	const char *algo;
 	const char *padding;
 	bool required;
 	int ret, i;
@@ -1063,11 +1063,11 @@ int fit_image_get_data_and_size(const void *fit, int noffset,
  *     0, on success
  *     -1, on failure
  */
-int fit_image_hash_get_algo(const void *fit, int noffset, char **algo)
+int fit_image_hash_get_algo(const void *fit, int noffset, const char **algo)
 {
 	int len;
 
-	*algo = (char *)fdt_getprop(fit, noffset, FIT_ALGO_PROP, &len);
+	*algo = (const char *)fdt_getprop(fit, noffset, FIT_ALGO_PROP, &len);
 	if (*algo == NULL) {
 		fit_get_debug(fit, noffset, FIT_ALGO_PROP, len);
 		return -1;
@@ -1265,7 +1265,7 @@ static int fit_image_check_hash(const void *fit, int noffset, const void *data,
 {
 	uint8_t value[FIT_MAX_HASH_LEN];
 	int value_len;
-	char *algo;
+	const char *algo;
 	uint8_t *fit_value;
 	int fit_value_len;
 	int ignore;
diff --git a/include/image.h b/include/image.h
index fd662e74b4..16ccc5b10f 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1011,7 +1011,7 @@ int fit_image_get_data_size_unciphered(const void *fit, int noffset,
 int fit_image_get_data_and_size(const void *fit, int noffset,
 				const void **data, size_t *size);
 
-int fit_image_hash_get_algo(const void *fit, int noffset, char **algo);
+int fit_image_hash_get_algo(const void *fit, int noffset, const char **algo);
 int fit_image_hash_get_value(const void *fit, int noffset, uint8_t **value,
 				int *value_len);
 
diff --git a/tools/image-host.c b/tools/image-host.c
index a6b0a94420..a027155f3c 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -63,7 +63,7 @@ static int fit_image_process_hash(void *fit, const char *image_name,
 	uint8_t value[FIT_MAX_HASH_LEN];
 	const char *node_name;
 	int value_len;
-	char *algo;
+	const char *algo;
 	int ret;
 
 	node_name = fit_get_name(fit, noffset, NULL);
@@ -160,7 +160,7 @@ static int fit_image_setup_sig(struct image_sign_info *info,
 		const char *engine_id)
 {
 	const char *node_name;
-	char *algo_name;
+	const char *algo_name;
 	const char *padding_name;
 
 	node_name = fit_get_name(fit, noffset, NULL);
-- 
2.31.1

[PATCH 2/3] mkimage: Drop unused OPT_STRING constant

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

The actual opt string is inlined - and different. Seems this was a
left-over from older versions of 603e26f76346.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 tools/mkimage.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/mkimage.c b/tools/mkimage.c
index fbe883ce36..a4844d0f18 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -146,7 +146,6 @@ static int add_content(int type, const char *fname)
 	return 0;
 }
 
-#define OPT_STRING "a:A:b:B:c:C:d:D:e:Ef:Fk:i:K:ln:N:p:O:rR:qstT:vVx"
 static void process_args(int argc, char **argv)
 {
 	char *ptr;
-- 
2.31.1

[PATCH 3/3] mkimage: Allow to specify the signature algorithm on the command line

[Jan Kiszka]

From: Jan Kiszka <jan.kiszka@siemens.com>

This permits to prepare FIT image description that do not hard-code the
final choice of the signature algorithm, possibly requiring the user to
patch the sources.

When -o <algo> is specified, this information is used in favor of the
'algo' property in the signature node. Furthermore, that property is set
accordingly when writing the image.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 doc/mkimage.1      |  5 +++++
 include/image.h    |  3 ++-
 tools/fit_image.c  |  3 ++-
 tools/image-host.c | 48 +++++++++++++++++++++++++++-------------------
 tools/imagetool.h  |  1 +
 tools/mkimage.c    |  5 ++++-
 6 files changed, 42 insertions(+), 23 deletions(-)

diff --git a/doc/mkimage.1 b/doc/mkimage.1
index fea5288784..0734bd36a1 100644
--- a/doc/mkimage.1
+++ b/doc/mkimage.1
@@ -155,6 +155,11 @@ the corresponding public key is written into this file for for run-time
 verification. Typically the file here is the device tree binary used by
 CONFIG_OF_CONTROL in U-Boot.
 
+.TP
+.BI "\-o [" "signing algorithm" "]"
+Specifies the algorithm to be used for signing a FIT image. The default is
+taken from the target signature nodes 'algo' properties.
+
 .TP
 .BI "\-p [" "external position" "]"
 Place external data at a static external position. See \-E. Instead of writing
diff --git a/include/image.h b/include/image.h
index 16ccc5b10f..4a7e9bc9a1 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1031,6 +1031,7 @@ int fit_cipher_data(const char *keydir, void *keydest, void *fit,
  * @require_keys: Mark all keys as 'required'
  * @engine_id:	Engine to use for signing
  * @cmdname:	Command name used when reporting errors
+ * @algo_name:	Algorithm name, or NULL if to be read from FIT
  *
  * Adds hash values for all component images in the FIT blob.
  * Hashes are calculated for all component images which have hash subnodes
@@ -1045,7 +1046,7 @@ int fit_cipher_data(const char *keydir, void *keydest, void *fit,
 int fit_add_verification_data(const char *keydir, const char *keyfile,
 			      void *keydest, void *fit, const char *comment,
 			      int require_keys, const char *engine_id,
-			      const char *cmdname);
+			      const char *cmdname, const char *algo_name);
 
 int fit_image_verify_with_data(const void *fit, int image_noffset,
 			       const void *data, size_t size);
diff --git a/tools/fit_image.c b/tools/fit_image.c
index f4f372ba62..428ddcf881 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -73,7 +73,8 @@ static int fit_add_file_data(struct image_tool_params *params, size_t size_inc,
 						params->comment,
 						params->require_keys,
 						params->engine_id,
-						params->cmdname);
+						params->cmdname,
+						params->algo_name);
 	}
 
 	if (dest_blob) {
diff --git a/tools/image-host.c b/tools/image-host.c
index a027155f3c..d2e67a06aa 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -107,7 +107,7 @@ static int fit_image_process_hash(void *fit, const char *image_name,
  */
 static int fit_image_write_sig(void *fit, int noffset, uint8_t *value,
 		int value_len, const char *comment, const char *region_prop,
-		int region_proplen, const char *cmdname)
+		int region_proplen, const char *cmdname, const char *algo_name)
 {
 	int string_size;
 	int ret;
@@ -150,6 +150,8 @@ static int fit_image_write_sig(void *fit, int noffset, uint8_t *value,
 					  strdata, sizeof(strdata));
 		}
 	}
+	if (algo_name && !ret)
+		ret = fdt_setprop_string(fit, noffset, "algo", algo_name);
 
 	return ret;
 }
@@ -157,17 +159,18 @@ static int fit_image_write_sig(void *fit, int noffset, uint8_t *value,
 static int fit_image_setup_sig(struct image_sign_info *info,
 		const char *keydir, const char *keyfile, void *fit,
 		const char *image_name, int noffset, const char *require_keys,
-		const char *engine_id)
+		const char *engine_id, const char *algo_name)
 {
 	const char *node_name;
-	const char *algo_name;
 	const char *padding_name;
 
 	node_name = fit_get_name(fit, noffset, NULL);
-	if (fit_image_hash_get_algo(fit, noffset, &algo_name)) {
-		printf("Can't get algo property for '%s' signature node in '%s' image node\n",
-		       node_name, image_name);
-		return -1;
+	if (!algo_name) {
+		if (fit_image_hash_get_algo(fit, noffset, &algo_name)) {
+			printf("Can't get algo property for '%s' signature node in '%s' image node\n",
+			       node_name, image_name);
+			return -1;
+		}
 	}
 
 	padding_name = fdt_getprop(fit, noffset, "padding", NULL);
@@ -215,7 +218,7 @@ static int fit_image_process_sig(const char *keydir, const char *keyfile,
 		void *keydest, void *fit, const char *image_name,
 		int noffset, const void *data, size_t size,
 		const char *comment, int require_keys, const char *engine_id,
-		const char *cmdname)
+		const char *cmdname, const char *algo_name)
 {
 	struct image_sign_info info;
 	struct image_region region;
@@ -226,7 +229,7 @@ static int fit_image_process_sig(const char *keydir, const char *keyfile,
 
 	if (fit_image_setup_sig(&info, keydir, keyfile, fit, image_name,
 				noffset, require_keys ? "image" : NULL,
-				engine_id))
+				engine_id, algo_name))
 		return -1;
 
 	node_name = fit_get_name(fit, noffset, NULL);
@@ -244,7 +247,7 @@ static int fit_image_process_sig(const char *keydir, const char *keyfile,
 	}
 
 	ret = fit_image_write_sig(fit, noffset, value, value_len, comment,
-			NULL, 0, cmdname);
+			NULL, 0, cmdname, algo_name);
 	if (ret) {
 		if (ret == -FDT_ERR_NOSPACE)
 			return -ENOSPC;
@@ -606,7 +609,7 @@ int fit_image_cipher_data(const char *keydir, void *keydest,
 int fit_image_add_verification_data(const char *keydir, const char *keyfile,
 		void *keydest, void *fit, int image_noffset,
 		const char *comment, int require_keys, const char *engine_id,
-		const char *cmdname)
+		const char *cmdname, const char* algo_name)
 {
 	const char *image_name;
 	const void *data;
@@ -643,7 +646,8 @@ int fit_image_add_verification_data(const char *keydir, const char *keyfile,
 				strlen(FIT_SIG_NODENAME))) {
 			ret = fit_image_process_sig(keydir, keyfile, keydest,
 				fit, image_name, noffset, data, size,
-				comment, require_keys, engine_id, cmdname);
+				comment, require_keys, engine_id, cmdname,
+				algo_name);
 		}
 		if (ret)
 			return ret;
@@ -927,7 +931,8 @@ static int fit_config_get_data(void *fit, int conf_noffset, int noffset,
 static int fit_config_process_sig(const char *keydir, const char *keyfile,
 		void *keydest,	void *fit, const char *conf_name,
 		int conf_noffset, int noffset, const char *comment,
-		int require_keys, const char *engine_id, const char *cmdname)
+		int require_keys, const char *engine_id, const char *cmdname,
+		const char *algo_name)
 {
 	struct image_sign_info info;
 	const char *node_name;
@@ -945,7 +950,8 @@ static int fit_config_process_sig(const char *keydir, const char *keyfile,
 		return -1;
 
 	if (fit_image_setup_sig(&info, keydir, keyfile, fit, conf_name, noffset,
-				require_keys ? "conf" : NULL, engine_id))
+				require_keys ? "conf" : NULL, engine_id,
+				algo_name))
 		return -1;
 
 	ret = info.crypto->sign(&info, region, region_count, &value,
@@ -962,7 +968,8 @@ static int fit_config_process_sig(const char *keydir, const char *keyfile,
 	}
 
 	ret = fit_image_write_sig(fit, noffset, value, value_len, comment,
-				region_prop, region_proplen, cmdname);
+				  region_prop, region_proplen, cmdname,
+				  algo_name);
 	if (ret) {
 		if (ret == -FDT_ERR_NOSPACE)
 			return -ENOSPC;
@@ -992,7 +999,7 @@ static int fit_config_process_sig(const char *keydir, const char *keyfile,
 static int fit_config_add_verification_data(const char *keydir,
 		const char *keyfile, void *keydest, void *fit, int conf_noffset,
 		const char *comment, int require_keys, const char *engine_id,
-		const char *cmdname)
+		const char *cmdname, const char *algo_name)
 {
 	const char *conf_name;
 	int noffset;
@@ -1011,7 +1018,7 @@ static int fit_config_add_verification_data(const char *keydir,
 			     strlen(FIT_SIG_NODENAME))) {
 			ret = fit_config_process_sig(keydir, keyfile, keydest,
 				fit, conf_name, conf_noffset, noffset, comment,
-				require_keys, engine_id, cmdname);
+				require_keys, engine_id, cmdname, algo_name);
 		}
 		if (ret)
 			return ret;
@@ -1058,7 +1065,7 @@ int fit_cipher_data(const char *keydir, void *keydest, void *fit,
 int fit_add_verification_data(const char *keydir, const char *keyfile,
 			      void *keydest, void *fit, const char *comment,
 			      int require_keys, const char *engine_id,
-			      const char *cmdname)
+			      const char *cmdname, const char *algo_name)
 {
 	int images_noffset, confs_noffset;
 	int noffset;
@@ -1082,7 +1089,7 @@ int fit_add_verification_data(const char *keydir, const char *keyfile,
 		 */
 		ret = fit_image_add_verification_data(keydir, keyfile, keydest,
 				fit, noffset, comment, require_keys, engine_id,
-				cmdname);
+				cmdname, algo_name);
 		if (ret)
 			return ret;
 	}
@@ -1106,7 +1113,8 @@ int fit_add_verification_data(const char *keydir, const char *keyfile,
 		ret = fit_config_add_verification_data(keydir, keyfile, keydest,
 						       fit, noffset, comment,
 						       require_keys,
-						       engine_id, cmdname);
+						       engine_id, cmdname,
+						       algo_name);
 		if (ret)
 			return ret;
 	}
diff --git a/tools/imagetool.h b/tools/imagetool.h
index e229a34ffc..d71027317f 100644
--- a/tools/imagetool.h
+++ b/tools/imagetool.h
@@ -69,6 +69,7 @@ struct image_tool_params {
 	const char *keydest;	/* Destination .dtb for public key */
 	const char *keyfile;	/* Filename of private or public key */
 	const char *comment;	/* Comment to add to signature node */
+	const char *algo_name;	/* Algorithm name to use hashing/signing */
 	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 */
diff --git a/tools/mkimage.c b/tools/mkimage.c
index a4844d0f18..ddb79331a6 100644
--- a/tools/mkimage.c
+++ b/tools/mkimage.c
@@ -154,7 +154,7 @@ static void process_args(int argc, char **argv)
 	int opt;
 
 	while ((opt = getopt(argc, argv,
-		   "a:A:b:B:c:C:d:D:e:Ef:FG:k:i:K:ln:N:p:O:rR:qstT:vVx")) != -1) {
+		   "a:A:b:B:c:C:d:D:e:Ef:FG:k:i:K:ln:N:p:o:O:rR:qstT:vVx")) != -1) {
 		switch (opt) {
 		case 'a':
 			params.addr = strtoull(optarg, &ptr, 16);
@@ -250,6 +250,9 @@ static void process_args(int argc, char **argv)
 		case 'N':
 			params.engine_id = optarg;
 			break;
+		case 'o':
+			params.algo_name = optarg;
+			break;
 		case 'O':
 			params.os = genimg_get_os_id(optarg);
 			if (params.os < 0) {
-- 
2.31.1

Re: [PATCH 0/3] mkimage: allow to specify signing algorithm

[Jan Kiszka]

On 25.11.21 20:03, Jan Kiszka wrote:
> Another step to decouple the FIT image specification from the actual
> signing: With these changes, the signature nodes can leave out an algo
> property, mkimage will initialize that as well while signing. This way,
> in-tree FIT source files can be prepared for gaining signatures without
> defining the key type or size upfront, forcing users to patch the code
> to change that.
> 
> Patch 1 is preparatory for this, patch 2 a drive-by cleanup.
> 
> A better solution would actually be if the algorithm was derived from
> the provided key. But the underlying crypto layer seems to be rather
> unprepared for that.
> 
> Jan
> 
> Jan Kiszka (3):
>    image-fit: Make string of algo parameter constant
>    mkimage: Drop unused OPT_STRING constant
>    mkimage: Allow to specify the signature algorithm on the command line
> 
>   boot/image-fit-sig.c |  2 +-
>   boot/image-fit.c     |  8 +++----
>   doc/mkimage.1        |  5 +++++
>   include/image.h      |  5 +++--
>   tools/fit_image.c    |  3 ++-
>   tools/image-host.c   | 50 +++++++++++++++++++++++++-------------------
>   tools/imagetool.h    |  1 +
>   tools/mkimage.c      |  6 ++++--
>   8 files changed, 49 insertions(+), 31 deletions(-)
> 

Ping on this series.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux

Re: [PATCH 0/3] mkimage: allow to specify signing algorithm

[Simon Glass]

Hi Jan,

On Thu, 13 Jan 2022 at 05:38, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 25.11.21 20:03, Jan Kiszka wrote:
> > Another step to decouple the FIT image specification from the actual
> > signing: With these changes, the signature nodes can leave out an algo
> > property, mkimage will initialize that as well while signing. This way,
> > in-tree FIT source files can be prepared for gaining signatures without
> > defining the key type or size upfront, forcing users to patch the code
> > to change that.
> >
> > Patch 1 is preparatory for this, patch 2 a drive-by cleanup.
> >
> > A better solution would actually be if the algorithm was derived from
> > the provided key. But the underlying crypto layer seems to be rather
> > unprepared for that.
> >
> > Jan
> >
> > Jan Kiszka (3):
> >    image-fit: Make string of algo parameter constant
> >    mkimage: Drop unused OPT_STRING constant
> >    mkimage: Allow to specify the signature algorithm on the command line
> >
> >   boot/image-fit-sig.c |  2 +-
> >   boot/image-fit.c     |  8 +++----
> >   doc/mkimage.1        |  5 +++++
> >   include/image.h      |  5 +++--
> >   tools/fit_image.c    |  3 ++-
> >   tools/image-host.c   | 50 +++++++++++++++++++++++++-------------------
> >   tools/imagetool.h    |  1 +
> >   tools/mkimage.c      |  6 ++++--
> >   8 files changed, 49 insertions(+), 31 deletions(-)
> >
>
> Ping on this series.
>
> Jan
>
> --
> Siemens AG, Technology
> Competence Center Embedded Linux

Somehow I did not get this. Can you please resend it?  I have lost
some patches to spam filters.

Regards,
Simon

Re: [PATCH 0/3] mkimage: allow to specify signing algorithm

[Dhananjay Phadke]

On 1/13/2022 4:38 AM, Jan Kiszka wrote:
> On 25.11.21 20:03, Jan Kiszka wrote:
>> Another step to decouple the FIT image specification from the actual
>> signing: With these changes, the signature nodes can leave out an algo
>> property, mkimage will initialize that as well while signing. This way,
>> in-tree FIT source files can be prepared for gaining signatures without
>> defining the key type or size upfront, forcing users to patch the code
>> to change that.

[resend to the list]

While encryption algo (rsa2048 vs rsa4096, etc) shouldn't need to be
explicitly specified as you noted below, how does it help to add it to
(already exhaustive) mkimage args? Parsing OID / length from keyfile
would be real change.

While rotating keys is common, how often algo is changed?

>>
>> Patch 1 is preparatory for this, patch 2 a drive-by cleanup.
>>
>> A better solution would actually be if the algorithm was derived from
>> the provided key. But the underlying crypto layer seems to be rather
>> unprepared for that.


Thanks,
DHananjay

Re: [PATCH 0/3] mkimage: allow to specify signing algorithm

[Jan Kiszka]

On 14.01.22 00:28, Dhananjay Phadke wrote:
> On 1/13/2022 4:38 AM, Jan Kiszka wrote:
>> On 25.11.21 20:03, Jan Kiszka wrote:
>>> Another step to decouple the FIT image specification from the actual
>>> signing: With these changes, the signature nodes can leave out an algo
>>> property, mkimage will initialize that as well while signing. This way,
>>> in-tree FIT source files can be prepared for gaining signatures without
>>> defining the key type or size upfront, forcing users to patch the code
>>> to change that.
> 
> [resend to the list]
> 
> While encryption algo (rsa2048 vs rsa4096, etc) shouldn't need to be
> explicitly specified as you noted below, how does it help to add it to
> (already exhaustive) mkimage args? Parsing OID / length from keyfile
> would be real change.
> 
> While rotating keys is common, how often algo is changed?
> 

The use case is decoupling of signature preparation from actual signing. 
Means, we would like to prepare binman image device trees for applying 
signature but without making the decision which key type and width 
should be used. That shall be up to the user (integrator) of the 
firmware image.

This series will enable a patch like 
https://github.com/siemens/u-boot/commit/1215a357ec1f58acde56c8ca4085aa43f8fdbf5a 
and a signature workflow that does not require to patch the U-Boot 
source anymore. It still makes the decision of using hash algo SHA256, 
but that is acceptable.

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux