Hi Ilias,
On Wed, 28 Aug 2024 at 06:37, Ilias Apalodimas <ilias.apalodi...@linaro.org>
wrote:
> Hi Raymond,
>
> [...]
>
> > --- a/lib/mbedtls/Makefile
> > +++ b/lib/mbedtls/Makefile
> > @@ -5,17 +5,23 @@
> >
> > MBEDTLS_LIB_DIR = external/mbedtls/library
> >
> > +# shim layer for hash
> > +obj-$(CONFIG_$(SPL_)MD5_MBEDTLS) += md5.o
> > +obj-$(CONFIG_$(SPL_)SHA1_MBEDTLS) += sha1.o
> > +obj-$(CONFIG_$(SPL_)SHA256_MBEDTLS) += sha256.o
> > +obj-$(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) += \
>
> Why do we need to rename these here? Can't you add them with the _MBEDTLS
> suffix on the patch that introduced them?
>
> Patch #2 introduced the digest library but set it as default. And this
patch moves it under _MBEDTLS kconfig.
I have to separate into two otherwise patch #3 will be too huge and hard to
be reviewed.
> > $(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);
> > + }
> > +
> > + 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....@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....@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);
>
> Patch #7 treats this differently and looks at the mbedtls_sha256_finish()
> result (for all hashing algos). I think this one is correct and the other
> one needs fixing
>
> The difference is just due to different API prototypes to be ported - one
returns void while
the other returns int. According to this difference I decided to check the
result of
mbedtls_sha256_finish() or not.
[snip]
Regards,
Raymond
