Hi Ilias,
On Fri, 26 Jul 2024 at 06:19, Ilias Apalodimas <ilias.apalodi...@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....@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
