Re: [PATCH] lib: Move selection of SPL hash algorithms from common/

2021-04-22 Thread Tom Rini
On Mon, Mar 22, 2021 at 08:33:31AM -0500, Alexandru Gagniuc wrote:

> When God said, "May there be FIT signature verification in SPL",
> Chuck Norris said "SPL image too big". And then there was this patch.
> 
> Enabling SPL_FIT_SIGNATURE increased the code size (armv7 platform) by
> about 16KiB, just enough to go over the SPL image limit. Of that:
>   * .text.sha256_process  3.8 KiB
>   * SHA1 implementation 4.4 KiB
> Although SHA1 wasn't required, it could not be disabled.
> 
> The hash algorithms are implemented in lib/, as is their Kconfig
> selection for u-boot main. However, Kconfig selection for SPL is
> implemented in common/. To put it mildly, this is inconsistent.
> MD5 selection, on the other hand, does not have this problem.
> 
> Moving the SPL hash switches to lib/ solves half the problem. They
> have to be renamed from SPL__SUPPORT to SPL_ to make
> them work elegantly with the CONFIG_IS_ENABLED() macro.
> 
> The second half of the problem is not referencing the  symbols
> when  is disabled. Unfortunately, this requires some more
> 
> The above #ifdef problem could be solved in several ways. One way
> could be to move the hash handlers to linker lists. This, however,
> won't work for userspace tools (mkimage), as they don't implement
> custom linker scripts. One could implement a _register()
> function for this case, and manually register all hashes. However,
> this is beyond the scope of this patch.
> 
> Signed-off-by: Alexandru Gagniuc 
> ---
> 
> This is designed to apply on top of the following series:
>  * [PATCH v6 00/11] Add support for ECDSA image signing
> 
>  common/hash.c  |  4 ++--
>  common/image-sig.c |  8 +--
>  common/spl/Kconfig | 54 --
>  include/image.h| 12 +--
>  lib/Kconfig| 39 +
>  lib/Makefile   |  6 +++---
>  6 files changed, 56 insertions(+), 67 deletions(-)

I like this idea.  As-is, there's a few problems.  socfpga_agilex_vab
and imx8mm_venice now fail to build due to missing sha384 support for
the former and sram overflow for the latter.   ls1046ardb_qspi_spl now
also grows SPL a bit by adding sha1 support.  Can you look in to these
please?  Thanks.

-- 
Tom


signature.asc
Description: PGP signature


[PATCH] lib: Move selection of SPL hash algorithms from common/

2021-03-22 Thread Alexandru Gagniuc
When God said, "May there be FIT signature verification in SPL",
Chuck Norris said "SPL image too big". And then there was this patch.

Enabling SPL_FIT_SIGNATURE increased the code size (armv7 platform) by
about 16KiB, just enough to go over the SPL image limit. Of that:
  * .text.sha256_process3.8 KiB
  * SHA1 implementation 4.4 KiB
Although SHA1 wasn't required, it could not be disabled.

The hash algorithms are implemented in lib/, as is their Kconfig
selection for u-boot main. However, Kconfig selection for SPL is
implemented in common/. To put it mildly, this is inconsistent.
MD5 selection, on the other hand, does not have this problem.

Moving the SPL hash switches to lib/ solves half the problem. They
have to be renamed from SPL__SUPPORT to SPL_ to make
them work elegantly with the CONFIG_IS_ENABLED() macro.

The second half of the problem is not referencing the  symbols
when  is disabled. Unfortunately, this requires some more

The above #ifdef problem could be solved in several ways. One way
could be to move the hash handlers to linker lists. This, however,
won't work for userspace tools (mkimage), as they don't implement
custom linker scripts. One could implement a _register()
function for this case, and manually register all hashes. However,
this is beyond the scope of this patch.

Signed-off-by: Alexandru Gagniuc 
---

This is designed to apply on top of the following series:
 * [PATCH v6 00/11] Add support for ECDSA image signing

 common/hash.c  |  4 ++--
 common/image-sig.c |  8 +--
 common/spl/Kconfig | 54 --
 include/image.h| 12 +--
 lib/Kconfig| 39 +
 lib/Makefile   |  6 +++---
 6 files changed, 56 insertions(+), 67 deletions(-)

diff --git a/common/hash.c b/common/hash.c
index fc64002f73..dbce70e89b 100644
--- a/common/hash.c
+++ b/common/hash.c
@@ -41,7 +41,7 @@ DECLARE_GLOBAL_DATA_PTR;
 
 static void reloc_update(void);
 
-#if defined(CONFIG_SHA1) && !defined(CONFIG_SHA_PROG_HW_ACCEL)
+#if IMAGE_ENABLE_SHA1 && !defined(CONFIG_SHA_PROG_HW_ACCEL)
 static int hash_init_sha1(struct hash_algo *algo, void **ctxp)
 {
sha1_context *ctx = malloc(sizeof(sha1_context));
@@ -213,7 +213,7 @@ static int hash_finish_crc32(struct hash_algo *algo, void 
*ctx, void *dest_buf,
  * Note that algorithm names must be in lower case.
  */
 static struct hash_algo hash_algo[] = {
-#ifdef CONFIG_SHA1
+#if IMAGE_ENABLE_SHA1
{
.name   = "sha1",
.digest_size= SHA1_SUM_LEN,
diff --git a/common/image-sig.c b/common/image-sig.c
index 0f8e592aba..dbef978bef 100644
--- a/common/image-sig.c
+++ b/common/image-sig.c
@@ -23,6 +23,7 @@ DECLARE_GLOBAL_DATA_PTR;
 #define IMAGE_MAX_HASHED_NODES 100
 
 struct checksum_algo checksum_algos[] = {
+#if IMAGE_ENABLE_SHA1
{
.name = "sha1",
.checksum_len = SHA1_SUM_LEN,
@@ -33,6 +34,8 @@ struct checksum_algo checksum_algos[] = {
 #endif
.calculate = hash_calculate,
},
+#endif
+#if IMAGE_ENABLE_SHA256
{
.name = "sha256",
.checksum_len = SHA256_SUM_LEN,
@@ -43,7 +46,8 @@ struct checksum_algo checksum_algos[] = {
 #endif
.calculate = hash_calculate,
},
-#ifdef CONFIG_SHA384
+#endif
+#if IMAGE_ENABLE_SHA384
{
.name = "sha384",
.checksum_len = SHA384_SUM_LEN,
@@ -55,7 +59,7 @@ struct checksum_algo checksum_algos[] = {
.calculate = hash_calculate,
},
 #endif
-#ifdef CONFIG_SHA512
+#if IMAGE_ENABLE_SHA512
{
.name = "sha512",
.checksum_len = SHA512_SUM_LEN,
diff --git a/common/spl/Kconfig b/common/spl/Kconfig
index 774541c02b..85c542e0e0 100644
--- a/common/spl/Kconfig
+++ b/common/spl/Kconfig
@@ -412,60 +412,6 @@ config SPL_CRC32_SUPPORT
  for detected accidental image corruption. For secure applications you
  should consider SHA1 or SHA256.
 
-config SPL_MD5_SUPPORT
-   bool "Support MD5"
-   depends on SPL_FIT
-   help
- Enable this to support MD5 in FIT images within SPL. An MD5
- checksum is a 128-bit hash value used to check that the image
- contents have not been corrupted. Note that MD5 is not considered
- secure as it is possible (with a brute-force attack) to adjust the
- image while still retaining the same MD5 hash value. For secure
- applications where images may be changed maliciously, you should
- consider SHA256 or SHA384.
-
-config SPL_SHA1_SUPPORT
-   bool "Support SHA1"
-   depends on SPL_FIT
-   select SHA1
-   help
- Enable this to support SHA1 in FIT images within SPL. A SHA1
- checksum is a 160-bit (20-byte) hash value used to check that the
- image contents have not been corrupted or maliciously altered.
- While SHA1 is f