[PATCH RFC 00/10] image: Reduce the abuse of #ifdefs in image-sig.c

[PATCH RFC 00/10] image: Reduce the abuse of #ifdefs in image-sig.c

[Alexandru Gagniuc]

This series is motivated by Simon's bratwurst:
   [PATCH v2 00/50] image: Reduce #ifdefs and ad-hoc defines in image code


A big problem with current mkimage code, as well as the code it shares
with the targets is that it uses a lot of #ifdefs. Some of the #ifdefs
are defined based on other macros, or CONFIG_() options.

Simon's approach to fixing this is to extend Kconfig to the host-side
of mkimage, and replace ifdefs with "if (CONFIG_IS_ENABLED())"
wherever possible. This would resolve most cosmetic issues caused by
the ungodly abuse of #ifdefs.

I do not like the aforementioned approach, because I believe it is a
band-aid to a much deeper problem. I believe the fundamental problem
is the incorrect separation of target code and host code, which has
led to the accumulation of crud over the years. I believe in
refactoring the current code in order to reduce the need for decision
points and branch divergence between the host and target.

In this series I intend to demonstrate a proof-of-concept for achieving
this with respect to signing algorithms. I treat the three
image_get_*_algo() functions as an interface, and decouple the host and
target implementations. This enable a dramatic reduction of #ifdefs
decision points in image-sig.c

The existing implementation is mostly suited for the host-side, where
it is reused. On the target-side, I implement a linker-list based
array of crypto_algo structures, inspired by the DM driver lists.

Two macros are deleted, rsa.h, and ecdsa.h are completely cleaned of
#ifdefs, and the new host-side implementation of image-sig.c has
zero #ifdefs. This comes at a minimal increase in the noumber of source
lines of code.

Only image_get_crypto_algo() is implemented as a linker list in this
POC. image_get_checksum_algo() and image_get_padding_algo() would also
see healthy benefits.

Alexandru Gagniuc (10):
  common: Move host-only logic in image-sig.c to separate file
  common: image-sig.c: Remove host-specific logic and #ifdefs
  image: Add support for placing crypto_algo in linker lists
  [UNTESTED] image: Add support for relocating crypto_algos in linker
    lists
  image: rsa: Move verification algorithm to a linker list
  image: image-sig.c: Remove crypto_algos array
  lib: ecdsa: Remove #ifdefs from ecdsa.h
  lib: rsa: Remove #ifdefs from rsa.h
  image: Eliminate IMAGE_ENABLE_VERIFY macro
  image: Eliminate IMAGE_ENABLE_VERIFY_ECDSA macro

 common/image-sig-host.c | 134 ++++++++++++++++++++++++++++++++++++++++
 common/image-sig.c      |  71 +++++----------------
 include/image.h         |  13 ++--
 include/u-boot/ecdsa.h  |  25 --------
 include/u-boot/rsa.h    |  47 --------------
 lib/rsa/rsa-verify.c    |  16 +++++
 tools/Makefile          |   2 +-
 7 files changed, 172 insertions(+), 136 deletions(-)
 create mode 100644 common/image-sig-host.c

-- 
2.31.1

[PATCH RFC 01/10] common: Move host-only logic in image-sig.c to separate file

[Alexandru Gagniuc]

image-sig.c is used to map a hash or crypto algorithm name to a
handler of that algorithm. There is some similarity between the host
and target variants, with the differences worked out by #ifdefs. The
purpose of this change is to remove those ifdefs.

First, copy the file to a host-only version, and remove target
specific code. Although it looks like we are duplicating code,
subsequent patches will change the way target algorithms are searched.
Besides we are only duplicating three string to struct mapping
functions. This isn't something to fuss about.
---
 common/image-sig-host.c | 134 ++++++++++++++++++++++++++++++++++++++++
 tools/Makefile          |   2 +-
 2 files changed, 135 insertions(+), 1 deletion(-)
 create mode 100644 common/image-sig-host.c

diff --git a/common/image-sig-host.c b/common/image-sig-host.c
new file mode 100644
index 0000000000..22e9c53c3e
--- /dev/null
+++ b/common/image-sig-host.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2013, Google Inc.
+ */
+
+#include "mkimage.h"
+#include <fdt_support.h>
+#include <time.h>
+#include <linux/libfdt.h>
+#include <image.h>
+#include <u-boot/ecdsa.h>
+#include <u-boot/rsa.h>
+#include <u-boot/hash-checksum.h>
+
+
+struct checksum_algo checksum_algos[] = {
+	{
+		.name = "sha1",
+		.checksum_len = SHA1_SUM_LEN,
+		.der_len = SHA1_DER_LEN,
+		.der_prefix = sha1_der_prefix,
+		.calculate_sign = EVP_sha1,
+		.calculate = hash_calculate,
+	},
+	{
+		.name = "sha256",
+		.checksum_len = SHA256_SUM_LEN,
+		.der_len = SHA256_DER_LEN,
+		.der_prefix = sha256_der_prefix,
+		.calculate_sign = EVP_sha256,
+		.calculate = hash_calculate,
+	},
+	{
+		.name = "sha384",
+		.checksum_len = SHA384_SUM_LEN,
+		.der_len = SHA384_DER_LEN,
+		.der_prefix = sha384_der_prefix,
+		.calculate_sign = EVP_sha384,
+		.calculate = hash_calculate,
+	},
+	{
+		.name = "sha512",
+		.checksum_len = SHA512_SUM_LEN,
+		.der_len = SHA512_DER_LEN,
+		.der_prefix = sha512_der_prefix,
+		.calculate_sign = EVP_sha512,
+		.calculate = hash_calculate,
+	},
+};
+
+struct crypto_algo crypto_algos[] = {
+	{
+		.name = "rsa2048",
+		.key_len = RSA2048_BYTES,
+		.sign = rsa_sign,
+		.add_verify_data = rsa_add_verify_data,
+		.verify = rsa_verify,
+	},
+	{
+		.name = "rsa4096",
+		.key_len = RSA4096_BYTES,
+		.sign = rsa_sign,
+		.add_verify_data = rsa_add_verify_data,
+		.verify = rsa_verify,
+	},
+	{
+		.name = "ecdsa256",
+		.key_len = ECDSA256_BYTES,
+		.sign = ecdsa_sign,
+		.add_verify_data = ecdsa_add_verify_data,
+		.verify = ecdsa_verify,
+	},
+};
+
+struct padding_algo padding_algos[] = {
+	{
+		.name = "pkcs-1.5",
+		.verify = padding_pkcs_15_verify,
+	},
+	{
+		.name = "pss",
+		.verify = padding_pss_verify,
+	}
+};
+
+struct checksum_algo *image_get_checksum_algo(const char *full_name)
+{
+	int i;
+	const char *name;
+
+	for (i = 0; i < ARRAY_SIZE(checksum_algos); i++) {
+		name = checksum_algos[i].name;
+		/* Make sure names match and next char is a comma */
+		if (!strncmp(name, full_name, strlen(name)) &&
+		    full_name[strlen(name)] == ',')
+			return &checksum_algos[i];
+	}
+
+	return NULL;
+}
+
+struct crypto_algo *image_get_crypto_algo(const char *full_name)
+{
+	int i;
+	const char *name;
+
+	/* Move name to after the comma */
+	name = strchr(full_name, ',');
+	if (!name)
+		return NULL;
+	name += 1;
+
+	for (i = 0; i < ARRAY_SIZE(crypto_algos); i++) {
+		if (!strcmp(crypto_algos[i].name, name))
+			return &crypto_algos[i];
+	}
+
+	return NULL;
+}
+
+struct padding_algo *image_get_padding_algo(const char *name)
+{
+	int i;
+
+	if (!name)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(padding_algos); i++) {
+		if (!strcmp(padding_algos[i].name, name))
+			return &padding_algos[i];
+	}
+
+	return NULL;
+}
diff --git a/tools/Makefile b/tools/Makefile
index d020c55d66..6751d9874b 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -58,7 +58,7 @@ hostprogs-$(CONFIG_FIT_SIGNATURE) += fit_info fit_check_sign
 hostprogs-$(CONFIG_CMD_BOOTEFI_SELFTEST) += file2include
 
 FIT_OBJS-$(CONFIG_FIT) := fit_common.o fit_image.o image-host.o common/image-fit.o
-FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig.o common/image-fit-sig.o
+FIT_SIG_OBJS-$(CONFIG_FIT_SIGNATURE) := common/image-sig-host.o common/image-fit-sig.o
 FIT_CIPHER_OBJS-$(CONFIG_FIT_CIPHER) := common/image-cipher.o
 
 # The following files are synced with upstream DTC.
-- 
2.31.1

[PATCH RFC 02/10] common: image-sig.c: Remove host-specific logic and #ifdefs

[Alexandru Gagniuc]

Remove any ifdefs in image-sig.c that were previously used to
differentiate from the host code. Note that all code dedicated to
relocating ->sign() and ->add_verify_data)_ can be safely removed,
as signing is not supported target-side.

NOTE that although it appears we are removing ecdsa256 support, this
is intentional. ecdsa_verify() is a no-op on the target, and is
currently only used by host code.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/image-sig.c | 39 ++-------------------------------------
 1 file changed, 2 insertions(+), 37 deletions(-)

diff --git a/common/image-sig.c b/common/image-sig.c
index 0f8e592aba..698b044d50 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -3,18 +3,11 @@
  * Copyright (c) 2013, Google Inc.
  */
 
-#ifdef USE_HOSTCC
-#include "mkimage.h"
-#include <fdt_support.h>
-#include <time.h>
-#include <linux/libfdt.h>
-#else
 #include <common.h>
 #include <log.h>
 #include <malloc.h>
 #include <asm/global_data.h>
 DECLARE_GLOBAL_DATA_PTR;
-#endif /* !USE_HOSTCC*/
 #include <image.h>
 #include <u-boot/ecdsa.h>
 #include <u-boot/rsa.h>
@@ -28,9 +21,6 @@ struct checksum_algo checksum_algos[] = {
 		.checksum_len = SHA1_SUM_LEN,
 		.der_len = SHA1_DER_LEN,
 		.der_prefix = sha1_der_prefix,
-#if IMAGE_ENABLE_SIGN
-		.calculate_sign = EVP_sha1,
-#endif
 		.calculate = hash_calculate,
 	},
 	{
@@ -38,9 +28,6 @@ struct checksum_algo checksum_algos[] = {
 		.checksum_len = SHA256_SUM_LEN,
 		.der_len = SHA256_DER_LEN,
 		.der_prefix = sha256_der_prefix,
-#if IMAGE_ENABLE_SIGN
-		.calculate_sign = EVP_sha256,
-#endif
 		.calculate = hash_calculate,
 	},
 #ifdef CONFIG_SHA384
@@ -49,9 +36,6 @@ struct checksum_algo checksum_algos[] = {
 		.checksum_len = SHA384_SUM_LEN,
 		.der_len = SHA384_DER_LEN,
 		.der_prefix = sha384_der_prefix,
-#if IMAGE_ENABLE_SIGN
-		.calculate_sign = EVP_sha384,
-#endif
 		.calculate = hash_calculate,
 	},
 #endif
@@ -61,9 +45,6 @@ struct checksum_algo checksum_algos[] = {
 		.checksum_len = SHA512_SUM_LEN,
 		.der_len = SHA512_DER_LEN,
 		.der_prefix = sha512_der_prefix,
-#if IMAGE_ENABLE_SIGN
-		.calculate_sign = EVP_sha512,
-#endif
 		.calculate = hash_calculate,
 	},
 #endif
@@ -74,24 +55,13 @@ struct crypto_algo crypto_algos[] = {
 	{
 		.name = "rsa2048",
 		.key_len = RSA2048_BYTES,
-		.sign = rsa_sign,
-		.add_verify_data = rsa_add_verify_data,
 		.verify = rsa_verify,
 	},
 	{
 		.name = "rsa4096",
 		.key_len = RSA4096_BYTES,
-		.sign = rsa_sign,
-		.add_verify_data = rsa_add_verify_data,
 		.verify = rsa_verify,
 	},
-	{
-		.name = "ecdsa256",
-		.key_len = ECDSA256_BYTES,
-		.sign = ecdsa_sign,
-		.add_verify_data = ecdsa_add_verify_data,
-		.verify = ecdsa_verify,
-	},
 };
 
 struct padding_algo padding_algos[] = {
@@ -112,16 +82,13 @@ struct checksum_algo *image_get_checksum_algo(const char *full_name)
 	int i;
 	const char *name;
 
-#if !defined(USE_HOSTCC) && defined(CONFIG_NEEDS_MANUAL_RELOC)
+#if defined(CONFIG_NEEDS_MANUAL_RELOC)
 	static bool done;
 
 	if (!done) {
 		done = true;
 		for (i = 0; i < ARRAY_SIZE(checksum_algos); i++) {
 			checksum_algos[i].name += gd->reloc_off;
-#if IMAGE_ENABLE_SIGN
-			checksum_algos[i].calculate_sign += gd->reloc_off;
-#endif
 			checksum_algos[i].calculate += gd->reloc_off;
 		}
 	}
@@ -143,15 +110,13 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name)
 	int i;
 	const char *name;
 
-#if !defined(USE_HOSTCC) && defined(CONFIG_NEEDS_MANUAL_RELOC)
+#if defined(CONFIG_NEEDS_MANUAL_RELOC)
 	static bool done;
 
 	if (!done) {
 		done = true;
 		for (i = 0; i < ARRAY_SIZE(crypto_algos); i++) {
 			crypto_algos[i].name += gd->reloc_off;
-			crypto_algos[i].sign += gd->reloc_off;
-			crypto_algos[i].add_verify_data += gd->reloc_off;
 			crypto_algos[i].verify += gd->reloc_off;
 		}
 	}
-- 
2.31.1

[PATCH RFC 03/10] image: Add support for placing crypto_algo in linker lists

[Alexandru Gagniuc]

The purpose of this change is to enable crypto algorithms to be placed
in linker lists, rather than be declared as a static array. The goal
is to remove the crypto_algos array in a subsequent patch.

Create a new linker list named "cryptos", and search it when
image_get_crypto_algo() is invoked.

NOTE that adding support for manual relocation of crypto_algos within
linker lists is beyond the scope of this patch.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/image-sig.c | 9 +++++++++
 include/image.h    | 5 +++++
 2 files changed, 14 insertions(+)

diff --git a/common/image-sig.c b/common/image-sig.c
index 698b044d50..2b7e23f99a 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -107,6 +107,7 @@ struct checksum_algo *image_get_checksum_algo(const char *full_name)
 
 struct crypto_algo *image_get_crypto_algo(const char *full_name)
 {
+	struct crypto_algo *crypto, *end;
 	int i;
 	const char *name;
 
@@ -133,6 +134,14 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name)
 			return &crypto_algos[i];
 	}
 
+	crypto = ll_entry_start(struct crypto_algo, cryptos);
+	end = ll_entry_end(struct crypto_algo, cryptos);
+	for (; crypto < end; crypto++) {
+		if (!strcmp(crypto->name, name))
+			return crypto;
+	}
+
+	/* Not found */
 	return NULL;
 }
 
diff --git a/include/image.h b/include/image.h
index 459685d4d4..1bda4ef725 100644
--- a/include/image.h
+++ b/include/image.h
@@ -47,6 +47,7 @@ struct fdt_region;
 #include <lmb.h>
 #include <asm/u-boot.h>
 #include <command.h>
+#include <linker_lists.h>
 
 /* Take notice of the 'ignore' property for hashes */
 #define IMAGE_ENABLE_IGNORE	1
@@ -1362,6 +1363,10 @@ struct crypto_algo {
 		      uint8_t *sig, uint sig_len);
 };
 
+/* Declare a new U-Boot crypto algorithm handler */
+#define U_BOOT_CRYPTO_ALGO(__name)						\
+ll_entry_declare(struct crypto_algo, __name, cryptos)
+
 struct padding_algo {
 	const char *name;
 	int (*verify)(struct image_sign_info *info,
-- 
2.31.1

[PATCH RFC 04/10] [UNTESTED] image: Add support for relocating crypto_algos in linker lists

[Alexandru Gagniuc]

Function pointers from crypto_algos array are relocated, when
NEEDS_MANUAL_RELOC is set. This relocation doesn't happen if the algo
is placed in a linker list. Implement this relocation.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/image-sig.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/common/image-sig.c b/common/image-sig.c
index 2b7e23f99a..b750751144 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -120,6 +120,13 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name)
 			crypto_algos[i].name += gd->reloc_off;
 			crypto_algos[i].verify += gd->reloc_off;
 		}
+
+		crypto = ll_entry_start(struct crypto_algo, cryptos);
+		end = ll_entry_end(struct crypto_algo, cryptos);
+		for (; crypto < end; crypto++) {
+			crypto->name += gd->reloc_off;
+			crypto->verify += gd->reloc_off;
+		}
 	}
 #endif
 
-- 
2.31.1

[PATCH RFC 05/10] image: rsa: Move verification algorithm to a linker list

[Alexandru Gagniuc]

Move the RSA verification crytpo_algo structure out of the
crypto_algos array, and into a linker list.

Although it appears we are adding an #ifdef to rsa-verify.c, the gains
outweigh this small inconvenience. This is because rsa_verify() is
defined differently based on #ifdefs. This change allows us to have
a single definition of rsa_verify().

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/image-sig.c   |  9 ---------
 lib/rsa/rsa-verify.c | 16 ++++++++++++++++
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/common/image-sig.c b/common/image-sig.c
index b750751144..d3be30289c 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -53,15 +53,6 @@ struct checksum_algo checksum_algos[] = {
 
 struct crypto_algo crypto_algos[] = {
 	{
-		.name = "rsa2048",
-		.key_len = RSA2048_BYTES,
-		.verify = rsa_verify,
-	},
-	{
-		.name = "rsa4096",
-		.key_len = RSA4096_BYTES,
-		.verify = rsa_verify,
-	},
 };
 
 struct padding_algo padding_algos[] = {
diff --git a/lib/rsa/rsa-verify.c b/lib/rsa/rsa-verify.c
index aee76f42d5..06b0d82e7d 100644
--- a/lib/rsa/rsa-verify.c
+++ b/lib/rsa/rsa-verify.c
@@ -571,3 +571,19 @@ int rsa_verify(struct image_sign_info *info,
 
 	return rsa_verify_hash(info, hash, sig, sig_len);
 }
+
+#ifndef USE_HOSTCC
+
+U_BOOT_CRYPTO_ALGO(rsa2048) = {
+	.name = "rsa2048",
+	.key_len = RSA2048_BYTES,
+	.verify = rsa_verify,
+};
+
+U_BOOT_CRYPTO_ALGO(rsa4096) = {
+	.name = "rsa4096",
+	.key_len = RSA4096_BYTES,
+	.verify = rsa_verify,
+};
+
+#endif
-- 
2.31.1

[PATCH RFC 06/10] image: image-sig.c: Remove crypto_algos array

[Alexandru Gagniuc]

Crytographic algorithms (currently RSA), are stored in linker lists.
The crypto_algos array is unused, so remove it, and any logic
associated with it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 common/image-sig.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/common/image-sig.c b/common/image-sig.c
index d3be30289c..6923f0a9e9 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -51,10 +51,6 @@ struct checksum_algo checksum_algos[] = {
 
 };
 
-struct crypto_algo crypto_algos[] = {
-	{
-};
-
 struct padding_algo padding_algos[] = {
 	{
 		.name = "pkcs-1.5",
@@ -107,10 +103,6 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name)
 
 	if (!done) {
 		done = true;
-		for (i = 0; i < ARRAY_SIZE(crypto_algos); i++) {
-			crypto_algos[i].name += gd->reloc_off;
-			crypto_algos[i].verify += gd->reloc_off;
-		}
 
 		crypto = ll_entry_start(struct crypto_algo, cryptos);
 		end = ll_entry_end(struct crypto_algo, cryptos);
@@ -127,11 +119,6 @@ struct crypto_algo *image_get_crypto_algo(const char *full_name)
 		return NULL;
 	name += 1;
 
-	for (i = 0; i < ARRAY_SIZE(crypto_algos); i++) {
-		if (!strcmp(crypto_algos[i].name, name))
-			return &crypto_algos[i];
-	}
-
 	crypto = ll_entry_start(struct crypto_algo, cryptos);
 	end = ll_entry_end(struct crypto_algo, cryptos);
 	for (; crypto < end; crypto++) {
-- 
2.31.1

[PATCH RFC 07/10] lib: ecdsa: Remove #ifdefs from ecdsa.h

[Alexandru Gagniuc]

It is no longer necessary to implement ecdsa_() functions as no-ops
depending on config options. It is merely sufficient to provide the
prototypes, as the ecdsa code is no longer linked when unused.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 include/u-boot/ecdsa.h | 25 -------------------------
 1 file changed, 25 deletions(-)

diff --git a/include/u-boot/ecdsa.h b/include/u-boot/ecdsa.h
index 979690d966..f6951c7346 100644
--- a/include/u-boot/ecdsa.h
+++ b/include/u-boot/ecdsa.h
@@ -15,7 +15,6 @@
  * @see "struct crypto_algo"
  * @{
  */
-#if IMAGE_ENABLE_SIGN
 /**
  * sign() - calculate and return signature for given input data
  *
@@ -49,22 +48,7 @@ int ecdsa_sign(struct image_sign_info *info, const struct image_region region[],
  * other -ve value on error
  */
 int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest);
-#else
-static inline
-int ecdsa_sign(struct image_sign_info *info, const struct image_region region[],
-	       int region_count, uint8_t **sigp, uint *sig_len)
-{
-	return -ENXIO;
-}
-
-static inline
-int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest)
-{
-	return -ENXIO;
-}
-#endif
 
-#if IMAGE_ENABLE_VERIFY_ECDSA
 /**
  * verify() - Verify a signature against some data
  *
@@ -78,15 +62,6 @@ int ecdsa_add_verify_data(struct image_sign_info *info, void *keydest)
 int ecdsa_verify(struct image_sign_info *info,
 		 const struct image_region region[], int region_count,
 		 uint8_t *sig, uint sig_len);
-#else
-static inline
-int ecdsa_verify(struct image_sign_info *info,
-		 const struct image_region region[], int region_count,
-		 uint8_t *sig, uint sig_len)
-{
-	return -ENXIO;
-}
-#endif
 /** @} */
 
 #define ECDSA256_BYTES	(256 / 8)
-- 
2.31.1

[PATCH RFC 08/10] lib: rsa: Remove #ifdefs from rsa.h

[Alexandru Gagniuc]

It is no longer necessary to implement rsa_() functions as no-ops
depending on config options. It is merely sufficient to provide the
prototypes, as the rsa code is no longer linked when unused.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 include/u-boot/rsa.h | 47 --------------------------------------------
 1 file changed, 47 deletions(-)

diff --git a/include/u-boot/rsa.h b/include/u-boot/rsa.h
index bed1c097c2..4ac373cd5e 100644
--- a/include/u-boot/rsa.h
+++ b/include/u-boot/rsa.h
@@ -31,7 +31,6 @@ struct rsa_public_key {
 
 struct image_sign_info;
 
-#if IMAGE_ENABLE_SIGN
 /**
  * sign() - calculate and return signature for given input data
  *
@@ -66,22 +65,7 @@ int rsa_sign(struct image_sign_info *info,
 		other -ve value on error
 */
 int rsa_add_verify_data(struct image_sign_info *info, void *keydest);
-#else
-static inline int rsa_sign(struct image_sign_info *info,
-		const struct image_region region[], int region_count,
-		uint8_t **sigp, uint *sig_len)
-{
-	return -ENXIO;
-}
-
-static inline int rsa_add_verify_data(struct image_sign_info *info,
-				      void *keydest)
-{
-	return -ENXIO;
-}
-#endif
 
-#if IMAGE_ENABLE_VERIFY
 /**
  * rsa_verify_hash() - Verify a signature against a hash
  *
@@ -124,37 +108,6 @@ int padding_pss_verify(struct image_sign_info *info,
 		       uint8_t *msg, int msg_len,
 		       const uint8_t *hash, int hash_len);
 #endif /* CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT */
-#else
-static inline int rsa_verify_hash(struct image_sign_info *info,
-				  const uint8_t *hash,
-				  uint8_t *sig, uint sig_len)
-{
-	return -ENXIO;
-}
-
-static inline int rsa_verify(struct image_sign_info *info,
-		const struct image_region region[], int region_count,
-		uint8_t *sig, uint sig_len)
-{
-	return -ENXIO;
-}
-
-static inline int padding_pkcs_15_verify(struct image_sign_info *info,
-					 uint8_t *msg, int msg_len,
-					 const uint8_t *hash, int hash_len)
-{
-	return -ENXIO;
-}
-
-#ifdef CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT
-static inline int padding_pss_verify(struct image_sign_info *info,
-				     uint8_t *msg, int msg_len,
-				     const uint8_t *hash, int hash_len)
-{
-	return -ENXIO;
-}
-#endif /* CONFIG_FIT_ENABLE_RSASSA_PSS_SUPPORT */
-#endif
 
 #define RSA_DEFAULT_PADDING_NAME		"pkcs-1.5"
 
-- 
2.31.1

[PATCH RFC 09/10] image: Eliminate IMAGE_ENABLE_VERIFY macro

[Alexandru Gagniuc]

This macro is no longer needed for code flow or #ifdefs. Remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 include/image.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/include/image.h b/include/image.h
index 1bda4ef725..4a7aa9358f 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1225,19 +1225,16 @@ int calculate_hash(const void *data, int data_len, const char *algo,
 #if defined(USE_HOSTCC)
 # if defined(CONFIG_FIT_SIGNATURE)
 #  define IMAGE_ENABLE_SIGN	1
-#  define IMAGE_ENABLE_VERIFY	1
 #  define IMAGE_ENABLE_VERIFY_ECDSA	1
 #  define FIT_IMAGE_ENABLE_VERIFY	1
 #  include <openssl/evp.h>
 # else
 #  define IMAGE_ENABLE_SIGN	0
-#  define IMAGE_ENABLE_VERIFY	0
 # define IMAGE_ENABLE_VERIFY_ECDSA	0
 #  define FIT_IMAGE_ENABLE_VERIFY	0
 # endif
 #else
 # define IMAGE_ENABLE_SIGN	0
-# define IMAGE_ENABLE_VERIFY		CONFIG_IS_ENABLED(RSA_VERIFY)
 # define IMAGE_ENABLE_VERIFY_ECDSA	0
 # define FIT_IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
 #endif
@@ -1294,7 +1291,7 @@ struct image_region {
 	int size;
 };
 
-#if IMAGE_ENABLE_VERIFY
+#if FIT_IMAGE_ENABLE_VERIFY
 # include <u-boot/hash-checksum.h>
 #endif
 struct checksum_algo {
-- 
2.31.1

[PATCH RFC 10/10] image: Eliminate IMAGE_ENABLE_VERIFY_ECDSA macro

[Alexandru Gagniuc]

This macro is no longer needed for code flow or #ifdefs. Remove it.

Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
---
 include/image.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/include/image.h b/include/image.h
index 4a7aa9358f..dd07771480 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1225,17 +1225,14 @@ int calculate_hash(const void *data, int data_len, const char *algo,
 #if defined(USE_HOSTCC)
 # if defined(CONFIG_FIT_SIGNATURE)
 #  define IMAGE_ENABLE_SIGN	1
-#  define IMAGE_ENABLE_VERIFY_ECDSA	1
 #  define FIT_IMAGE_ENABLE_VERIFY	1
 #  include <openssl/evp.h>
 # else
 #  define IMAGE_ENABLE_SIGN	0
-# define IMAGE_ENABLE_VERIFY_ECDSA	0
 #  define FIT_IMAGE_ENABLE_VERIFY	0
 # endif
 #else
 # define IMAGE_ENABLE_SIGN	0
-# define IMAGE_ENABLE_VERIFY_ECDSA	0
 # define FIT_IMAGE_ENABLE_VERIFY	CONFIG_IS_ENABLED(FIT_SIGNATURE)
 #endif
 
-- 
2.31.1

[PATCH RFC 01/10] common: Move host-only logic in image-sig.c to separate file

[Simon Glass]

Hi Alexandru,

On Fri, 14 May 2021 at 13:46, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> image-sig.c is used to map a hash or crypto algorithm name to a
> handler of that algorithm. There is some similarity between the host
> and target variants, with the differences worked out by #ifdefs. The
> purpose of this change is to remove those ifdefs.
>
> First, copy the file to a host-only version, and remove target
> specific code. Although it looks like we are duplicating code,
> subsequent patches will change the way target algorithms are searched.
> Besides we are only duplicating three string to struct mapping
> functions. This isn't something to fuss about.
> ---
>  common/image-sig-host.c | 134 ++++++++++++++++++++++++++++++++++++++++
>  tools/Makefile          |   2 +-
>  2 files changed, 135 insertions(+), 1 deletion(-)
>  create mode 100644 common/image-sig-host.c

Will we never support signing in the board code? So far it is true,
but I wonder if it will remain so, as things get more and more
complicated. For example, we may want to sign the devicetree (somehow)
after fix-ups. The current code structure makes it possible to add
signing if needed. If we decided we wanted to sign on the board, how
would we refactor things with this approach?

If this is host code, can we move it to tools/ ?

Regards,
Simon

[PATCH RFC 02/10] common: image-sig.c: Remove host-specific logic and #ifdefs

[Simon Glass]

On Fri, 14 May 2021 at 13:46, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> Remove any ifdefs in image-sig.c that were previously used to
> differentiate from the host code. Note that all code dedicated to
> relocating ->sign() and ->add_verify_data)_ can be safely removed,
> as signing is not supported target-side.
>
> NOTE that although it appears we are removing ecdsa256 support, this
> is intentional. ecdsa_verify() is a no-op on the target, and is
> currently only used by host code.

(need to fix that quick!)

>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/image-sig.c | 39 ++-------------------------------------
>  1 file changed, 2 insertions(+), 37 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH RFC 03/10] image: Add support for placing crypto_algo in linker lists

[Simon Glass]

On Fri, 14 May 2021 at 13:46, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> The purpose of this change is to enable crypto algorithms to be placed
> in linker lists, rather than be declared as a static array. The goal
> is to remove the crypto_algos array in a subsequent patch.
>
> Create a new linker list named "cryptos", and search it when
> image_get_crypto_algo() is invoked.
>
> NOTE that adding support for manual relocation of crypto_algos within
> linker lists is beyond the scope of this patch.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/image-sig.c | 9 +++++++++
>  include/image.h    | 5 +++++
>  2 files changed, 14 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH RFC 04/10] [UNTESTED] image: Add support for relocating crypto_algos in linker lists

[Simon Glass]

On Fri, 14 May 2021 at 13:46, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> Function pointers from crypto_algos array are relocated, when
> NEEDS_MANUAL_RELOC is set. This relocation doesn't happen if the algo
> is placed in a linker list. Implement this relocation.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/image-sig.c | 7 +++++++
>  1 file changed, 7 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

I don't normally bother with this, since it is due to a toolchain
failure, that really should have been fixed years ago. But I am sure
someone will be happy.

+Michal Simek

[PATCH RFC 05/10] image: rsa: Move verification algorithm to a linker list

[Simon Glass]

On Fri, 14 May 2021 at 13:46, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> Move the RSA verification crytpo_algo structure out of the
> crypto_algos array, and into a linker list.
>
> Although it appears we are adding an #ifdef to rsa-verify.c, the gains
> outweigh this small inconvenience. This is because rsa_verify() is
> defined differently based on #ifdefs. This change allows us to have
> a single definition of rsa_verify().
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/image-sig.c   |  9 ---------
>  lib/rsa/rsa-verify.c | 16 ++++++++++++++++
>  2 files changed, 16 insertions(+), 9 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH RFC 06/10] image: image-sig.c: Remove crypto_algos array

[Simon Glass]

On Fri, 14 May 2021 at 13:46, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> Crytographic algorithms (currently RSA), are stored in linker lists.
> The crypto_algos array is unused, so remove it, and any logic
> associated with it.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  common/image-sig.c | 13 -------------
>  1 file changed, 13 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH RFC 07/10] lib: ecdsa: Remove #ifdefs from ecdsa.h

[Simon Glass]

On Fri, 14 May 2021 at 13:46, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> It is no longer necessary to implement ecdsa_() functions as no-ops
> depending on config options. It is merely sufficient to provide the
> prototypes, as the ecdsa code is no longer linked when unused.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  include/u-boot/ecdsa.h | 25 -------------------------
>  1 file changed, 25 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH RFC 08/10] lib: rsa: Remove #ifdefs from rsa.h

[Simon Glass]

On Fri, 14 May 2021 at 13:46, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> It is no longer necessary to implement rsa_() functions as no-ops
> depending on config options. It is merely sufficient to provide the
> prototypes, as the rsa code is no longer linked when unused.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  include/u-boot/rsa.h | 47 --------------------------------------------
>  1 file changed, 47 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH RFC 09/10] image: Eliminate IMAGE_ENABLE_VERIFY macro

[Simon Glass]

On Fri, 14 May 2021 at 13:46, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> This macro is no longer needed for code flow or #ifdefs. Remove it.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  include/image.h | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH RFC 10/10] image: Eliminate IMAGE_ENABLE_VERIFY_ECDSA macro

[Simon Glass]

On Fri, 14 May 2021 at 13:46, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>
> This macro is no longer needed for code flow or #ifdefs. Remove it.
>
> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
> ---
>  include/image.h | 3 ---
>  1 file changed, 3 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

[PATCH RFC 01/10] common: Move host-only logic in image-sig.c to separate file

[Alex G.]

On 5/15/21 10:20 AM, Simon Glass wrote:
> Hi Alexandru,
> 
> On Fri, 14 May 2021 at 13:46, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>>
>> image-sig.c is used to map a hash or crypto algorithm name to a
>> handler of that algorithm. There is some similarity between the host
>> and target variants, with the differences worked out by #ifdefs. The
>> purpose of this change is to remove those ifdefs.
>>
>> First, copy the file to a host-only version, and remove target
>> specific code. Although it looks like we are duplicating code,
>> subsequent patches will change the way target algorithms are searched.
>> Besides we are only duplicating three string to struct mapping
>> functions. This isn't something to fuss about.
>> ---
>>   common/image-sig-host.c | 134 ++++++++++++++++++++++++++++++++++++++++
>>   tools/Makefile          |   2 +-
>>   2 files changed, 135 insertions(+), 1 deletion(-)
>>   create mode 100644 common/image-sig-host.c
> 
> Will we never support signing in the board code? So far it is true,
> but I wonder if it will remain so, as things get more and more
> complicated. For example, we may want to sign the devicetree (somehow)
> after fix-ups. The current code structure makes it possible to add
> signing if needed. If we decided we wanted to sign on the board, how
> would we refactor things with this approach?

We'd have the logistics of keeping private keys available to firmware 
and only to firmware, but those are orthogonal to the problem. Assuming 
we implemented a device-side *_sign(), then we would add it to the 
linker list, via the proposed U_BOOT_CRYPTO_ALGO():


int rsa_device_side_sign(...)
{
	if (!CONFIG_IS_ENABLED(RSA_SIGN_ON_DEVICE))
		return -EIEIO;
	
	return do_rsa_device_side_sign(...);
}

  U_BOOT_CRYPTO_ALGO(rsa2048) = {
  	.name = "rsa2048",
  	.key_len = RSA2048_BYTES,
  	.verify = rsa_verify,
  	.sign = rsa_device_side_sign,
  };

> If this is host code, can we move it to tools/ ?

Definitely!

[PATCH RFC 04/10] [UNTESTED] image: Add support for relocating crypto_algos in linker lists

[Michal Simek]

On 5/15/21 5:20 PM, Simon Glass wrote:
> On Fri, 14 May 2021 at 13:46, Alexandru Gagniuc <mr.nuke.me@gmail.com> wrote:
>>
>> Function pointers from crypto_algos array are relocated, when
>> NEEDS_MANUAL_RELOC is set. This relocation doesn't happen if the algo
>> is placed in a linker list. Implement this relocation.
>>
>> Signed-off-by: Alexandru Gagniuc <mr.nuke.me@gmail.com>
>> ---
>>  common/image-sig.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> I don't normally bother with this, since it is due to a toolchain
> failure, that really should have been fixed years ago. But I am sure
> someone will be happy.
> 
> +Michal Simek

It looks good to me.
Acked-by: Michal Simek <michal.simek@xilinx.com>

Thanks,
Michal