diff mbox series

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

Message ID 20210730010805.17845-4-chiawei_wang@aspeedtech.com
State Accepted
Commit e5d870fa1ecbcd4efcc13fa6d69c6754e39cff62
Delegated to: Tom Rini
Headers show
Series crypto: Add new UCLASS_HASH | expand

Commit Message

ChiaWei Wang July 30, 2021, 1:08 a.m. UTC
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

Comments

Tom Rini Sept. 2, 2021, 1:28 p.m. UTC | #1
On Fri, Jul 30, 2021 at 09:08:04AM +0800, 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>

Applied to u-boot/next, thanks!
Alex G. Sept. 16, 2021, 3:48 p.m. UTC | #2
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
ChiaWei Wang Sept. 22, 2021, 3:18 a.m. UTC | #3
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
diff mbox series

Patch

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
+	help
+	  Enable driver for hashing operations in software. Currently
+	  it support multiple hash algorithm including CRC/MD5/SHA.
diff --git a/drivers/crypto/hash/Makefile b/drivers/crypto/hash/Makefile
index 83acf3d47b..33d88161ed 100644
--- a/drivers/crypto/hash/Makefile
+++ b/drivers/crypto/hash/Makefile
@@ -3,3 +3,4 @@ 
 # Copyright (c) 2021 ASPEED Technology Inc.
 
 obj-$(CONFIG_DM_HASH) += hash-uclass.o
+obj-$(CONFIG_HASH_SOFTWARE) += hash_sw.o
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;
+}
+
+static void hash_update_crc16_ccitt(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	*((uint16_t *)ctx) = crc16_ccitt(*((uint16_t *)ctx), ibuf, ilen);
+}
+
+static void hash_finish_crc16_ccitt(void *ctx, void *obuf)
+{
+	*((uint16_t *)obuf) = *((uint16_t *)ctx);
+}
+
+/* CRC32 */
+static void hash_init_crc32(void *ctx)
+{
+	*((uint32_t *)ctx) = 0;
+}
+
+static void hash_update_crc32(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	*((uint32_t *)ctx) = crc32(*((uint32_t *)ctx), ibuf, ilen);
+}
+
+static void hash_finish_crc32(void *ctx, void *obuf)
+{
+	*((uint32_t *)obuf) = *((uint32_t *)ctx);
+}
+
+/* MD5 */
+static void hash_init_md5(void *ctx)
+{
+	MD5Init((struct MD5Context *)ctx);
+}
+
+static void hash_update_md5(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	MD5Update((struct MD5Context *)ctx, ibuf, ilen);
+}
+
+static void hash_finish_md5(void *ctx, void *obuf)
+{
+	MD5Final(obuf, (struct MD5Context *)ctx);
+}
+
+/* SHA1 */
+static void hash_init_sha1(void *ctx)
+{
+	sha1_starts((sha1_context *)ctx);
+}
+
+static void hash_update_sha1(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	sha1_update((sha1_context *)ctx, ibuf, ilen);
+}
+
+static void hash_finish_sha1(void *ctx, void *obuf)
+{
+	sha1_finish((sha1_context *)ctx, obuf);
+}
+
+/* SHA256 */
+static void hash_init_sha256(void *ctx)
+{
+	sha256_starts((sha256_context *)ctx);
+}
+
+static void hash_update_sha256(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	sha256_update((sha256_context *)ctx, ibuf, ilen);
+}
+
+static void hash_finish_sha256(void *ctx, void *obuf)
+{
+	sha256_finish((sha256_context *)ctx, obuf);
+}
+
+/* SHA384 */
+static void hash_init_sha384(void *ctx)
+{
+	sha384_starts((sha512_context *)ctx);
+}
+
+static void hash_update_sha384(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	sha384_update((sha512_context *)ctx, ibuf, ilen);
+}
+
+static void hash_finish_sha384(void *ctx, void *obuf)
+{
+	sha384_finish((sha512_context *)ctx, obuf);
+}
+
+/* SHA512 */
+static void hash_init_sha512(void *ctx)
+{
+	sha512_starts((sha512_context *)ctx);
+}
+
+static void hash_update_sha512(void *ctx, const void *ibuf, uint32_t ilen)
+{
+	sha512_update((sha512_context *)ctx, ibuf, ilen);
+}
+
+static void hash_finish_sha512(void *ctx, void *obuf)
+{
+	sha512_finish((sha512_context *)ctx, obuf);
+}
+
+struct sw_hash_ctx {
+	enum HASH_ALGO algo;
+	uint8_t algo_ctx[];
+};
+
+struct sw_hash_impl {
+	void (*init)(void *ctx);
+	void (*update)(void *ctx, const void *ibuf, uint32_t ilen);
+	void (*finish)(void *ctx, void *obuf);
+	uint32_t ctx_alloc_sz;
+};
+
+static struct sw_hash_impl sw_hash_impl[HASH_ALGO_NUM] = {
+	[HASH_ALGO_CRC16_CCITT] = {
+		.init = hash_init_crc16_ccitt,
+		.update = hash_update_crc16_ccitt,
+		.finish = hash_finish_crc16_ccitt,
+		.ctx_alloc_sz = sizeof(uint16_t),
+	},
+
+	[HASH_ALGO_CRC32] = {
+		.init = hash_init_crc32,
+		.update = hash_update_crc32,
+		.finish = hash_finish_crc32,
+		.ctx_alloc_sz = sizeof(uint32_t),
+	},
+
+	[HASH_ALGO_MD5] = {
+		.init = hash_init_md5,
+		.update = hash_update_md5,
+		.finish = hash_finish_md5,
+		.ctx_alloc_sz = sizeof(struct MD5Context),
+	},
+
+	[HASH_ALGO_SHA1] = {
+		.init = hash_init_sha1,
+		.update = hash_update_sha1,
+		.finish = hash_finish_sha1,
+		.ctx_alloc_sz = sizeof(sha1_context),
+	},
+
+	[HASH_ALGO_SHA256] = {
+		.init = hash_init_sha256,
+		.update = hash_update_sha256,
+		.finish = hash_finish_sha256,
+		.ctx_alloc_sz = sizeof(sha256_context),
+	},
+
+	[HASH_ALGO_SHA384] = {
+		.init = hash_init_sha384,
+		.update = hash_update_sha384,
+		.finish = hash_finish_sha384,
+		.ctx_alloc_sz = sizeof(sha512_context),
+	},
+
+	[HASH_ALGO_SHA512] = {
+		.init = hash_init_sha512,
+		.update = hash_update_sha512,
+		.finish = hash_finish_sha512,
+		.ctx_alloc_sz = sizeof(sha512_context),
+	},
+};
+
+static int sw_hash_init(struct udevice *dev, enum HASH_ALGO algo, void **ctxp)
+{
+	struct sw_hash_ctx *hash_ctx;
+	struct sw_hash_impl *hash_impl = &sw_hash_impl[algo];
+
+	hash_ctx = malloc(sizeof(hash_ctx->algo) + hash_impl->ctx_alloc_sz);
+	if (!hash_ctx)
+		return -ENOMEM;
+
+	hash_ctx->algo = algo;
+
+	hash_impl->init(hash_ctx->algo_ctx);
+
+	*ctxp = hash_ctx;
+
+	return 0;
+}
+
+static int sw_hash_update(struct udevice *dev, void *ctx, const void *ibuf, uint32_t ilen)
+{
+	struct sw_hash_ctx *hash_ctx = ctx;
+	struct sw_hash_impl *hash_impl = &sw_hash_impl[hash_ctx->algo];
+
+	hash_impl->update(hash_ctx->algo_ctx, ibuf, ilen);
+
+	return 0;
+}
+
+static int sw_hash_finish(struct udevice *dev, void *ctx, void *obuf)
+{
+	struct sw_hash_ctx *hash_ctx = ctx;
+	struct sw_hash_impl *hash_impl = &sw_hash_impl[hash_ctx->algo];
+
+	hash_impl->finish(hash_ctx->algo_ctx, obuf);
+
+	free(ctx);
+
+	return 0;
+}
+
+static int sw_hash_digest_wd(struct udevice *dev, enum HASH_ALGO algo,
+			     const void *ibuf, const uint32_t ilen,
+			     void *obuf, uint32_t chunk_sz)
+{
+	int rc;
+	void *ctx;
+	const void *cur, *end;
+	uint32_t chunk;
+
+	rc = sw_hash_init(dev, algo, &ctx);
+	if (rc)
+		return rc;
+
+	if (CONFIG_IS_ENABLED(HW_WATCHDOG) || CONFIG_IS_ENABLED(WATCHDOG)) {
+		cur = ibuf;
+		end = ibuf + ilen;
+
+		while (cur < end) {
+			chunk = end - cur;
+			if (chunk > chunk_sz)
+				chunk = chunk_sz;
+
+			rc = sw_hash_update(dev, ctx, cur, chunk);
+			if (rc)
+				return rc;
+
+			cur += chunk;
+			WATCHDOG_RESET();
+		}
+	} else {
+		rc = sw_hash_update(dev, ctx, ibuf, ilen);
+		if (rc)
+			return rc;
+	}
+
+	rc = sw_hash_finish(dev, ctx, obuf);
+	if (rc)
+		return rc;
+
+	return 0;
+}
+
+static int sw_hash_digest(struct udevice *dev, enum HASH_ALGO algo,
+			  const void *ibuf, const uint32_t ilen,
+			  void *obuf)
+{
+	/* re-use the watchdog version with input length as the chunk_sz */
+	return sw_hash_digest_wd(dev, algo, ibuf, ilen, obuf, ilen);
+}
+
+static const struct hash_ops hash_ops_sw = {
+	.hash_init = sw_hash_init,
+	.hash_update = sw_hash_update,
+	.hash_finish = sw_hash_finish,
+	.hash_digest_wd = sw_hash_digest_wd,
+	.hash_digest = sw_hash_digest,
+};
+
+U_BOOT_DRIVER(hash_sw) = {
+	.name = "hash_sw",
+	.id = UCLASS_HASH,
+	.ops = &hash_ops_sw,
+	.flags = DM_FLAG_PRE_RELOC,
+};
+
+U_BOOT_DRVINFO(hash_sw) = {
+	.name = "hash_sw",
+};