drivers:crypto:hisilicon:Add HASH and HMAC algorithm #6823

LeiSen62 · 2024-05-08T09:08:38Z

Add HASH and HMAC algorithm by SEC, and support SHA1, SHA224, SHA256, SHA384, SHA512, MD5, SM3, and HMAC algorithms based on these algorithms.

jenswi-linaro · 2024-05-21T09:05:50Z

I'm concerned with the header files in core/drivers/crypto/hisilicon/include. They are in the global include path so they can potentially be included by some .h file and made available to any .c file. It seems that they are not intended to be used in that way, but that's not entirely clear.

I would prefer if all .h files here were kept in core/drivers/crypto/hisilicon and included as #include "...". Then it's clear that they are internal .h files that aren't supposed to be included in source files outside of this directory. What do others think?

LeiSen62 · 2024-05-21T11:18:34Z

The header files in core/drivers/crypto/hisilicon/includes are only used by the source files in core/drivers/crypto/hisilicon, so I also think it would be better to put all .h files in core/drivers/crypto/hisilicon/.

jenswi-linaro · 2024-05-21T12:32:36Z

Thanks, then we're on the same page. Please add a patch on top of this that moves the .h files as we agreed.

jenswi-linaro · 2024-05-21T14:11:25Z

core/drivers/crypto/hisilicon/include/sec_main.h

@@ -49,6 +49,298 @@
 #define SEC_ICV_LEN_OFFSET_V3	4
 #define SEC_DK_LEN_OFFSET_V3	16
 #define SEC_KEY_SEL_OFFSET_V3	21
+#define SEC_GET_FIELD(val, mask, shift) (((val) & (mask)) >> (shift))
+#define ALIGN(_val, _size) \


Please use ROUNDUP() from <util.h> instead.

jenswi-linaro · 2024-05-22T07:17:05Z

core/drivers/crypto/hisilicon/sec_hash.c

+	if (ctx->has_next && !ctx->iv_len) {
+		/* LONG BD FIRST */
+		sqe->ai_apd_cs |= AI_GEN_INNER;
+		sqe->ai_apd_cs |= (AUTHPAD_NOPAD << SEC_APAD_OFFSET);


SHIFT_U32() or SHIFT_U64() are generally preferred since they are safer.

ai_apd_cs is u8

Sure in this particular case there no issue since SEC_APAD_OFFSET is 2, but you you need to crosscheck to see that it isn't 31 in case AUTHPAD_NOPAD is a signed 32-bit integer. So it's better to use these shift macros where possible.

jenswi-linaro · 2024-05-22T07:19:45Z

core/drivers/crypto/hisilicon/sec_hash.c

+	 * To ensure in data is NULL, reset ctx need to free in data
+	 * which is not NULL.
+	 */
+	if (hash_ctx->in) {


There's no need to check this since free(NULL) does nothing.

core/drivers/crypto/hisilicon/sec_hash.c

jenswi-linaro · 2024-05-22T07:37:37Z

core/drivers/crypto/hisilicon/sec_hash.c

+
+static TEE_Result sec_hash_initialize(struct crypto_hash_ctx *ctx)
+{
+	struct crypto_hash *hash;


Please initialize all local variables. Same for the rest of this file.

jenswi-linaro · 2024-05-22T07:43:45Z

core/drivers/crypto/hisilicon/sec_hash.c

+	out_hash_ctx->long_data_len = in_hash_ctx->long_data_len;
+
+	if (in_hash_ctx->in) {
+		out_hash_ctx->in = (uint8_t *)malloc(out_hash_ctx->buf_len);


No need to cast void pointers.

jenswi-linaro · 2024-05-22T07:45:04Z

core/drivers/crypto/hisilicon/sec_hash.c

+	.copy_state = sec_hash_copy_state,
+};
+
+struct crypto_hash *to_hash_ctx(struct crypto_hash_ctx *ctx)


This function should be static

jenswi-linaro · 2024-05-22T07:48:27Z

core/drivers/crypto/hisilicon/sec_hash.c

+	return HISI_QM_DRVCRYPT_NO_ERR;
+}
+
+TEE_Result sec_digest_ctx_init(struct hashctx *hash_ctx,


Global functions should use a prefix. Wouldn't hisi_sec_ make sense in this file? Same for all the other globals in this file.

I will use hisi_sec_ as a prefix for global functions, thanks.

jenswi-linaro · 2024-05-22T07:49:31Z

core/drivers/crypto/hisilicon/sec_hmac.c

+	}
+
+	hash = to_hmac_ctx(ctx);
+	if (!hash) {


to_hmac_ctx() doesn't return NULL.

jenswi-linaro · 2024-05-22T07:49:54Z

core/drivers/crypto/hisilicon/sec_hmac.c

+	.copy_state = sec_hmac_copy_state,
+};
+
+struct crypto_hmac *to_hmac_ctx(struct crypto_mac_ctx *ctx)


This function should be static.

jenswi-linaro

In the title of the commit messages, please add a single space after each colon.

jenswi-linaro · 2024-05-23T07:34:56Z

core/drivers/crypto/hisilicon/sec_hash.c

+
+static enum hisi_drv_status sec_digest_fill_bd3_sqe(void *bd, void *msg)
+{
+	struct hisi_sec_bd3_sqe *sqe = (struct hisi_sec_bd3_sqe *)bd;


No need to cast a void pointer, same below.

jenswi-linaro · 2024-05-23T07:35:09Z

core/drivers/crypto/hisilicon/sec_hash.c

+	struct hashctx *ctx = (struct hashctx *)msg;
+	uint32_t alg_type = 0;
+
+	sqe->bd_param = (BD_TYPE3 | SHIFT_U32(ctx->scene, SEC_SCENE_OFFSET_V3));


The outer () are not needed.

jenswi-linaro · 2024-05-23T07:35:33Z

core/drivers/crypto/hisilicon/sec_hash.c

+			return HISI_QM_DRVCRYPT_IN_EPARA;
+	}
+
+	sqe->auth_mac_key |= SHIFT_U64((ctx->mac_len / SEC_ENCODE_BYTES),


The inner () are not needed.

jenswi-linaro · 2024-05-23T07:37:20Z

core/drivers/crypto/hisilicon/sec_hash.c

+		return TEE_ERROR_BAD_PARAMETERS;
+	}
+
+	hash = (struct crypto_hash *)calloc(1, sizeof(*hash));


No need to cast a void pointer, please fix all the places in this patch.

jenswi-linaro · 2024-05-23T15:05:26Z

Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

Add HASH and HMAC algorithm by SEC, and support SHA1, SHA224, SHA256, SHA384, SHA512, MD5, SM3, and HMAC algorithms based on these algorithms. Signed-off-by: leisen <leisen1@huawei.com> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

The header files in core/drivers/crypto/hisilicon/includes are only used by the source files in core/drivers/crypto/hisilicon, so move the header file from core/drivers/crypto/hisilicon/include to core/drivers/crypto/hisilicon/. Signed-off-by: leisen <leisen1@huawei.com> Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

LeiSen62 · 2024-05-24T08:57:26Z

Hi @jforissier ,all review comments have been modified. Can you help me merge the patch?

jforissier · 2024-05-24T09:12:27Z

A few space characters added to the second commit subject, and pushed to master. Thanks!

LeiSen62 force-pushed the master branch from 36906dc to 975198a Compare May 13, 2024 13:39

LeiSen62 force-pushed the master branch 2 times, most recently from e180d37 to f108abf Compare May 21, 2024 15:18

jenswi-linaro reviewed May 22, 2024

View reviewed changes

LeiSen62 force-pushed the master branch 4 times, most recently from c245159 to 55d4ac6 Compare May 22, 2024 15:18

jenswi-linaro reviewed May 23, 2024

View reviewed changes

LeiSen62 force-pushed the master branch from 55d4ac6 to 58f6ab0 Compare May 23, 2024 14:06

LeiSen62 added 2 commits May 23, 2024 23:08

LeiSen62 force-pushed the master branch from 58f6ab0 to e146f9f Compare May 23, 2024 15:12

jforissier closed this May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers:crypto:hisilicon:Add HASH and HMAC algorithm #6823

drivers:crypto:hisilicon:Add HASH and HMAC algorithm #6823

LeiSen62 commented May 8, 2024

jenswi-linaro commented May 21, 2024

LeiSen62 commented May 21, 2024

jenswi-linaro commented May 21, 2024

jenswi-linaro May 21, 2024

jenswi-linaro May 22, 2024

LeiSen62 May 22, 2024

jenswi-linaro May 22, 2024

jenswi-linaro May 22, 2024

jenswi-linaro May 22, 2024

jenswi-linaro May 22, 2024

jenswi-linaro May 22, 2024

jenswi-linaro May 22, 2024

LeiSen62 May 22, 2024

jenswi-linaro May 22, 2024

jenswi-linaro May 22, 2024

jenswi-linaro left a comment •

edited

jenswi-linaro May 23, 2024

jenswi-linaro May 23, 2024

jenswi-linaro May 23, 2024

jenswi-linaro May 23, 2024

jenswi-linaro commented May 23, 2024

LeiSen62 commented May 24, 2024

jforissier commented May 24, 2024

drivers:crypto:hisilicon:Add HASH and HMAC algorithm #6823

drivers:crypto:hisilicon:Add HASH and HMAC algorithm #6823

Conversation

LeiSen62 commented May 8, 2024

jenswi-linaro commented May 21, 2024

LeiSen62 commented May 21, 2024

jenswi-linaro commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented May 23, 2024

LeiSen62 commented May 24, 2024

jforissier commented May 24, 2024

jenswi-linaro left a comment •

edited