Hi Tom,

On Tue, 2 Jul 2024 at 18:48, Tom Rini trini@konsulko.com wrote:

On Tue, Jul 02, 2024 at 11:22:40AM -0700, Raymond Mao wrote:

...

Adapt digest header files to support both original libs and MbedTLS by switching on/off MBEDTLS_LIB_CRYPTO. Introduce <alg>_LEGACY kconfig for legacy hash implementations.

[snip]

...

diff --git a/lib/mbedtls/Kconfig b/lib/mbedtls/Kconfig index 3e9057f1acf..6662a9d20f1 100644 --- a/lib/mbedtls/Kconfig +++ b/lib/mbedtls/Kconfig @@ -21,9 +21,105 @@ if LEGACY_CRYPTO

config LEGACY_CRYPTO_BASIC bool "legacy basic crypto libraries"

• select MD5_LEGACY if MD5
  

• select SHA1_LEGACY if SHA1
  

• select SHA256_LEGACY if SHA256
  

• select SHA512_LEGACY if SHA512
  

• select SHA384_LEGACY if SHA384
  

• select SPL_MD5_LEGACY if MD5 && SPL
  

• select SPL_SHA1_LEGACY if SHA1 && SPL
  

• select SPL_SHA256_LEGACY if SHA256 && SPL
  

• select SPL_SHA512_LEGACY if SHA512 && SPL
  

• select SPL_SHA384_LEGACY if SHA384 && SPL
  help
    Enable legacy basic crypto libraries.
  
  

+if LEGACY_CRYPTO_BASIC

+config SHA1_LEGACY

• bool "Enable SHA1 support with legacy crypto library"
  

• depends on LEGACY_CRYPTO_BASIC && SHA1
  

• help
  

•   This option enables support of hashing using SHA1 algorithm
  

•   with legacy crypto library.
  

+config SHA256_LEGACY

• bool "Enable SHA256 support with legacy crypto library"
  

• depends on LEGACY_CRYPTO_BASIC && SHA256
  

• help
  

•   This option enables support of hashing using SHA256 algorithm
  

•   with legacy crypto library.
  

+config SHA512_LEGACY

• bool "Enable SHA512 support with legacy crypto library"
  

• depends on LEGACY_CRYPTO_BASIC && SHA512
  

• default y if TI_SECURE_DEVICE && FIT_SIGNATURE
  

• help
  

•   This option enables support of hashing using SHA512 algorithm
  

•   with legacy crypto library.
  

+config SHA384_LEGACY

• bool "Enable SHA384 support with legacy crypto library"
  

• depends on LEGACY_CRYPTO_BASIC && SHA384
  

• select SHA512_LEGACY
  

• help
  

•   This option enables support of hashing using SHA384 algorithm
  

•   with legacy crypto library.
  

+config MD5_LEGACY

• bool "Enable MD5 support with legacy crypto library"
  

• depends on LEGACY_CRYPTO_BASIC && MD5
  

• help
  

•   This option enables support of hashing using MD5 algorithm
  

•   with legacy crypto library.
  

+if SPL

+config SPL_SHA1_LEGACY

• bool "Enable SHA1 support in SPL with legacy crypto library"
  

• depends on LEGACY_CRYPTO_BASIC && SPL_SHA1
  

• default y if SHA1 && LEGACY_CRYPTO_BASIC
  

• help
  

•   This option enables support of hashing using SHA1 algorithm
  

•   with legacy crypto library.
  

+config SPL_SHA256_LEGACY

• bool "Enable SHA256 support in SPL with legacy crypto library"
  

• depends on LEGACY_CRYPTO_BASIC && SPL_SHA256
  

• default y if SHA256 && LEGACY_CRYPTO_BASIC
  

• help
  

•   This option enables support of hashing using SHA256 algorithm
  

•   with legacy crypto library.
  

+config SPL_SHA512_LEGACY

• bool "Enable SHA512 support in SPL with legacy crypto library"
  

• depends on LEGACY_CRYPTO_BASIC && SPL_SHA512
  

• default y if SHA512 && LEGACY_CRYPTO_BASIC
  

• help
  

•   This option enables support of hashing using SHA512 algorithm
  

•   with legacy crypto library.
  

+config SPL_SHA384_LEGACY

• bool "Enable SHA384 support in SPL with legacy crypto library"
  

• depends on LEGACY_CRYPTO_BASIC && SPL_SHA384
  

• default y if SHA384 && LEGACY_CRYPTO_BASIC
  

• select SPL_SHA512
  

• help
  

•   This option enables support of hashing using SHA384 algorithm
  

•   with legacy crypto library.
  

+config SPL_MD5_LEGACY

• bool "Enable MD5 support in SPL with legacy crypto library"
  

• depends on LEGACY_CRYPTO_BASIC && SPL_MD5
  

• default y if MD5 && LEGACY_CRYPTO_BASIC
  

• help
  

•   This option enables support of hashing using MD5 algorithm
  

•   with legacy crypto library.
  

+endif # SPL

+endif # LEGACY_CRYPTO_BASIC

config LEGACY_CRYPTO_CERT bool "legacy certificate libraries" help

This is all certainly moving in the right direction, but there's dependency issues: aarch64: w+ xilinx_zynqmp_kria +(xilinx_zynqmp_kria) +(xilinx_zynqmp_kria) WARNING: unmet direct dependencies detected for SPL_MD5_LEGACY +(xilinx_zynqmp_kria) Depends on [n]: LEGACY_CRYPTO [=y] && SPL [=y] && LEGACY_CRYPTO_BASIC [=y] && SPL_MD5 [=n] +(xilinx_zynqmp_kria) Selected by [y]: +(xilinx_zynqmp_kria) - LEGACY_CRYPTO_BASIC [=y] && LEGACY_CRYPTO [=y] && MD5 [=y] && SPL [=y]

I am a bit confused by SPL_MD5, why it is [n] when both MD5 and SPL are [y]. Of course we can replace 'SPL_MD5' with 'MD5 && SPL' to solve the warning, but I guess the wrong dependence is on SPL_MD5 but not the new added ones. ``` config SPL_MD5 bool "Support MD5 algorithm in SPL" depends on SPL ``` I think it should be 'default y if SPL && MD5' instead of 'depends on SPL'.

Similarly SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE is not selected when both ASYMMETRIC_PUBLIC_KEY_SUBTYPE and SPL are selected; SPL_ASN1_DECODER is not selected when both ASN1_DECODER and SPL are selected.

I think generally for each algorithm, 'SPL_<ALG>' config should be selected when both 'SPL' and '<ALG>' are selected.

If you agree I can fix this in the next patch set.

Regards, Raymond

On Tue, 2 Jul 2024 at 21:26, Raymond Mao raymond.mao@linaro.org wrote:

We don't need an API specially for non-watchdog since sha1_csum_wd supports it by disabling CONFIG_HW_WATCHDOG and CONFIG_WATCHDOG. Set 0x10000 as default chunk size for SHA1.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v4

• Initial patch.

board/gdsys/a38x/hre.c | 2 +- include/u-boot/sha1.h | 12 ++---------- lib/sha1.c | 13 ------------- lib/tpm-v1.c | 2 +- 4 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/board/gdsys/a38x/hre.c b/board/gdsys/a38x/hre.c index f303793b63b..06856ea36d3 100644 --- a/board/gdsys/a38x/hre.c +++ b/board/gdsys/a38x/hre.c @@ -166,7 +166,7 @@ static int find_key(struct udevice *tpm, const uint8_t auth[20], return -1; if (err) continue;

•           sha1_csum(buf, buf_len, digest);
  

•           sha1_csum_wd(buf, buf_len, digest, SHA1_DEF_CHUNK_SZ);
            if (!memcmp(digest, pubkey_digest, 20)) {
                    *handle = key_handles[i];
                    return 0;
  

diff --git a/include/u-boot/sha1.h b/include/u-boot/sha1.h index ab88134fb98..36c3db15e22 100644 --- a/include/u-boot/sha1.h +++ b/include/u-boot/sha1.h @@ -39,6 +39,8 @@ extern "C" { #define SHA1_SUM_LEN 20 #define SHA1_DER_LEN 15

+#define SHA1_DEF_CHUNK_SZ 0x10000

extern const uint8_t sha1_der_prefix[];

#if defined(CONFIG_MBEDTLS_LIB_CRYPTO) @@ -81,16 +83,6 @@ void sha1_update(sha1_context *ctx, const unsigned char *input, */ void sha1_finish( sha1_context *ctx, unsigned char output[20] );

-/**

• • \brief Output = SHA-1( input buffer )
• • \param input buffer holding the data
• • \param ilen length of the input data
• • \param output SHA-1 checksum result
• */

-void sha1_csum(const unsigned char *input, unsigned int ilen,

•           unsigned char *output);
  

/**

• \brief Output = SHA-1( input buffer ), with watchdog triggering

diff --git a/lib/sha1.c b/lib/sha1.c index 7ef536f4b5d..81412283b49 100644 --- a/lib/sha1.c +++ b/lib/sha1.c @@ -304,19 +304,6 @@ void sha1_finish (sha1_context * ctx, unsigned char output[20]) PUT_UINT32_BE (ctx->state[4], output, 16); }

-/*

• • Output = SHA-1( input buffer )
• */

-void sha1_csum(const unsigned char *input, unsigned int ilen,

•          unsigned char *output)
  

-{

•   sha1_context ctx;
  

•   sha1_starts (&ctx);
  

•   sha1_update (&ctx, input, ilen);
  

•   sha1_finish (&ctx, output);
  

-}

/*

• Output = SHA-1( input buffer ). Trigger the watchdog every 'chunk_sz'
• bytes of input processed.

diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c index e66023da5e6..a6727c575fd 100644 --- a/lib/tpm-v1.c +++ b/lib/tpm-v1.c @@ -871,7 +871,7 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20], return -1; if (err) continue;

•           sha1_csum(buf, buf_len, digest);
  

•           sha1_csum_wd(buf, buf_len, digest, SHA1_DEF_CHUNK_SZ);
            if (!memcmp(digest, pubkey_digest, 20)) {
                    *handle = key_handles[i];
                    return 0;
  

-- 2.25.1

Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Hi Raymond

[...]

+if MBEDTLS_LIB_CRYPTO

+config SHA1_MBEDTLS

•   bool "Enable SHA1 support with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SHA1
  

•   help
  

•     This option enables support of hashing using SHA1 algorithm
  

•     with MbedTLS crypto library.
  

+config SHA256_MBEDTLS

•   bool "Enable SHA256 support with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SHA256
  

•   help
  

•     This option enables support of hashing using SHA256 algorithm
  

•     with MbedTLS crypto library.
  

+config SHA512_MBEDTLS

•   bool "Enable SHA512 support with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SHA512
  

•   default y if TI_SECURE_DEVICE && FIT_SIGNATURE
  

I don't like this default 'y' for 'ti secure devices'. This is a board specific option. I think it should be defined on the defconfig of those boards instead.

•   help
  

•     This option enables support of hashing using SHA512 algorithm
  

•     with MbedTLS crypto library.
  

+config SHA384_MBEDTLS

•   bool "Enable SHA384 support with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SHA384
  

•   select SHA512_MBEDTLS
  

•   help
  

•     This option enables support of hashing using SHA384 algorithm
  

•     with MbedTLS crypto library.
  

+config MD5_MBEDTLS

•   bool "Enable MD5 support with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && MD5
  

•   help
  

•     This option enables support of hashing using MD5 algorithm
  

•     with MbedTLS crypto library.
  

+if SPL

+config SPL_SHA1_MBEDTLS

•   bool "Enable SHA1 support in SPL with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SPL_SHA1
  

•   default y if SHA1 && MBEDTLS_LIB_CRYPTO
  

•   help
  

•     This option enables support of hashing using SHA1 algorithm
  

•     with MbedTLS crypto library.
  

+config SPL_SHA256_MBEDTLS

•   bool "Enable SHA256 support in SPL with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SPL_SHA256
  

•   default y if SHA256 && MBEDTLS_LIB_CRYPTO
  

•   help
  

•     This option enables support of hashing using SHA256 algorithm
  

•     with MbedTLS crypto library.
  

+config SPL_SHA512_MBEDTLS

•   bool "Enable SHA512 support in SPL with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SPL_SHA512
  

•   default y if SHA512 && MBEDTLS_LIB_CRYPTO
  

•   help
  

•     This option enables support of hashing using SHA512 algorithm
  

•     with MbedTLS crypto library.
  

+config SPL_SHA384_MBEDTLS

•   bool "Enable SHA384 support in SPL with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SPL_SHA384
  

•   default y if SHA384 && MBEDTLS_LIB_CRYPTO
  

•   select SPL_SHA512
  

•   help
  

•     This option enables support of hashing using SHA384 algorithm
  

•     with MbedTLS crypto library.
  

+config SPL_MD5_MBEDTLS

•   bool "Enable MD5 support in SPL with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SPL_MD5
  

•   default y if MD5 && MBEDTLS_LIB_CRYPTO
  

•   help
  

•     This option enables support of hashing using MD5 algorithm
  

•     with MbedTLS crypto library.
  

+endif # SPL

+endif # MBEDTLS_LIB_CRYPTO

config MBEDTLS_LIB_X509 bool "MbedTLS certificate libraries" diff --git a/lib/mbedtls/Makefile b/lib/mbedtls/Makefile index 803ea0b62a0..32a98b7f4ca 100644 --- a/lib/mbedtls/Makefile +++ b/lib/mbedtls/Makefile @@ -13,17 +13,24 @@ ccflags-y += \ -I$(src)/external/mbedtls/include \ -I$(src)/external/mbedtls/library

+# shim layer for hash +obj-$(CONFIG_MBEDTLS_LIB_CRYPTO) += hash_mbedtls.o +hash_mbedtls-$(CONFIG_$(SPL_)MD5_MBEDTLS) += md5.o +hash_mbedtls-$(CONFIG_$(SPL_)SHA1_MBEDTLS) += sha1.o +hash_mbedtls-$(CONFIG_$(SPL_)SHA256_MBEDTLS) += sha256.o +hash_mbedtls-$(CONFIG_$(SPL_)SHA512_MBEDTLS) += sha512.o

# MbedTLS crypto library obj-$(CONFIG_MBEDTLS_LIB_CRYPTO) += mbedtls_lib_crypto.o mbedtls_lib_crypto-y += \ $(MBEDTLS_LIB_DIR)/platform_util.o \ $(MBEDTLS_LIB_DIR)/constant_time.o \ $(MBEDTLS_LIB_DIR)/md.o -mbedtls_lib_crypto-$(CONFIG_$(SPL_)MD5) += $(MBEDTLS_LIB_DIR)/md5.o -mbedtls_lib_crypto-$(CONFIG_$(SPL_)SHA1) += $(MBEDTLS_LIB_DIR)/sha1.o -mbedtls_lib_crypto-$(CONFIG_$(SPL_)SHA256) += \ +mbedtls_lib_crypto-$(CONFIG_$(SPL_)MD5_MBEDTLS) += $(MBEDTLS_LIB_DIR)/md5.o +mbedtls_lib_crypto-$(CONFIG_$(SPL_)SHA1_MBEDTLS) += $(MBEDTLS_LIB_DIR)/sha1.o +mbedtls_lib_crypto-$(CONFIG_$(SPL_)SHA256_MBEDTLS) += \ $(MBEDTLS_LIB_DIR)/sha256.o -mbedtls_lib_crypto-$(CONFIG_$(SPL_)SHA512) += \ +mbedtls_lib_crypto-$(CONFIG_$(SPL_)SHA512_MBEDTLS) += \ $(MBEDTLS_LIB_DIR)/sha512.o

# MbedTLS X509 library diff --git a/lib/mbedtls/md5.c b/lib/mbedtls/md5.c new file mode 100644 index 00000000000..04388fce249 --- /dev/null +++ b/lib/mbedtls/md5.c @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Hash shim layer on MbedTLS Crypto library
• • Copyright (c) 2024 Linaro Limited
• • Author: Raymond Mao raymond.mao@linaro.org
• */

+#include "compiler.h"

+#ifndef USE_HOSTCC +#include <watchdog.h> +#endif /* USE_HOSTCC */ +#include <u-boot/md5.h>

+void MD5Init(MD5Context *ctx) +{

•   mbedtls_md5_init(ctx);
  

•   mbedtls_md5_starts(ctx);
  

+}

+void MD5Update(MD5Context *ctx, unsigned char const *buf, unsigned int len) +{

•   mbedtls_md5_update(ctx, buf, len);
  

+}

+void MD5Final(unsigned char digest[16], MD5Context *ctx) +{

•   mbedtls_md5_finish(ctx, digest);
  

•   mbedtls_md5_free(ctx);
  

+}

+void md5_wd(const unsigned char *input, unsigned int len,

•       unsigned char output[16], unsigned int chunk_sz)
  

+{

•   MD5Context context;
  

•   MD5Init(&context);
  

•   if (IS_ENABLED(CONFIG_HW_WATCHDOG) || IS_ENABLED(CONFIG_WATCHDOG)) {
  

•           const unsigned char *curr = input;
  

•           const unsigned char *end = input + len;
  

•           int chunk;
  

•           while (curr < end) {
  

•                   chunk = end - curr;
  

•                   if (chunk > chunk_sz)
  

•                           chunk = chunk_sz;
  

•                   MD5Update(&context, curr, chunk);
  

•                   curr += chunk;
  

•                   schedule();
  

•           }
  

•   } else {
  

•           MD5Update(&context, input, len);
  

•   }
  

You can probably split this part to a function, that takes the hashing algo as an argument, since the loop is identical for md5, sha1, sha256 etc

•   MD5Final(output, &context);
  

+} diff --git a/lib/mbedtls/sha1.c b/lib/mbedtls/sha1.c new file mode 100644 index 00000000000..2aee5037795 --- /dev/null +++ b/lib/mbedtls/sha1.c @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Hash shim layer on MbedTLS Crypto library
• • Copyright (c) 2024 Linaro Limited
• • Author: Raymond Mao raymond.mao@linaro.org
• */

+#ifndef USE_HOSTCC +#include <cyclic.h> +#endif /* USE_HOSTCC */ +#include <string.h> +#include <u-boot/sha1.h>

+const u8 sha1_der_prefix[SHA1_DER_LEN] = {

•   0x30, 0x21, 0x30, 0x09, 0x06, 0x05, 0x2b, 0x0e,
  

•   0x03, 0x02, 0x1a, 0x05, 0x00, 0x04, 0x14
  

+};

+void sha1_starts(sha1_context *ctx) +{

•   mbedtls_sha1_init(ctx);
  

•   mbedtls_sha1_starts(ctx);
  

+}

+void sha1_update(sha1_context *ctx, const unsigned char *input,

•            unsigned int length)
  

+{

•   mbedtls_sha1_update(ctx, input, length);
  

+}

+void sha1_finish(sha1_context *ctx, unsigned char output[SHA1_SUM_LEN]) +{

•   mbedtls_sha1_finish(ctx, output);
  

•   mbedtls_sha1_free(ctx);
  

+}

+void sha1_csum_wd(const unsigned char *input, unsigned int ilen,

•             unsigned char *output, unsigned int chunk_sz)
  

+{

•   sha1_context ctx;
  

•   sha1_starts(&ctx);
  

•   if (IS_ENABLED(CONFIG_HW_WATCHDOG) || IS_ENABLED(CONFIG_WATCHDOG)) {
  

•           const unsigned char *curr = input;
  

•           const unsigned char *end = input + ilen;
  

•           int chunk;
  

•           while (curr < end) {
  

•                   chunk = end - curr;
  

•                   if (chunk > chunk_sz)
  

•                           chunk = chunk_sz;
  

•                   sha1_update(&ctx, curr, chunk);
  

•                   curr += chunk;
  

•                   schedule();
  

•           }
  

•   } else {
  

•           sha1_update(&ctx, input, ilen);
  

•   }
  

•   sha1_finish(&ctx, output);
  

+}

+void sha1_hmac(const unsigned char *key, int keylen,

•          const unsigned char *input, unsigned int ilen,
  

•          unsigned char *output)
  

+{

•   int i;
  

•   sha1_context ctx;
  

•   unsigned char k_ipad[K_PAD_LEN];
  

•   unsigned char k_opad[K_PAD_LEN];
  

•   unsigned char tmpbuf[20];
  

•   if (keylen > K_PAD_LEN)
  

•           return;
  

•   memset(k_ipad, K_IPAD_VAL, sizeof(k_ipad));
  

•   memset(k_opad, K_OPAD_VAL, sizeof(k_opad));
  

•   for (i = 0; i < keylen; i++) {
  

•           k_ipad[i] ^= key[i];
  

•           k_opad[i] ^= key[i];
  

•   }
  

•   sha1_starts(&ctx);
  

•   sha1_update(&ctx, k_ipad, sizeof(k_ipad));
  

•   sha1_update(&ctx, input, ilen);
  

•   sha1_finish(&ctx, tmpbuf);
  

•   sha1_starts(&ctx);
  

•   sha1_update(&ctx, k_opad, sizeof(k_opad));
  

•   sha1_update(&ctx, tmpbuf, sizeof(tmpbuf));
  

•   sha1_finish(&ctx, output);
  

•   memset(k_ipad, 0, sizeof(k_ipad));
  

•   memset(k_opad, 0, sizeof(k_opad));
  

•   memset(tmpbuf, 0, sizeof(tmpbuf));
  

•   memset(&ctx, 0, sizeof(sha1_context));
  

+} diff --git a/lib/mbedtls/sha256.c b/lib/mbedtls/sha256.c new file mode 100644 index 00000000000..24aa58fa674 --- /dev/null +++ b/lib/mbedtls/sha256.c @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Hash shim layer on MbedTLS Crypto library
• • Copyright (c) 2024 Linaro Limited
• • Author: Raymond Mao raymond.mao@linaro.org
• */

+#ifndef USE_HOSTCC +#include <cyclic.h> +#endif /* USE_HOSTCC */ +#include <u-boot/sha256.h>

+const u8 sha256_der_prefix[SHA256_DER_LEN] = {

•   0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86,
  

•   0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01, 0x05,
  

•   0x00, 0x04, 0x20
  

+};

+void sha256_starts(sha256_context *ctx) +{

•   mbedtls_sha256_init(ctx);
  

•   mbedtls_sha256_starts(ctx, 0);
  

+}

+void +sha256_update(sha256_context *ctx, const uint8_t *input, uint32_t length) +{

•   mbedtls_sha256_update(ctx, input, length);
  

+}

+void sha256_finish(sha256_context *ctx, uint8_t digest[SHA256_SUM_LEN]) +{

•   mbedtls_sha256_finish(ctx, digest);
  

•   mbedtls_sha256_free(ctx);
  

+}

+void sha256_csum_wd(const unsigned char *input, unsigned int ilen,

•               unsigned char *output, unsigned int chunk_sz)
  

+{

•   sha256_context ctx;
  

•   sha256_starts(&ctx);
  

•   if (IS_ENABLED(CONFIG_HW_WATCHDOG) || IS_ENABLED(CONFIG_WATCHDOG)) {
  

•           const unsigned char *curr = input;
  

•           const unsigned char *end = input + ilen;
  

•           int chunk;
  

•           while (curr < end) {
  

•                   chunk = end - curr;
  

•                   if (chunk > chunk_sz)
  

•                           chunk = chunk_sz;
  

•                   sha256_update(&ctx, curr, chunk);
  

•                   curr += chunk;
  

•                   schedule();
  

•           }
  

•   } else {
  

•           sha256_update(&ctx, input, ilen);
  

•   }
  

•   sha256_finish(&ctx, output);
  

+} diff --git a/lib/mbedtls/sha512.c b/lib/mbedtls/sha512.c new file mode 100644 index 00000000000..5615248cb91 --- /dev/null +++ b/lib/mbedtls/sha512.c @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Hash shim layer on MbedTLS Crypto library
• • Copyright (c) 2024 Linaro Limited
• • Author: Raymond Mao raymond.mao@linaro.org
• */

+#ifndef USE_HOSTCC +#include <cyclic.h> +#endif /* USE_HOSTCC */ +#include <compiler.h> +#include <u-boot/sha512.h>

+const u8 sha384_der_prefix[SHA384_DER_LEN] = {

•   0x30, 0x41, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86,
  

•   0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x02, 0x05,
  

•   0x00, 0x04, 0x30
  

+};

+const u8 sha512_der_prefix[SHA512_DER_LEN] = {

•   0x30, 0x51, 0x30, 0x0d, 0x06, 0x09, 0x60, 0x86,
  

•   0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x03, 0x05,
  

•   0x00, 0x04, 0x40
  

+};

+void sha384_starts(sha512_context *ctx) +{

•   mbedtls_sha512_init(ctx);
  

•   mbedtls_sha512_starts(ctx, 1);
  

+}

+void +sha384_update(sha512_context *ctx, const uint8_t *input, uint32_t length) +{

•   mbedtls_sha512_update(ctx, input, length);
  

+}

+void sha384_finish(sha512_context *ctx, uint8_t digest[SHA384_SUM_LEN]) +{

•   mbedtls_sha512_finish(ctx, digest);
  

•   mbedtls_sha512_free(ctx);
  

+}

+void sha384_csum_wd(const unsigned char *input, unsigned int length,

•               unsigned char *output, unsigned int chunk_sz)
  

+{

•   mbedtls_sha512(input, length, output, 1);
  

+}

+void sha512_starts(sha512_context *ctx) +{

•   mbedtls_sha512_init(ctx);
  

•   mbedtls_sha512_starts(ctx, 0);
  

+}

+void +sha512_update(sha512_context *ctx, const uint8_t *input, uint32_t length) +{

•   mbedtls_sha512_update(ctx, input, length);
  

+}

+void sha512_finish(sha512_context *ctx, uint8_t digest[SHA512_SUM_LEN]) +{

•   mbedtls_sha512_finish(ctx, digest);
  

•   mbedtls_sha512_free(ctx);
  

+}

+void sha512_csum_wd(const unsigned char *input, unsigned int ilen,

•               unsigned char *output, unsigned int chunk_sz)
  

+{

•   sha512_context ctx;
  

•   sha512_starts(&ctx);
  

•   if (IS_ENABLED(CONFIG_HW_WATCHDOG) || IS_ENABLED(CONFIG_WATCHDOG)) {
  

•           const unsigned char *curr = input;
  

•           const unsigned char *end = input + ilen;
  

•           int chunk;
  

•           while (curr < end) {
  

•                   chunk = end - curr;
  

•                   if (chunk > chunk_sz)
  

•                           chunk = chunk_sz;
  

•                   sha512_update(&ctx, curr, chunk);
  

•                   curr += chunk;
  

•                   schedule();
  

•           }
  

•   } else {
  

•           sha512_update(&ctx, input, ilen);
  

•   }
  

•   sha512_finish(&ctx, output);
  

+}

2.25.1

Hi Ilias,

On Fri, 26 Jul 2024 at 06:19, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

Hi Raymond

[...]

...

+if MBEDTLS_LIB_CRYPTO

+config SHA1_MBEDTLS

•   bool "Enable SHA1 support with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SHA1
  

•   help
  

•     This option enables support of hashing using SHA1 algorithm
  

•     with MbedTLS crypto library.
  

+config SHA256_MBEDTLS

•   bool "Enable SHA256 support with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SHA256
  

•   help
  

•     This option enables support of hashing using SHA256 algorithm
  

•     with MbedTLS crypto library.
  

+config SHA512_MBEDTLS

•   bool "Enable SHA512 support with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SHA512
  

•   default y if TI_SECURE_DEVICE && FIT_SIGNATURE
  

I don't like this default 'y' for 'ti secure devices'. This is a board specific option. I think it should be defined on the defconfig of those boards instead.

This dependence completely follows the original SHA512.

Removing TI_SECURE_DEVICE brings some additions work out of the scope of this patch series since we have to update all defconfigs which enables ARCH_K3 (TI_SECURE_DEVICE is an implicit setting of ARCH_K3). If we plan to do this, it is better to have another optimization patch set after this one is merged.

...

•   help
  

•     This option enables support of hashing using SHA512 algorithm
  

•     with MbedTLS crypto library.
  

+config SHA384_MBEDTLS

•   bool "Enable SHA384 support with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SHA384
  

•   select SHA512_MBEDTLS
  

•   help
  

•     This option enables support of hashing using SHA384 algorithm
  

•     with MbedTLS crypto library.
  

+config MD5_MBEDTLS

•   bool "Enable MD5 support with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && MD5
  

•   help
  

•     This option enables support of hashing using MD5 algorithm
  

•     with MbedTLS crypto library.
  

+if SPL

+config SPL_SHA1_MBEDTLS

•   bool "Enable SHA1 support in SPL with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SPL_SHA1
  

•   default y if SHA1 && MBEDTLS_LIB_CRYPTO
  

•   help
  

•     This option enables support of hashing using SHA1 algorithm
  

•     with MbedTLS crypto library.
  

+config SPL_SHA256_MBEDTLS

•   bool "Enable SHA256 support in SPL with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SPL_SHA256
  

•   default y if SHA256 && MBEDTLS_LIB_CRYPTO
  

•   help
  

•     This option enables support of hashing using SHA256 algorithm
  

•     with MbedTLS crypto library.
  

+config SPL_SHA512_MBEDTLS

•   bool "Enable SHA512 support in SPL with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SPL_SHA512
  

•   default y if SHA512 && MBEDTLS_LIB_CRYPTO
  

•   help
  

•     This option enables support of hashing using SHA512 algorithm
  

•     with MbedTLS crypto library.
  

+config SPL_SHA384_MBEDTLS

•   bool "Enable SHA384 support in SPL with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SPL_SHA384
  

•   default y if SHA384 && MBEDTLS_LIB_CRYPTO
  

•   select SPL_SHA512
  

•   help
  

•     This option enables support of hashing using SHA384 algorithm
  

•     with MbedTLS crypto library.
  

+config SPL_MD5_MBEDTLS

•   bool "Enable MD5 support in SPL with MbedTLS crypto library"
  

•   depends on MBEDTLS_LIB_CRYPTO && SPL_MD5
  

•   default y if MD5 && MBEDTLS_LIB_CRYPTO
  

•   help
  

•     This option enables support of hashing using MD5 algorithm
  

•     with MbedTLS crypto library.
  

+endif # SPL

+endif # MBEDTLS_LIB_CRYPTO

config MBEDTLS_LIB_X509 bool "MbedTLS certificate libraries" diff --git a/lib/mbedtls/Makefile b/lib/mbedtls/Makefile index 803ea0b62a0..32a98b7f4ca 100644 --- a/lib/mbedtls/Makefile +++ b/lib/mbedtls/Makefile @@ -13,17 +13,24 @@ ccflags-y += \ -I$(src)/external/mbedtls/include \ -I$(src)/external/mbedtls/library

+# shim layer for hash +obj-$(CONFIG_MBEDTLS_LIB_CRYPTO) += hash_mbedtls.o +hash_mbedtls-$(CONFIG_$(SPL_)MD5_MBEDTLS) += md5.o +hash_mbedtls-$(CONFIG_$(SPL_)SHA1_MBEDTLS) += sha1.o +hash_mbedtls-$(CONFIG_$(SPL_)SHA256_MBEDTLS) += sha256.o +hash_mbedtls-$(CONFIG_$(SPL_)SHA512_MBEDTLS) += sha512.o

# MbedTLS crypto library obj-$(CONFIG_MBEDTLS_LIB_CRYPTO) += mbedtls_lib_crypto.o mbedtls_lib_crypto-y += \ $(MBEDTLS_LIB_DIR)/platform_util.o \ $(MBEDTLS_LIB_DIR)/constant_time.o \ $(MBEDTLS_LIB_DIR)/md.o -mbedtls_lib_crypto-$(CONFIG_$(SPL_)MD5) += $(MBEDTLS_LIB_DIR)/md5.o -mbedtls_lib_crypto-$(CONFIG_$(SPL_)SHA1) += $(MBEDTLS_LIB_DIR)/sha1.o -mbedtls_lib_crypto-$(CONFIG_$(SPL_)SHA256) += \ +mbedtls_lib_crypto-$(CONFIG_$(SPL_)MD5_MBEDTLS) +=

$(MBEDTLS_LIB_DIR)/md5.o

...

+mbedtls_lib_crypto-$(CONFIG_$(SPL_)SHA1_MBEDTLS) +=

$(MBEDTLS_LIB_DIR)/sha1.o

...

+mbedtls_lib_crypto-$(CONFIG_$(SPL_)SHA256_MBEDTLS) += \ $(MBEDTLS_LIB_DIR)/sha256.o -mbedtls_lib_crypto-$(CONFIG_$(SPL_)SHA512) += \ +mbedtls_lib_crypto-$(CONFIG_$(SPL_)SHA512_MBEDTLS) += \ $(MBEDTLS_LIB_DIR)/sha512.o

# MbedTLS X509 library diff --git a/lib/mbedtls/md5.c b/lib/mbedtls/md5.c new file mode 100644 index 00000000000..04388fce249 --- /dev/null +++ b/lib/mbedtls/md5.c @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Hash shim layer on MbedTLS Crypto library
• • Copyright (c) 2024 Linaro Limited
• • Author: Raymond Mao raymond.mao@linaro.org
• */

+#include "compiler.h"

+#ifndef USE_HOSTCC +#include <watchdog.h> +#endif /* USE_HOSTCC */ +#include <u-boot/md5.h>

+void MD5Init(MD5Context *ctx) +{

•   mbedtls_md5_init(ctx);
  

•   mbedtls_md5_starts(ctx);
  

+}

+void MD5Update(MD5Context *ctx, unsigned char const *buf, unsigned int

len)

...

+{

•   mbedtls_md5_update(ctx, buf, len);
  

+}

+void MD5Final(unsigned char digest[16], MD5Context *ctx) +{

•   mbedtls_md5_finish(ctx, digest);
  

•   mbedtls_md5_free(ctx);
  

+}

+void md5_wd(const unsigned char *input, unsigned int len,

•       unsigned char output[16], unsigned int chunk_sz)
  

+{

•   MD5Context context;
  

•   MD5Init(&context);
  

•   if (IS_ENABLED(CONFIG_HW_WATCHDOG) ||
  

IS_ENABLED(CONFIG_WATCHDOG)) {

...

•           const unsigned char *curr = input;
  

•           const unsigned char *end = input + len;
  

•           int chunk;
  

•           while (curr < end) {
  

•                   chunk = end - curr;
  

•                   if (chunk > chunk_sz)
  

•                           chunk = chunk_sz;
  

•                   MD5Update(&context, curr, chunk);
  

•                   curr += chunk;
  

•                   schedule();
  

•           }
  

•   } else {
  

•           MD5Update(&context, input, len);
  

•   }
  

You can probably split this part to a function, that takes the hashing algo as an argument, since the loop is identical for md5, sha1, sha256 etc

I think this change does not bring too much value.

They are calling different "_update()" functions and we need to switch the algorithm to select which one should be called. Other than that, we need to create a new file (e.g. md_common.c) to contain this new common function. As a trade-off, I prefer to keep the current implementation.

[snip]

Regards, Raymond

Hi Raymond

On Tue, 2 Jul 2024 at 21:27, Raymond Mao raymond.mao@linaro.org wrote:

Integrate common/hash.c on the hash shim layer so that hash APIs from mbedtls can be leveraged by boot/image and efi_loader.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v2

• Use the original head files instead of creating new ones.

Changes in v3

• Add handle checkers for malloc.

Changes in v4

• None.

common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 143 insertions(+)

diff --git a/common/hash.c b/common/hash.c index ac63803fed9..96caf074374 100644 --- a/common/hash.c +++ b/common/hash.c @@ -35,6 +35,141 @@ #include <u-boot/sha512.h> #include <u-boot/md5.h>

+#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO)

+static int hash_init_sha1(struct hash_algo *algo, void **ctxp) +{

•   int ret;
  

•   mbedtls_sha1_context *ctx = malloc(sizeof(mbedtls_sha1_context));
  

•   if (!ctx)
  

•           return -ENOMEM;
  

•   mbedtls_sha1_init(ctx);
  

•   ret = mbedtls_sha1_starts(ctx);
  

•   if (!ret) {
  

•           *ctxp = ctx;
  

•   } else {
  

•           mbedtls_sha1_free(ctx);
  

•           free(ctx);
  

•   }
  

•   return ret;
  

+}

+static int hash_update_sha1(struct hash_algo *algo, void *ctx, const void *buf,

•                       unsigned int size, int is_last)
  

+{

•   return mbedtls_sha1_update((mbedtls_sha1_context *)ctx, buf, size);
  

+}

+static int +hash_finish_sha1(struct hash_algo *algo, void *ctx, void *dest_buf, int size) +{

•   int ret;
  

•   if (size < algo->digest_size)
  

•           return -1;
  

•   ret = mbedtls_sha1_finish((mbedtls_sha1_context *)ctx, dest_buf);
  

•   if (!ret) {
  

•           mbedtls_sha1_free((mbedtls_sha1_context *)ctx);
  

•           free(ctx);
  

•   }
  

•   return ret;
  

+}

+static int hash_init_sha256(struct hash_algo *algo, void **ctxp) +{

•   int ret;
  

•   int is224 = algo->digest_size == SHA224_SUM_LEN ? 1 : 0;
  

•   mbedtls_sha256_context *ctx = malloc(sizeof(mbedtls_sha256_context));
  

I prefer sizeof(*ctx) on all the allocations. Also since it's a crypto stack I prefer a calloc (sha384 for example will allocate more bytes)

•   if (!ctx)
  

•           return -ENOMEM;
  

•   mbedtls_sha256_init(ctx);
  

•   ret = mbedtls_sha256_starts(ctx, is224);
  

•   if (!ret) {
  

•           *ctxp = ctx;
  

•   } else {
  

•           mbedtls_sha256_free(ctx);
  

•           free(ctx);
  

•   }
  

•   return ret;
  

+}

+static int hash_update_sha256(struct hash_algo *algo, void *ctx, const void *buf,

•                         uint size, int is_last)
  

+{

•   return mbedtls_sha256_update((mbedtls_sha256_context *)ctx, buf, size);
  

+}

+static int +hash_finish_sha256(struct hash_algo *algo, void *ctx, void *dest_buf, int size) +{

•   int ret;
  

•   if (size < algo->digest_size)
  

•           return -1;
  

•   ret = mbedtls_sha256_finish((mbedtls_sha256_context *)ctx, dest_buf);
  

•   if (!ret) {
  

•           mbedtls_sha256_free((mbedtls_sha256_context *)ctx);
  

•           free(ctx);
  

•   }
  

•   return ret;
  

+}

+static int hash_init_sha512(struct hash_algo *algo, void **ctxp) +{

•   int ret;
  

•   int is384 = algo->digest_size == SHA384_SUM_LEN ? 1 : 0;
  

•   mbedtls_sha512_context *ctx = malloc(sizeof(mbedtls_sha512_context));
  

•   if (!ctx)
  

•           return -ENOMEM;
  

•   mbedtls_sha512_init(ctx);
  

•   ret = mbedtls_sha512_starts(ctx, is384);
  

•   if (!ret) {
  

•           *ctxp = ctx;
  

•   } else {
  

•           mbedtls_sha512_free(ctx);
  

•           free(ctx);
  

•   }
  

•   return ret;
  

+}

+static int hash_update_sha512(struct hash_algo *algo, void *ctx, const void *buf,

•                         uint size, int is_last)
  

+{

•   return mbedtls_sha512_update((mbedtls_sha512_context *)ctx, buf, size);
  

+}

+static int +hash_finish_sha512(struct hash_algo *algo, void *ctx, void *dest_buf, int size) +{

•   int ret;
  

•   if (size < algo->digest_size)
  

•           return -1;
  

•   ret = mbedtls_sha512_finish((mbedtls_sha512_context *)ctx, dest_buf);
  

•   if (!ret) {
  

•           mbedtls_sha512_free((mbedtls_sha512_context *)ctx);
  

•           free(ctx);
  

•   }
  

•   return ret;
  

+}

+#else /* CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) */

static int __maybe_unused hash_init_sha1(struct hash_algo *algo, void **ctxp) { sha1_context *ctx = malloc(sizeof(sha1_context)); @@ -143,6 +278,8 @@ static int __maybe_unused hash_finish_sha512(struct hash_algo *algo, void *ctx, return 0; }

+#endif /* CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) */

static int hash_init_crc16_ccitt(struct hash_algo *algo, void **ctxp) { uint16_t *ctx = malloc(sizeof(uint16_t)); @@ -267,10 +404,16 @@ static struct hash_algo hash_algo[] = { .hash_init = hw_sha_init, .hash_update = hw_sha_update, .hash_finish = hw_sha_finish, +#else +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO)

•           .hash_init      = hash_init_sha512,
  

•           .hash_update    = hash_update_sha512,
  

•           .hash_finish    = hash_finish_sha512,
  

I don't have strong opinion here, but you could alternatively define sha384 wrappers for mbedTLS and avoid this ifdef. In any case I personally don't mind.

#else .hash_init = hash_init_sha384, .hash_update = hash_update_sha384, .hash_finish = hash_finish_sha384, +#endif #endif },

#endif

2.25.1

Hi Simon,

On Fri, 5 Jul 2024 at 04:36, Simon Glass sjg@chromium.org wrote:

Hi,

On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

Hi Raymond

On Tue, 2 Jul 2024 at 21:27, Raymond Mao raymond.mao@linaro.org wrote:

...

Integrate common/hash.c on the hash shim layer so that hash APIs from mbedtls can be leveraged by boot/image and efi_loader.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v2

• Use the original head files instead of creating new ones.

Changes in v3

• Add handle checkers for malloc.

Changes in v4

• None.

common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 143 insertions(+)

diff --git a/common/hash.c b/common/hash.c index ac63803fed9..96caf074374 100644 --- a/common/hash.c +++ b/common/hash.c @@ -35,6 +35,141 @@ #include <u-boot/sha512.h> #include <u-boot/md5.h>

+#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO)

+static int hash_init_sha1(struct hash_algo *algo, void **ctxp) +{

•   int ret;
  

•   mbedtls_sha1_context *ctx =
  

malloc(sizeof(mbedtls_sha1_context));

Why do we need allocation here? We should avoid it where possible.

The API "hash_init_sha1(struct hash_algo *algo, void **ctxp)" is passing a

pointer address and expecting to get the context from the pointer, it is reasonable to do the allocation. On top of that, this patch doesn't make changes on this API itself, but just adapted it to MbedTLS stacks, thus you can see the allocation is needed by the original API as well.

Regards, Raymond

On Fri, Jul 19, 2024 at 04:05:09PM +0100, Simon Glass wrote:

Hi Raymond,

On Thu, 18 Jul 2024 at 17:46, Raymond Mao raymond.mao@linaro.org wrote:

...

Hi Simon,

On Fri, 5 Jul 2024 at 04:36, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

Hi Raymond

On Tue, 2 Jul 2024 at 21:27, Raymond Mao raymond.mao@linaro.org wrote:

...

Integrate common/hash.c on the hash shim layer so that hash APIs from mbedtls can be leveraged by boot/image and efi_loader.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v2

• Use the original head files instead of creating new ones.

Changes in v3

• Add handle checkers for malloc.

Changes in v4

• None.

common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 143 insertions(+)

diff --git a/common/hash.c b/common/hash.c index ac63803fed9..96caf074374 100644 --- a/common/hash.c +++ b/common/hash.c @@ -35,6 +35,141 @@ #include <u-boot/sha512.h> #include <u-boot/md5.h>

+#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO)

+static int hash_init_sha1(struct hash_algo *algo, void **ctxp) +{

•   int ret;
  

•   mbedtls_sha1_context *ctx = malloc(sizeof(mbedtls_sha1_context));
  

Why do we need allocation here? We should avoid it where possible.

The API "hash_init_sha1(struct hash_algo *algo, void **ctxp)" is passing a pointer address and expecting to get the context from the pointer, it is reasonable to do the allocation. On top of that, this patch doesn't make changes on this API itself, but just adapted it to MbedTLS stacks, thus you can see the allocation is needed by the original API as well.

Oh dear., I see Now I am looking at the code. It is full of #ifdefs for different cases.

The whole thing needs a bit of a rationalisation before adding another case.

If you're referring too the hash_algo struct, I'm not sure we can do something different that doesn't in turn increase size globally. And long term some of this may be able to go away if we can remove non-mbedTLS options.

On Sat, Jul 20, 2024 at 01:36:02PM +0100, Simon Glass wrote:

Hi Tom,

On Fri, 19 Jul 2024 at 16:25, Tom Rini trini@konsulko.com wrote:

...

On Fri, Jul 19, 2024 at 04:05:09PM +0100, Simon Glass wrote:

...

Hi Raymond,

On Thu, 18 Jul 2024 at 17:46, Raymond Mao raymond.mao@linaro.org wrote:

...

Hi Simon,

On Fri, 5 Jul 2024 at 04:36, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

...

Hi Raymond

On Tue, 2 Jul 2024 at 21:27, Raymond Mao raymond.mao@linaro.org wrote: > > Integrate common/hash.c on the hash shim layer so that hash APIs > from mbedtls can be leveraged by boot/image and efi_loader. > > Signed-off-by: Raymond Mao raymond.mao@linaro.org > --- > Changes in v2 > - Use the original head files instead of creating new ones. > Changes in v3 > - Add handle checkers for malloc. > Changes in v4 > - None. > > common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 143 insertions(+) > > diff --git a/common/hash.c b/common/hash.c > index ac63803fed9..96caf074374 100644 > --- a/common/hash.c > +++ b/common/hash.c > @@ -35,6 +35,141 @@ > #include <u-boot/sha512.h> > #include <u-boot/md5.h> > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > + > +static int hash_init_sha1(struct hash_algo *algo, void **ctxp) > +{ > + int ret; > + mbedtls_sha1_context *ctx = malloc(sizeof(mbedtls_sha1_context));

Why do we need allocation here? We should avoid it where possible.

The API "hash_init_sha1(struct hash_algo *algo, void **ctxp)" is passing a pointer address and expecting to get the context from the pointer, it is reasonable to do the allocation. On top of that, this patch doesn't make changes on this API itself, but just adapted it to MbedTLS stacks, thus you can see the allocation is needed by the original API as well.

Oh dear., I see Now I am looking at the code. It is full of #ifdefs for different cases.

The whole thing needs a bit of a rationalisation before adding another case.

If you're referring too the hash_algo struct, I'm not sure we can do something different that doesn't in turn increase size globally. And long term some of this may be able to go away if we can remove non-mbedTLS options.

Well, at least using the existing functions rather than writing entirely new ones would help.

The real culprit here is the hardware-acceleration stuff, but making the software side messy too is not nice. Hashing should move to a linker-list approach for the implementation, and probably driver model for the hardware acceleration.

I'm fine with looking at that, after mbedtls is merged and we're looking at what can be removed / cleaned up. Perhaps we'll be able to shift most/all of the hardware assisted algorithms to being handled within mbedtls, I don't know.

Hi Tom,

On Sat, 20 Jul 2024 at 13:13, Tom Rini trini@konsulko.com wrote:

On Sat, Jul 20, 2024 at 01:36:02PM +0100, Simon Glass wrote:

...

Hi Tom,

On Fri, 19 Jul 2024 at 16:25, Tom Rini trini@konsulko.com wrote:

...

On Fri, Jul 19, 2024 at 04:05:09PM +0100, Simon Glass wrote:

...

Hi Raymond,

On Thu, 18 Jul 2024 at 17:46, Raymond Mao raymond.mao@linaro.org

wrote:

...

...

...

...

Hi Simon,

On Fri, 5 Jul 2024 at 04:36, Simon Glass sjg@chromium.org wrote:

...

Hi,

On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas <

ilias.apalodimas@linaro.org> wrote:

...

...

...

...

...

> > Hi Raymond > > On Tue, 2 Jul 2024 at 21:27, Raymond Mao <

raymond.mao@linaro.org> wrote:

...

...

...

...

...

> > > > Integrate common/hash.c on the hash shim layer so that hash

APIs

...

...

...

...

...

> > from mbedtls can be leveraged by boot/image and efi_loader. > > > > Signed-off-by: Raymond Mao raymond.mao@linaro.org > > --- > > Changes in v2 > > - Use the original head files instead of creating new ones. > > Changes in v3 > > - Add handle checkers for malloc. > > Changes in v4 > > - None. > > > > common/hash.c | 143

++++++++++++++++++++++++++++++++++++++++++++++++++

...

...

...

...

...

> > 1 file changed, 143 insertions(+) > > > > diff --git a/common/hash.c b/common/hash.c > > index ac63803fed9..96caf074374 100644 > > --- a/common/hash.c > > +++ b/common/hash.c > > @@ -35,6 +35,141 @@ > > #include <u-boot/sha512.h> > > #include <u-boot/md5.h> > > > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > > + > > +static int hash_init_sha1(struct hash_algo *algo, void

**ctxp)

...

...

...

...

...

> > +{ > > + int ret; > > + mbedtls_sha1_context *ctx =

malloc(sizeof(mbedtls_sha1_context));

...

...

...

...

...

Why do we need allocation here? We should avoid it where possible.

The API "hash_init_sha1(struct hash_algo *algo, void **ctxp)" is

passing a pointer

...

...

...

...

address and expecting to get the context from the pointer, it is

reasonable to do the

...

...

...

...

allocation. On top of that, this patch doesn't make changes on this API

itself, but just adapted

...

...

...

...

it to MbedTLS stacks, thus you can see the allocation is needed by

the original API

...

...

...

...

as well.

Oh dear., I see Now I am looking at the code. It is full of #ifdefs for different cases.

The whole thing needs a bit of a rationalisation before adding

another case.

...

...

If you're referring too the hash_algo struct, I'm not sure we can do something different that doesn't in turn increase size globally. And long term some of this may be able to go away if we can remove non-mbedTLS options.

Well, at least using the existing functions rather than writing entirely new ones would help.

The real culprit here is the hardware-acceleration stuff, but making the software side messy too is not nice. Hashing should move to a linker-list approach for the implementation, and probably driver model for the hardware acceleration.

I'm fine with looking at that, after mbedtls is merged and we're looking at what can be removed / cleaned up. Perhaps we'll be able to shift most/all of the hardware assisted algorithms to being handled within mbedtls, I don't know.

Yes, I think to handle this within mbedtls is straight forward.

Regards, Raymond

Hi Ilias,

On Wed, 3 Jul 2024 at 04:56, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

Hi Raymond

On Tue, 2 Jul 2024 at 21:27, Raymond Mao raymond.mao@linaro.org wrote:

...

Integrate common/hash.c on the hash shim layer so that hash APIs from mbedtls can be leveraged by boot/image and efi_loader.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v2

• Use the original head files instead of creating new ones.

Changes in v3

• Add handle checkers for malloc.

Changes in v4

• None.

common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 143 insertions(+)

diff --git a/common/hash.c b/common/hash.c index ac63803fed9..96caf074374 100644 --- a/common/hash.c +++ b/common/hash.c

[snip]

...

@@ -267,10 +404,16 @@ static struct hash_algo hash_algo[] = { .hash_init = hw_sha_init, .hash_update = hw_sha_update, .hash_finish = hw_sha_finish, +#else +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO)

•           .hash_init      = hash_init_sha512,
  

•           .hash_update    = hash_update_sha512,
  

•           .hash_finish    = hash_finish_sha512,
  

I don't have strong opinion here, but you could alternatively define sha384 wrappers for mbedTLS and avoid this ifdef. In any case I personally don't mind.

I will prefer not to create a new wrapper function in order to save

space.

Regards, Raymond

On Tue, 2 Jul 2024 at 21:27, Raymond Mao raymond.mao@linaro.org wrote:

Add the mbedtls include directories into the build system.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v2

• None.

Changes in v3

• Remove changes for PLATFORM_CPPFLAGS.

Changes in v4

• Fix errors when building without "O=".
• Minor fix of the include directories.

Makefile | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/Makefile b/Makefile index 07d7947c8af..fd855dbd5c9 100644 --- a/Makefile +++ b/Makefile @@ -829,6 +829,12 @@ KBUILD_HOSTCFLAGS += $(if $(CONFIG_TOOLS_DEBUG),-g) UBOOTINCLUDE := \ -Iinclude \ $(if $(KBUILD_SRC), -I$(srctree)/include) \

•   $(if $(CONFIG_MBEDTLS_LIB), \
  

•           "-DMBEDTLS_CONFIG_FILE=\"mbedtls_def_config.h\"" \
  

don't you need a full path to the config file?

•           -I$(srctree)/lib/mbedtls \
  

•           -I$(srctree)/lib/mbedtls/port \
  

•           -I$(srctree)/lib/mbedtls/external/mbedtls \
  

•           -I$(srctree)/lib/mbedtls/external/mbedtls/include) \
  

Most of these paths are explicitly added in patch #3. Why don't we just keep them there?

Thanks /Ilias

    $(if $(CONFIG_$(SPL_)SYS_THUMB_BUILD), \
            $(if $(CONFIG_HAS_THUMB2), \
                    $(if $(CONFIG_CPU_V7M), \

-- 2.25.1

Hi Ilias,

On Wed, 3 Jul 2024 at 07:35, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

On Tue, 2 Jul 2024 at 21:27, Raymond Mao raymond.mao@linaro.org wrote:

...

Add the mbedtls include directories into the build system.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v2

• None.

Changes in v3

• Remove changes for PLATFORM_CPPFLAGS.

Changes in v4

• Fix errors when building without "O=".
• Minor fix of the include directories.

Makefile | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/Makefile b/Makefile index 07d7947c8af..fd855dbd5c9 100644 --- a/Makefile +++ b/Makefile @@ -829,6 +829,12 @@ KBUILD_HOSTCFLAGS += $(if $(CONFIG_TOOLS_DEBUG),-g) UBOOTINCLUDE := \ -Iinclude \ $(if $(KBUILD_SRC), -I$(srctree)/include) \

•   $(if $(CONFIG_MBEDTLS_LIB), \
  

•           "-DMBEDTLS_CONFIG_FILE=\"mbedtls_def_config.h\"" \
  

don't you need a full path to the config file?

No, we don't need the full path. Since "$(srctree)/lib/mbedtls" is in the include directories, and the this filename is unique in the project.

Regards, Raymond

Hi Ilias,

On Tue, 23 Jul 2024 at 03:45, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

On Thu, 18 Jul 2024 at 23:12, Raymond Mao raymond.mao@linaro.org wrote:

...

Hi Ilias,

On Wed, 3 Jul 2024 at 07:35, Ilias Apalodimas <

ilias.apalodimas@linaro.org> wrote:

...

...

On Tue, 2 Jul 2024 at 21:27, Raymond Mao raymond.mao@linaro.org

wrote:

...

...

...

Add the mbedtls include directories into the build system.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v2

• None.

Changes in v3

• Remove changes for PLATFORM_CPPFLAGS.

Changes in v4

• Fix errors when building without "O=".
• Minor fix of the include directories.

Makefile | 6 ++++++ 1 file changed, 6 insertions(+)

diff --git a/Makefile b/Makefile index 07d7947c8af..fd855dbd5c9 100644 --- a/Makefile +++ b/Makefile @@ -829,6 +829,12 @@ KBUILD_HOSTCFLAGS += $(if

$(CONFIG_TOOLS_DEBUG),-g)

...

...

...

UBOOTINCLUDE := \ -Iinclude \ $(if $(KBUILD_SRC), -I$(srctree)/include) \

•   $(if $(CONFIG_MBEDTLS_LIB), \
  

•           "-DMBEDTLS_CONFIG_FILE=\"mbedtls_def_config.h\"" \
  

don't you need a full path to the config file?

No, we don't need the full path. Since "$(srctree)/lib/mbedtls" is in the include directories, and the

this filename is unique

...

in the project.

Then why are they needed on patch #3? Don't we include them already in the generic Makefile?

I will remove the ones from #3, and then squash this patch with #3.

Regards, Raymond

Hi Raymond

On Tue, 2 Jul 2024 at 21:29, Raymond Mao raymond.mao@linaro.org wrote:

Move public_key_free and public_key_signature_free as helper functions that can be shared by legacy crypto lib and MbedTLS implementation.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v4

• Initial patch.

lib/crypto/Makefile | 4 +++- lib/crypto/public_key.c | 31 ------------------------- lib/crypto/public_key_helper.c | 42 ++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+), 32 deletions(-) create mode 100644 lib/crypto/public_key_helper.c

diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile index bec1bc95a65..4ad1849040d 100644 --- a/lib/crypto/Makefile +++ b/lib/crypto/Makefile @@ -7,7 +7,9 @@ obj-$(CONFIG_$(SPL_)ASYMMETRIC_KEY_TYPE) += asymmetric_keys.o

asymmetric_keys-y := asymmetric_type.o

-obj-$(CONFIG_$(SPL_)ASYMMETRIC_PUBLIC_KEY_SUBTYPE) += public_key.o +obj-$(CONFIG_$(SPL_)ASYMMETRIC_PUBLIC_KEY_SUBTYPE) += \

•   public_key_helper.o \
  

•   public_key.o
  
  

# # RSA public key parser diff --git a/lib/crypto/public_key.c b/lib/crypto/public_key.c index 6efe951c057..408742907f1 100644 --- a/lib/crypto/public_key.c +++ b/lib/crypto/public_key.c @@ -51,38 +51,7 @@ static void public_key_describe(const struct key *asymmetric_key, } #endif

-/*

• • Destroy a public key algorithm key.
• */

-void public_key_free(struct public_key *key) -{

•   if (key) {
  

•           kfree(key->key);
  

•           kfree(key->params);
  

•           kfree(key);
  

•   }
  

-} -EXPORT_SYMBOL_GPL(public_key_free);

#ifdef __UBOOT__ -/*

• • from <linux>/crypto/asymmetric_keys/signature.c
• • Destroy a public key signature.
• */

-void public_key_signature_free(struct public_key_signature *sig) -{

•   int i;
  

•   if (sig) {
  

•           for (i = 0; i < ARRAY_SIZE(sig->auth_ids); i++)
  

•                   free(sig->auth_ids[i]);
  

•           free(sig->s);
  

•           free(sig->digest);
  

•           free(sig);
  

•   }
  

-} -EXPORT_SYMBOL_GPL(public_key_signature_free);

/**

• public_key_verify_signature - Verify a signature using a public key.

diff --git a/lib/crypto/public_key_helper.c b/lib/crypto/public_key_helper.c new file mode 100644 index 00000000000..4cb21edddf3 --- /dev/null +++ b/lib/crypto/public_key_helper.c @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • X509 helper functions
• • Copyright (c) 2012 Red Hat, Inc. All Rights Reserved.
• • Written by David Howells (dhowells@redhat.com)
• • Copyright (c) 2024 Linaro Limited
• • Author: Raymond Mao raymond.mao@linaro.org

Similar comments here, the patch is ok, just remove the copyright and author

• */

+#include <linux/compat.h> +#include <crypto/public_key.h>

+/*

• • Destroy a public key algorithm key.
• */

+void public_key_free(struct public_key *key) +{

•   if (key) {
  

•           kfree(key->key);
  

•           kfree(key->params);
  

•           kfree(key);
  

•   }
  

+}

+/*

• • from <linux>/crypto/asymmetric_keys/signature.c
• • Destroy a public key signature.
• */

+void public_key_signature_free(struct public_key_signature *sig) +{

•   int i;
  

•   if (sig) {
  

•           for (i = 0; i < ARRAY_SIZE(sig->auth_ids); i++)
  

•                   kfree(sig->auth_ids[i]);
  

•           kfree(sig->s);
  

•           kfree(sig->digest);
  

•           kfree(sig);
  

•   }
  

+}

2.25.1

With the above fixed Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On Tue, 2 Jul 2024 at 21:29, Raymond Mao raymond.mao@linaro.org wrote:

Move x509_check_for_self_signed as a common helper function that can be shared by legacy crypto lib and MbedTLS implementation.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v4

• Initial patch.

lib/crypto/Makefile | 1 + lib/crypto/x509_helper.c | 67 ++++++++++++++++++++++++++++++++++++ lib/crypto/x509_public_key.c | 56 +----------------------------- 3 files changed, 69 insertions(+), 55 deletions(-) create mode 100644 lib/crypto/x509_helper.c

diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile index 4ad1849040d..946cc3a7b59 100644 --- a/lib/crypto/Makefile +++ b/lib/crypto/Makefile @@ -37,6 +37,7 @@ x509_key_parser-y := \ x509.asn1.o \ x509_akid.asn1.o \ x509_cert_parser.o \

•   x509_helper.o \
    x509_public_key.o
  
  

$(obj)/x509_cert_parser.o: \ diff --git a/lib/crypto/x509_helper.c b/lib/crypto/x509_helper.c new file mode 100644 index 00000000000..d0c80907ec3 --- /dev/null +++ b/lib/crypto/x509_helper.c @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • X509 helper functions
• • Copyright (C) 2012 Red Hat, Inc. All Rights Reserved.
• • Written by David Howells (dhowells@redhat.com)
• • Copyright (c) 2024 Linaro Limited
• • Author: Raymond Mao raymond.mao@linaro.org

Same comment with your other patches moving code around. You haven't contributed a substantial amount of code here to justify the copyright and authorship

• */

+#include <linux/err.h> +#include <crypto/public_key.h> +#include <crypto/x509_parser.h>

+/*

• • Check for self-signedness in an X.509 cert and if found, check the signature
• • immediately if we can.
• */

+int x509_check_for_self_signed(struct x509_certificate *cert) +{

•   int ret = 0;
  

•   if (cert->raw_subject_size != cert->raw_issuer_size ||
  

•       memcmp(cert->raw_subject, cert->raw_issuer,
  

•              cert->raw_issuer_size))
  

•           goto not_self_signed;
  

•   if (cert->sig->auth_ids[0] || cert->sig->auth_ids[1]) {
  

•           /*
  

•            * If the AKID is present it may have one or two parts. If
  

•            * both are supplied, both must match.
  

•            */
  

•           bool a = asymmetric_key_id_same(cert->skid,
  

•                                           cert->sig->auth_ids[1]);
  

•           bool b = asymmetric_key_id_same(cert->id,
  

•                                           cert->sig->auth_ids[0]);
  

•           if (!a && !b)
  

•                   goto not_self_signed;
  

•           ret = -EKEYREJECTED;
  

•           if (((a && !b) || (b && !a)) &&
  

•               cert->sig->auth_ids[0] && cert->sig->auth_ids[1])
  

•                   goto out;
  

•   }
  

•   ret = -EKEYREJECTED;
  

•   if (strcmp(cert->pub->pkey_algo, cert->sig->pkey_algo))
  

•           goto out;
  

•   ret = public_key_verify_signature(cert->pub, cert->sig);
  

•   if (ret == -ENOPKG) {
  

•           cert->unsupported_sig = true;
  

•           goto not_self_signed;
  

•   }
  

•   if (ret < 0)
  

•           goto out;
  

•   pr_devel("Cert Self-signature verified");
  

•   cert->self_signed = true;
  

+out:

•   return ret;
  

+not_self_signed:

•   return 0;
  

+} diff --git a/lib/crypto/x509_public_key.c b/lib/crypto/x509_public_key.c index a10145a7cdc..4ba13c1adc3 100644 --- a/lib/crypto/x509_public_key.c +++ b/lib/crypto/x509_public_key.c @@ -139,61 +139,7 @@ error: return ret; }

-/*

• • Check for self-signedness in an X.509 cert and if found, check the signature
• • immediately if we can.
• */

-int x509_check_for_self_signed(struct x509_certificate *cert) -{

•   int ret = 0;
  

•   pr_devel("==>%s()\n", __func__);
  

•   if (cert->raw_subject_size != cert->raw_issuer_size ||
  

•       memcmp(cert->raw_subject, cert->raw_issuer,
  

•              cert->raw_issuer_size) != 0)
  

•           goto not_self_signed;
  

•   if (cert->sig->auth_ids[0] || cert->sig->auth_ids[1]) {
  

•           /* If the AKID is present it may have one or two parts.  If
  

•            * both are supplied, both must match.
  

•            */
  

•           bool a = asymmetric_key_id_same(cert->skid, cert->sig->auth_ids[1]);
  

•           bool b = asymmetric_key_id_same(cert->id, cert->sig->auth_ids[0]);
  

•           if (!a && !b)
  

•                   goto not_self_signed;
  

•           ret = -EKEYREJECTED;
  

•           if (((a && !b) || (b && !a)) &&
  

•               cert->sig->auth_ids[0] && cert->sig->auth_ids[1])
  

•                   goto out;
  

•   }
  

•   ret = -EKEYREJECTED;
  

•   if (strcmp(cert->pub->pkey_algo, cert->sig->pkey_algo) != 0)
  

•           goto out;
  

•   ret = public_key_verify_signature(cert->pub, cert->sig);
  

•   if (ret < 0) {
  

•           if (ret == -ENOPKG) {
  

•                   cert->unsupported_sig = true;
  

•                   ret = 0;
  

•           }
  

•           goto out;
  

•   }
  

•   pr_devel("Cert Self-signature verified");
  

•   cert->self_signed = true;
  

-out:

•   pr_devel("<==%s() = %d\n", __func__, ret);
  

•   return ret;
  

-not_self_signed:

•   pr_devel("<==%s() = 0 [not]\n", __func__);
  

•   return 0;
  

-} +#endif /* !CONFIG_IS_ENABLED(MBEDTLS_LIB_X509) */

#ifndef __UBOOT__ /* -- 2.25.1

with the above fixed Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Hi Raymond

On Tue, 2 Jul 2024 at 21:30, Raymond Mao raymond.mao@linaro.org wrote:

Move pkcs7_get_content_data as a helper function that can be shared by legacy crypto lib and MbedTLS implementation.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v4

• Initial patch.

lib/crypto/Makefile | 1 + lib/crypto/pkcs7_helper.c | 40 +++++++++++++++++++++++++++++++++++++++ lib/crypto/pkcs7_parser.c | 28 --------------------------- 3 files changed, 41 insertions(+), 28 deletions(-) create mode 100644 lib/crypto/pkcs7_helper.c

diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile index 946cc3a7b59..16059088f26 100644 --- a/lib/crypto/Makefile +++ b/lib/crypto/Makefile @@ -53,6 +53,7 @@ $(obj)/x509_akid.asn1.o: $(obj)/x509_akid.asn1.c $(obj)/x509_akid.asn1.h obj-$(CONFIG_$(SPL_)PKCS7_MESSAGE_PARSER) += pkcs7_message.o pkcs7_message-y := \ pkcs7.asn1.o \

•   pkcs7_helper.o \
    pkcs7_parser.o
  

obj-$(CONFIG_$(SPL_)PKCS7_VERIFY) += pkcs7_verify.o

diff --git a/lib/crypto/pkcs7_helper.c b/lib/crypto/pkcs7_helper.c new file mode 100644 index 00000000000..6c8dcd1a935 --- /dev/null +++ b/lib/crypto/pkcs7_helper.c @@ -0,0 +1,40 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • PKCS7 helper functions
• • Copyright (c) 2012 Red Hat, Inc. All Rights Reserved.
• • Written by David Howells (dhowells@redhat.com)
• • Copyright (c) 2024 Linaro Limited
• • Author: Raymond Mao raymond.mao@linaro.org

Copyright and authorship is added if you add a significant portion of the file. You are moving a function around here, so just keep the original author & copyright

• */

+#include <linux/kernel.h> +#include <linux/err.h> +#include <crypto/pkcs7_parser.h>

+/**

• • pkcs7_get_content_data - Get access to the PKCS#7 content
• • @pkcs7: The preparsed PKCS#7 message to access
• • @_data: Place to return a pointer to the data
• • @_data_len: Place to return the data length
• • @_headerlen: Size of ASN.1 header not included in _data
• • Get access to the data content of the PKCS#7 message. The size of the
• • header of the ASN.1 object that contains it is also provided and can be used
• • to adjust *_data and *_data_len to get the entire object.
• • Returns -ENODATA if the data object was missing from the message.
• */

+int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,

•                      const void **_data, size_t *_data_len,
  

•                      size_t *_headerlen)
  

+{

•   if (!pkcs7->data)
  

•           return -ENODATA;
  

•   *_data = pkcs7->data;
  

•   *_data_len = pkcs7->data_len;
  

•   if (_headerlen)
  

•           *_headerlen = pkcs7->data_hdrlen;
  

•   return 0;
  

+} diff --git a/lib/crypto/pkcs7_parser.c b/lib/crypto/pkcs7_parser.c index d5efa828d6a..c849dc0d92d 100644 --- a/lib/crypto/pkcs7_parser.c +++ b/lib/crypto/pkcs7_parser.c @@ -182,34 +182,6 @@ out_no_ctx: } EXPORT_SYMBOL_GPL(pkcs7_parse_message);

-/**

• • pkcs7_get_content_data - Get access to the PKCS#7 content
• • @pkcs7: The preparsed PKCS#7 message to access
• • @_data: Place to return a pointer to the data
• • @_data_len: Place to return the data length
• • @_headerlen: Size of ASN.1 header not included in _data
• • Get access to the data content of the PKCS#7 message. The size of the
• • header of the ASN.1 object that contains it is also provided and can be used
• • to adjust *_data and *_data_len to get the entire object.
• • Returns -ENODATA if the data object was missing from the message.
• */

-int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,

•                      const void **_data, size_t *_data_len,
  

•                      size_t *_headerlen)
  

-{

•   if (!pkcs7->data)
  

•           return -ENODATA;
  

•   *_data = pkcs7->data;
  

•   *_data_len = pkcs7->data_len;
  

•   if (_headerlen)
  

•           *_headerlen = pkcs7->data_hdrlen;
  

•   return 0;
  

-} -EXPORT_SYMBOL_GPL(pkcs7_get_content_data);

/*

• Note an OID when we find one for later processing when we know how
• to interpret it.

-- 2.25.1

with the above fixed Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

On Tue, 2 Jul 2024 at 21:30, Raymond Mao raymond.mao@linaro.org wrote:

Add porting layer for public key on top of MbedTLS X509 library. Introduce _LEGACY and _MBEDTLS kconfigs for public key legacy and MbedTLS implementations respectively.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v2

• Move the porting layer to MbedTLS dir.

Changes in v3

• None.

Changes in v4

• Introduce _LEGACY and _MBEDTLS kconfigs for public key legacy and MbedTLS implementations respectively.
• Move common functions to helper.

lib/mbedtls/Kconfig | 50 ++++++++++++++++++++++++ lib/mbedtls/Makefile | 7 +++- lib/mbedtls/public_key.c | 82 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 1 deletion(-) create mode 100644 lib/mbedtls/public_key.c

diff --git a/lib/mbedtls/Kconfig b/lib/mbedtls/Kconfig index 0cdf0135667..6f29b0c81a2 100644 --- a/lib/mbedtls/Kconfig +++ b/lib/mbedtls/Kconfig @@ -122,9 +122,35 @@ endif # LEGACY_CRYPTO_BASIC

config LEGACY_CRYPTO_CERT bool "legacy certificate libraries"

•   select ASYMMETRIC_PUBLIC_KEY_SUBTYPE_LEGACY if \
  

•           ASYMMETRIC_PUBLIC_KEY_SUBTYPE
  

•   select SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE_LEGACY if \
  

•           ASYMMETRIC_PUBLIC_KEY_SUBTYPE && SPL
    help
      Enable legacy certificate libraries.
  
  

+if LEGACY_CRYPTO_CERT

+config ASYMMETRIC_PUBLIC_KEY_SUBTYPE_LEGACY

•   bool "Asymmetric public key crypto with legacy certificate library"
  

•   depends on LEGACY_CRYPTO_CERT && ASYMMETRIC_PUBLIC_KEY_SUBTYPE
  

•   help
  

•     This option chooses legacy certificate library for asymmetric public
  

•     key crypto algorithm.
  

+if SPL

+config SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE_LEGACY

subtype is either legacy or mbedTLS? If yes I think you can get rid of it and have SPL_ASYMMETRIC_PUBLIC_KEY_LEGACY and ASYMMETRIC_PUBLIC_KEY_MBEDTLS

•   bool "Asymmetric public key crypto with legacy certificate library in SPL"
  

•   depends on LEGACY_CRYPTO_CERT && SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE
  

•   help
  

•     This option chooses legacy certificate library for asymmetric public
  

•     key crypto algorithm in SPL.
  

+endif # SPL

+endif # LEGACY_CRYPTO_CERT

endif # LEGACY_CRYPTO

if MBEDTLS_LIB @@ -232,7 +258,31 @@ endif # MBEDTLS_LIB_CRYPTO

config MBEDTLS_LIB_X509 bool "MbedTLS certificate libraries"

•   select ASYMMETRIC_PUBLIC_KEY_SUBTYPE_MBEDTLS if \
  

•           ASYMMETRIC_PUBLIC_KEY_SUBTYPE
  

•   select SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE_MBEDTLS if \
  

•           ASYMMETRIC_PUBLIC_KEY_SUBTYPE && SPL
  

Why do we have to select SPL here and in the Kconfig above for the legacy option?

    help
      Enable MbedTLS certificate libraries.

+if MBEDTLS_LIB_X509

+config ASYMMETRIC_PUBLIC_KEY_SUBTYPE_MBEDTLS

•   bool "Asymmetric public key crypto with MbedTLS certificate library"
  

•   help
  

•     This option chooses MbedTLS certificate library for asymmetric public
  

•     key crypto algorithm.
  

+if SPL

+config SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE_MBEDTLS

•   bool "Asymmetric public key crypto with MbedTLS certificate library in SPL"
  

•   help
  

•     This option chooses MbedTLS certificate library for asymmetric public
  

•     key crypto algorithm in SPL.
  

+endif # SPL

+endif # MBEDTLS_LIB_X509

endif # MBEDTLS_LIB diff --git a/lib/mbedtls/Makefile b/lib/mbedtls/Makefile index 32a98b7f4ca..f06d0704502 100644 --- a/lib/mbedtls/Makefile +++ b/lib/mbedtls/Makefile @@ -20,6 +20,11 @@ hash_mbedtls-$(CONFIG_$(SPL_)SHA1_MBEDTLS) += sha1.o hash_mbedtls-$(CONFIG_$(SPL_)SHA256_MBEDTLS) += sha256.o hash_mbedtls-$(CONFIG_$(SPL_)SHA512_MBEDTLS) += sha512.o

+# x509 libraries +obj-$(CONFIG_MBEDTLS_LIB_X509) += x509_mbedtls.o +x509_mbedtls-$(CONFIG_$(SPL_)ASYMMETRIC_PUBLIC_KEY_SUBTYPE_MBEDTLS) += \

•   public_key.o
  

# MbedTLS crypto library obj-$(CONFIG_MBEDTLS_LIB_CRYPTO) += mbedtls_lib_crypto.o mbedtls_lib_crypto-y += \ @@ -45,7 +50,7 @@ mbedtls_lib_x509-$(CONFIG_$(SPL_)RSA_PUBLIC_KEY_PARSER) += \ $(MBEDTLS_LIB_DIR)/bignum_core.o \ $(MBEDTLS_LIB_DIR)/rsa.o \ $(MBEDTLS_LIB_DIR)/rsa_alt_helpers.o -mbedtls_lib_x509-$(CONFIG_$(SPL_)ASYMMETRIC_PUBLIC_KEY_SUBTYPE) += \ +mbedtls_lib_x509-$(CONFIG_$(SPL_)ASYMMETRIC_PUBLIC_KEY_SUBTYPE_MBEDTLS) += \ $(MBEDTLS_LIB_DIR)/pk.o \ $(MBEDTLS_LIB_DIR)/pk_wrap.o \ $(MBEDTLS_LIB_DIR)/pkparse.o diff --git a/lib/mbedtls/public_key.c b/lib/mbedtls/public_key.c new file mode 100644 index 00000000000..076a61862cb --- /dev/null +++ b/lib/mbedtls/public_key.c @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Public key helper functions using MbedTLS X509 library
• • Copyright (c) 2024 Linaro Limited
• • Author: Raymond Mao raymond.mao@linaro.org
• */

+#include <linux/compat.h> +#include <crypto/public_key.h>

+int public_key_verify_signature(const struct public_key *pkey,

•                           const struct public_key_signature *sig)
  

+{

•   mbedtls_md_type_t mb_hash_algo;
  

•   mbedtls_pk_context pk_ctx;
  

•   int ret;
  

•   if (!pkey || !sig || pkey->key_is_private)
  

•           return -EINVAL;
  

•   /*
  

•    * ECRDSA (Elliptic Curve RedDSA) from Red Hat is not supported by
  

•    * MbedTLS
  

•    */
  

That's not Redhat AFAICT. That's Elliptic Curve (Russian) Digital Signature Algorithm described in GOST 34.10-2018, GOST R 34.10-2012, RFC 7091, ISO/IEC 14888-3:2018

•   if (strcmp(pkey->pkey_algo, "rsa")) {
  

•           pr_err("Encryption is not RSA: %s\n", sig->pkey_algo);
  

•           return -EINVAL;
  

•   }
  

•   /*
  

•    * Can be pkcs1 or raw, but pkcs1 is expected.
  

•    * This is just for argument checking, not necessarily passed to MbedTLS,
  

•    * For RSA signatures, MbedTLS typically supports the PKCS#1 v1.5
  

•    * (aka. pkcs1) encoding by default.
  

•    * The library internally handles the details of decoding and verifying
  

•    * the signature according to the expected encoding for the specified algorithm.
  

•    */
  

•   if (strcmp(sig->encoding, "pkcs1")) {
  

•           pr_err("Encoding %s is not supported, only supports pkcs1\n",
  

•                  sig->encoding);
  

•           return -EINVAL;
  

•   }
  

•   if (!strcmp(sig->hash_algo, "sha1"))
  

•           mb_hash_algo = MBEDTLS_MD_SHA1;
  

•   else if (!strcmp(sig->hash_algo, "sha224"))
  

•           mb_hash_algo = MBEDTLS_MD_SHA224;
  

•   else if (!strcmp(sig->hash_algo, "sha256"))
  

•           mb_hash_algo = MBEDTLS_MD_SHA256;
  

•   else if (!strcmp(sig->hash_algo, "sha384"))
  

•           mb_hash_algo = MBEDTLS_MD_SHA384;
  

•   else if (!strcmp(sig->hash_algo, "sha512"))
  

•           mb_hash_algo = MBEDTLS_MD_SHA512;
  

•   else    /* Unknown or unsupported hash algorithm */
  

•           return -EINVAL;
  

•   /* Initialize the mbedtls_pk_context with RSA key type */
  

•   mbedtls_pk_init(&pk_ctx);
  

•   /* Parse the DER-encoded public key */
  

•   ret = mbedtls_pk_parse_public_key(&pk_ctx, pkey->key, pkey->keylen);
  

•   if (ret) {
  

•           pr_err("Failed to parse public key, ret:-0x%04x\n", -ret);
  

•           ret = -EINVAL;
  

•           goto err_key;
  

•   }
  

•   /* Ensure that it is a RSA key */
  

•   if (mbedtls_pk_get_type(&pk_ctx) != MBEDTLS_PK_RSA) {
  

•           pr_err("Only RSA keys are supported\n");
  

•           ret = -EKEYREJECTED;
  

•           goto err_key;
  

•   }
  

•   /* Verify the hash */
  

•   ret = mbedtls_pk_verify(&pk_ctx, mb_hash_algo, sig->digest,
  

•                           sig->digest_size, sig->s, sig->s_size);
  

+err_key:

•   mbedtls_pk_free(&pk_ctx);
  

•   return ret;
  

+}

2.25.1

Thanks /Ilias

Hi Raymond

+#if CONFIG_IS_ENABLED(MBEDTLS_LIB_X509) +/* Backup of part of the parsing context */

I am not sure I understand the comment

+struct x509_cert_mbedtls_ctx {

•   void    *tbs;                   /* Signed data */
  

•   void    *raw_serial;            /* Raw serial number in ASN.1 */
  

•   void    *raw_issuer;            /* Raw issuer name in ASN.1 */
  

•   void    *raw_subject;           /* Raw subject name in ASN.1 */
  

•   void    *raw_skid;              /* Raw subjectKeyId in ASN.1 */
  

+}; +#endif

+/*

• • MbedTLS integration Notes:
• • Fields we don't need to populate from MbedTLS:

You mean *for* mbedTLS?

• • 'raw_sig' and 'raw_sig_size' are buffer for x509_parse_context,

'raw_sig' and 'raw_sig_size' are used in x509_parse_context(), which in turn is not used in mbedTLS?

• • not needed for MbedTLS.
• • 'signer' and 'seen' are used internally by pkcs7_verify.
• • 'verified' is not inuse.

either 'unsued' or 'not in use'

• */

struct x509_certificate { +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_X509)

•   struct x509_cert_mbedtls_ctx *mbedtls_ctx;
  

+#endif struct x509_certificate *next; struct x509_certificate *signer; /* Certificate that signed this one */ struct public_key *pub; /* Public key details */ @@ -48,6 +76,32 @@ struct x509_certificate {

• x509_cert_parser.c

*/ extern void x509_free_certificate(struct x509_certificate *cert); +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_X509) +/**

• • x509_populate_pubkey() - Populate public key from MbedTLS context
• • @cert: Pointer to MbedTLS X509 cert
• • @pub_key: Pointer to the populated public key handle
• • Return: 0 on succcess, error code on failure
• */

+int x509_populate_pubkey(mbedtls_x509_crt *cert, struct public_key **pub_key); +/**

• • x509_populate_cert() - Populate X509 cert from MbedTLS context
• • @mbedtls_cert: Pointer to MbedTLS X509 cert
• • @pcert: Pointer to the populated X509 cert handle
• • Return: 0 on succcess, error code on failure
• */

+int x509_populate_cert(mbedtls_x509_crt *mbedtls_cert,

•                  struct x509_certificate **pcert);
  

+/**

• • x509_get_timestamp() - Translate timestamp from MbedTLS context
• • @x509_time: Pointer to MbedTLS time
• • Return: Time in time64_t format
• */

+time64_t x509_get_timestamp(const mbedtls_x509_time *x509_time); +#endif extern struct x509_certificate *x509_cert_parse(const void *data, size_t datalen); extern int x509_decode_time(time64_t *_t, size_t hdrlen, unsigned char tag, @@ -56,6 +110,8 @@ extern int x509_decode_time(time64_t *_t, size_t hdrlen, /*

• x509_public_key.c

*/ +#if !CONFIG_IS_ENABLED(MBEDTLS_LIB_X509) extern int x509_get_sig_params(struct x509_certificate *cert); +#endif extern int x509_check_for_self_signed(struct x509_certificate *cert); #endif /* _X509_PARSER_H */ diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig index 6e0656ad1c5..6106190677e 100644 --- a/lib/crypto/Kconfig +++ b/lib/crypto/Kconfig @@ -1,6 +1,6 @@ menuconfig ASYMMETRIC_KEY_TYPE bool "Asymmetric (public-key cryptographic) key Support"

•   depends on FIT_SIGNATURE
  

•   depends on LEGACY_CRYPTO_CERT || MBEDTLS_LIB_X509
    help
      This option provides support for a key type that holds the data for
      the asymmetric keys used for public key cryptographic operations such
  

diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile index 228ae443a27..7f5f04d582c 100644 --- a/lib/crypto/Makefile +++ b/lib/crypto/Makefile @@ -32,11 +32,11 @@ endif # X.509 Certificate handling # obj-$(CONFIG_$(SPL_)X509_CERTIFICATE_PARSER) += x509_key_parser.o -x509_key_parser-y := \ +x509_key_parser-y := x509_helper.o +x509_key_parser-$(CONFIG_$(SPL_)X509_CERTIFICATE_PARSER_LEGACY) += \ x509.asn1.o \ x509_akid.asn1.o \ x509_cert_parser.o \

•   x509_helper.o \
    x509_public_key.o
  
  

$(obj)/x509_cert_parser.o: \ diff --git a/lib/crypto/x509_public_key.c b/lib/crypto/x509_public_key.c index 4ba13c1adc3..310edbd21be 100644 --- a/lib/crypto/x509_public_key.c +++ b/lib/crypto/x509_public_key.c @@ -30,6 +30,8 @@ #include "x509_parser.h" #endif

+#if !CONFIG_IS_ENABLED(MBEDTLS_LIB_X509)

/*

• Set up the signature parameters in an X.509 certificate. This involves
• digesting the signed data and extracting the signature.

-- 2.25.1

Hi Ilias,

On Fri, 26 Jul 2024 at 06:10, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:

Hi Raymond

On Tue, 2 Jul 2024 at 21:33, Raymond Mao raymond.mao@linaro.org wrote:

...

Add porting layer for MSCode on top of MbedTLS ASN1 library. Introduce _LEGACY and _MBEDTLS kconfigs for MSCode legacy and MbedTLS implementations respectively.

You should mention explicitly on the commit message, that this patch is expected to be merged in mbedTLS upstream and we can remove it in the future

This patch is native U-Boot patch. The ones for MbedTLS upstream are all

tagged with "mbedtls/external".

...

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v2

• Move the porting layer to MbedTLS dir.

Changes in v3

• None.

Changes in v4

• Introduce _LEGACY and _MBEDTLS kconfigs for MSCode legacy and MbedTLS implementations respectively.
• Fix a few code style.

lib/mbedtls/Kconfig | 17 +++++ lib/mbedtls/Makefile | 1 + lib/mbedtls/mscode_parser.c | 123 ++++++++++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+) create mode 100644 lib/mbedtls/mscode_parser.c

diff --git a/lib/mbedtls/Kconfig b/lib/mbedtls/Kconfig index 8c5b617bb48..d8a8f87e031 100644 --- a/lib/mbedtls/Kconfig +++ b/lib/mbedtls/Kconfig @@ -126,6 +126,7 @@ config LEGACY_CRYPTO_CERT ASYMMETRIC_PUBLIC_KEY_SUBTYPE select X509_CERTIFICATE_PARSER_LEGACY if X509_CERTIFICATE_PARSER select PKCS7_MESSAGE_PARSER_LEGACY if PKCS7_MESSAGE_PARSER

•   select MSCODE_PARSER_LEGACY if MSCODE_PARSER
    select SPL_ASYMMETRIC_PUBLIC_KEY_SUBTYPE_LEGACY if \
            ASYMMETRIC_PUBLIC_KEY_SUBTYPE && SPL
    help
  

@@ -156,6 +157,14 @@ config PKCS7_MESSAGE_PARSER_LEGACY This option chooses legacy certificate library for PKCS7

message

...

      parser.

+config MSCODE_PARSER_LEGACY

Where is this used? Is it on a later patch? The MSCODE_PARSER_MBEDTLS option is part of this patch on the Makefile

It is used in the next patch (#24) , it is good to split into two since

they are in different domains (mbedtls/crypto).

Regards, Raymond

Hi Raymond,

On Tue, 2 Jul 2024 at 21:33, Raymond Mao raymond.mao@linaro.org wrote:

Previous patch has introduced MbedTLS porting layer for mscode parser, here to adjust the header and makefiles accordingly.

Signed-off-by: Raymond Mao raymond.mao@linaro.org

Changes in v2

• Move the porting layer to MbedTLS dir.

Changes in v3

• Update commit message.

Changes in v4

• Control building legacy library via '_LEGACY' Kconfig.
• Minor fix of the include directories.

include/crypto/mscode.h | 4 ++++ lib/crypto/Makefile | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/crypto/mscode.h b/include/crypto/mscode.h index 551058b96e6..678e69001b9 100644 --- a/include/crypto/mscode.h +++ b/include/crypto/mscode.h @@ -9,6 +9,10 @@ #ifndef __UBOOT__ #include <crypto/hash_info.h> #endif +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_X509) +#include <mbedtls/asn1.h> +#include <mbedtls/oid.h> +#endif

struct pefile_context { #ifndef __UBOOT__ diff --git a/lib/crypto/Makefile b/lib/crypto/Makefile index 428dcba0a6b..9bbd8b48d77 100644 --- a/lib/crypto/Makefile +++ b/lib/crypto/Makefile @@ -63,7 +63,7 @@ obj-$(CONFIG_$(SPL_)PKCS7_VERIFY) += pkcs7_verify.o # # Signed PE binary-wrapped key handling # -obj-$(CONFIG_$(SPL_)MSCODE_PARSER) += mscode.o +obj-$(CONFIG_$(SPL_)MSCODE_PARSER_LEGACY) += mscode.o

The change it self is correct. But patches should be self contained as much as possible without breaking the compilation or functionality. IOW I think this should be in a different patch where the MSCODE_PARSER_LEGACY is introduced.

Cheers /Ilias

mscode-y := \ mscode_parser.o \ -- 2.25.1