Re: [PATCH 3/4] crypto: hash: Add software hash DM driver

["Alex G." <mr.nuke.me@gmail.com>]

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.

> 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().

Alex