Hi Alex,
From: Alex G. mr.nuke.me@gmail.com Sent: Thursday, September 16, 2021 11:49 PM
On 7/29/21 8:08 PM, Chia-Wei Wang wrote:
Add purely software-implmented drivers to support multiple hash operations including CRC, MD5, and SHA family.
This driver is based on the new hash uclass.
Signed-off-by: Chia-Wei Wang chiawei_wang@aspeedtech.com
drivers/crypto/hash/Kconfig | 11 ++ drivers/crypto/hash/Makefile | 1 + drivers/crypto/hash/hash_sw.c | 301
++++++++++++++++++++++++++++++++++
3 files changed, 313 insertions(+) create mode 100644 drivers/crypto/hash/hash_sw.c
diff --git a/drivers/crypto/hash/Kconfig b/drivers/crypto/hash/Kconfig index e226144b9b..cd29a5c6a4 100644 --- a/drivers/crypto/hash/Kconfig +++ b/drivers/crypto/hash/Kconfig @@ -3,3 +3,14 @@ config DM_HASH depends on DM help If you want to use driver model for Hash, say Y.
+config HASH_SOFTWARE
- bool "Enable driver for Hash in software"
- depends on DM_HASH
- depends on MD5
- depends on SHA1
- depends on SHA256
- depends on SHA512_ALGO
I would have expected a U_BOOT_DRIVER() for each hash algo, rather than a U_BOOT_DRIVER() wich encompassess all possible algos. If I'm trying to use SHA256 in SPL, I might not have the room too add SHA1 and MD5, so I'd have issues using HASH_SOFTWARE, as designed.
I agree with the SPL size issue. A future patches to move those CONFIG_SHAxxx/CONFIG_MD5 options into the DM-based hash_sw.c could be an option. Thus the balance between the hash algos support and the code size can be managed.
diff --git a/drivers/crypto/hash/hash_sw.c b/drivers/crypto/hash/hash_sw.c new file mode 100644 index 0000000000..fea9d12609 --- /dev/null +++ b/drivers/crypto/hash/hash_sw.c @@ -0,0 +1,301 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Copyright (c) 2021 ASPEED Technology Inc.
- Author: ChiaWei Wang chiawei_wang@aspeedtech.com */ #include
+<config.h> #include <common.h> #include <dm.h> #include <log.h> +#include <malloc.h> #include <watchdog.h> #include <u-boot/hash.h> +#include <u-boot/crc.h> #include <u-boot/md5.h> #include +<u-boot/sha1.h> #include <u-boot/sha256.h> #include <u-boot/sha512.h>
+/* CRC16-CCITT */ +static void hash_init_crc16_ccitt(void *ctx) {
- *((uint16_t *)ctx) = 0;
Undefined behavior: Pointer aliased type-punning. I would suggest using memset(). Might not be necessarrym as expleined in the next comment.
+static void hash_update_crc16_ccitt(void *ctx, const void *ibuf, +uint32_t ilen) static void hash_finish_crc16_ccitt(void *ctx, void +*obuf) +/* CRC32 */ +static void hash_init_crc32(void *ctx) static void +hash_update_crc32(void *ctx, const void *ibuf, uint32_t ilen) static +void hash_finish_crc32(void *ctx, void *obuf) +/* SHA1 */ +static void hash_init_sha1(void *ctx) +/* SHA256 */ +/* SHA384 */ +/* SHA512 */
This logic already exists in common/hash.c for hash_Lookup_algo() and hash_progressive_algo().
Yes. To prevent modifying the 'static' keyword in common/hash.c while reusing the hash lib, the same logic appears in the DM-based hash_sw.c.
Chiawei