Re: [PATCH v6 06/28] mbedtls: add digest shim layer for MbedTLS

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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?

>  	$(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);
> +	}
> +
> +	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);

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 

> +}
> +
> +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) {
 
[...]

Thanks
/Ilias