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

[ChiaWei Wang]

Hi Alex,

> From: Alex G. <mr.nuke.me at 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 at 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 at 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