driver: crypto: hisilicon: add X25519 and X448 algorithm #7001

yuzexiyzx · 2024-08-19T11:04:21Z

add X22519 and X448

yuzexiyzx · 2024-08-20T01:32:18Z

@jforissier @jenswi-linaro Do you have any comments?

jenswi-linaro

The commit message could have a few more words about what this patch does. The comment at the start of mpre_mongomery.c, "Kunpeng hardware accelerator hpre montgomery algorithm implementation", gives an idea of what's going on.

jenswi-linaro · 2024-08-20T06:03:40Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+
+	ret = hpre_bin_from_crypto_bin(p, p, msg->key_bytes, msg->curve_bytes);
+	if (ret) {
+		EMSG("Fail to transfer montgomery p from crypto_bin to


Please keep this print in one line, same goes the for other prints. It's OK if this line with the format string becomes longer than 80.

etienne-lms · 2024-08-21T15:53:05Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+
+/* NID_X25519 */
+/* p = (2 ^ 255 - 19) big endian */
+static uint8_t g_x25519_p[] = {


etienne-lms · 2024-08-21T15:53:27Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+	0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05
+};
+
+static struct hpre_mgm_curve g_curve_list[] = {


const also.

This change will also request the related argument of hpre_montgomery_params_fill() and hpre_montgomery_request_init() is also const.

core/drivers/crypto/hisilicon/hpre_montgomery.c

+
+	return TEE_SUCCESS;
+pub_err:
+	free(key->priv);


etienne-lms · 2024-08-21T15:56:55Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+		return HISI_QM_DRVCRYPT_EINVAL;
+	}
+
+	return 0;


return HISI_QM_DRVCRYPT_NO_ERR

etienne-lms · 2024-08-21T15:57:29Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+	shift = bsize - p_size;
+	ret = memcmp(u + shift, p + shift, p_size);
+	if (ret >= 0) {
+		EMSG("u > p");


> or >=?

etienne-lms · 2024-08-21T16:05:40Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+	return ret;
+}
+
+static struct drvcrypt_montgomery driver_x25519 = {


etienne-lms · 2024-08-21T16:08:29Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+{
+	struct hisi_qp *montgomery_qp;
+	TEE_Result res = TEE_SUCCESS;
+	int32_t ret = 0;


Type should be enum hisi_drv_status.

res and ret labels are very similar, it can be confusing. Could you rename one of them? For example:

- int32_t ret = 0; + enum hisi_drv_status status = HISI_QM_DRVCRYPT_NO_ERR;

etienne-lms · 2024-08-21T16:10:21Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+	ret = hpre_montgomery_do_task(&msg);
+	if (ret) {
+		EMSG("Fail to do montgomery shared secret task! ret = 0x%"
+		     PIRX16, ret);


s/PIRX16/PRIX32/

etienne-lms · 2024-08-21T16:10:48Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+
+	ret = hpre_montgomery_do_task(&msg);
+	if (ret) {
+		EMSG("Fail to do montgomery key pair task ret = 0x%"PRIX16,


s/PRIX16/PRIX32

etienne-lms · 2024-08-21T16:14:27Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+					       uint8_t *privkey,
+					       uint8_t *pubkey)
+{
+	TEE_Result ret = TEE_SUCCESS;


ret should be of type enum hisi_drv_status.

You forgot to address this comment.

Sorry,missed this.
Solved.

etienne-lms · 2024-08-26T08:06:50Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+
+static TEE_Result hpre_montgomery_do_task(struct hpre_montgomery_msg *msg)
+{
+	struct hisi_qp *montgomery_qp;


etienne-lms · 2024-08-26T08:09:00Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+
+	ret = hpre_montgomery_do_task(&msg);
+	if (ret) {
+		EMSG("Fail to do montgomery shared secret task! ret = 0x%"PIRX32,


s/PIRX32/PRIX32/

etienne-lms · 2024-08-26T08:10:53Z

core/drivers/crypto/hisilicon/hpre_montgomery.h

+	uint32_t result;
+};
+
+TEE_Result hpre_montgomery_init(void);


I don't think exporting this function is needed, it's called from an initcall level. It could be static within hpre_montgomery.c.

etienne-lms · 2024-08-26T08:12:00Z

core/drivers/crypto/hisilicon/hpre_montgomery.h

+#ifndef __HPRE_MONTGOMERYH__
+#define __HPRE_MONTGOMERYH__
+
+#include <stdbool.h>


It seems stdbool.h is no more needed.

yuzexiyzx · 2024-08-27T09:24:04Z

@jenswi-linaro @etienne-lms All comments have been solved.

yuzexiyzx · 2024-08-29T07:11:52Z

@jenswi-linaro @etienne-lms All comments have been solved.

jenswi-linaro · 2024-08-29T07:55:41Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+	 * byte to zero instead.
+	 */
+	if (msg->key_bytes == BITS_TO_BYTES(X25519_KEY_BITS)) {
+		dst[31] &= 248;


Please add a comment explaining that this is for the X25519 case with a 32-byte integer.
It's a bit curious that masking is done using decimal values instead of hexadecimal, it might be worth mentioning that's how it's done in the example in the RFC.

jenswi-linaro · 2024-08-29T07:59:10Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+		dst[0] |= 64;
+		u[0] &= 0x7F;
+	} else {
+		dst[55 + bsize - dsize] &= 252;


Please add a comment explaining that this is for the X448 case with a 56-byte integer.
Why is bsize - dsize needed?

jenswi-linaro · 2024-08-29T07:59:47Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+	struct hpre_montgomery_msg *msg = info;
+	struct hpre_sqe *sqe = bd;
+
+	sqe->w0 = msg->alg_type | (0x1 << HPRE_DONE_SHIFT);


Prefer SHIFT_U32(0x1, HPRE_DONE_SHIFT)

jenswi-linaro · 2024-08-29T08:01:23Z

core/drivers/crypto/hisilicon/sub.mk

@@ -6,3 +6,4 @@ srcs-y += sec_cipher.c
 srcs-$(CFG_HISILICON_ACC_V3) += hpre_main.c
 srcs-$(CFG_HISILICON_ACC_V3) += hpre_dh.c
 srcs-$(CFG_HISILICON_ACC_V3) += hpre_ecc.c
+srcs-$(CFG_HISILICON_ACC_V3) += hpre_montgomery.c


Please add the missing end-of-line character.

yuzexiyzx · 2024-08-30T09:13:03Z

@jenswi-linaro @etienne-lms All comments have been solved.

yuzexiyzx · 2024-09-02T03:17:25Z

@jenswi-linaro @etienne-lms All comments have been solved.

etienne-lms · 2024-09-02T10:00:26Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+					       uint8_t *privkey,
+					       uint8_t *pubkey)
+{
+	TEE_Result ret = TEE_SUCCESS;


You forgot to address this comment.

etienne-lms · 2024-09-02T10:06:57Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+	err1 = HPRE_TASK_ETYPE1(sqe->w0);
+	done = HPRE_TASK_DONE(sqe->w0);
+	if (done != HPRE_HW_TASK_DONE || err || err1) {
+		EMSG("HPRE do x_dh fail! done=0x%"PRIX16", etype=0x%"PRIX16",etype1=0x%"PRIX16, done, err, err1);


For coding style:

EMSG("HPRE do x_dh fail! done=0x%"PRIX16", etype=0x%"PRIX16", etype1=0x%"PRIX16, done, err, err1);

etienne-lms · 2024-09-02T10:08:41Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+
+	ret = hpre_montgomery_do_task(&msg);
+	if (ret) {
+		EMSG("Fail to do montgomery shared secret task! ret = 0x%"PRIX32, ret);


EMSG("Fail to do montgomery shared secret task! ret = %#"PRIX32, ret);

etienne-lms · 2024-09-02T10:10:22Z

core/drivers/crypto/hisilicon/hpre_montgomery.h

+ * Copyright (c) 2024 HiSilicon Limited.
+ */
+#ifndef __HPRE_MONTGOMERYH__
+#define __HPRE_MONTGOMERYH__


s/__HPRE_MONTGOMERYH__/__HPRE_MONTGOMERY_H__/

jenswi-linaro · 2024-09-03T06:01:34Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+	uint8_t *u = msg->in;
+	uint8_t *dst = k;
+	uint32_t bsize = msg->key_bytes;
+	uint32_t dsize = msg->curve_bytes;


What's the difference between dsize and psize? They always hold the same value.

psize is the size of public key. Its value is the same as curve_bytes.

maybe I can just use dsize here

jenswi-linaro · 2024-09-03T06:06:03Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+		 * to 576 when curve_bytes is between 384 and 576, and the high-order
+		 * bits will be set to 0.
+		 */
+		dst[55 + bsize - dsize] &= 0xFC;


I never got an answer to why bsize - dsize is needed. Doesn't dst[55 + bsize - dsize] address out of range of dst[]?

It is our hardware limit that key_bytes will be set to 72(576/8) when curve_bytes is between 48 and 72.So, dsize is 56 here, and bsize is 72.

jenswi-linaro · 2024-09-03T06:07:31Z

core/drivers/crypto/hisilicon/hpre_montgomery.c

+		dst[0 + bsize - dsize] |= 0x80;
+	}
+
+	shift = bsize - psize;


How would you describe shift, what does it represent?

Shift means the offset I need to take into accout when I deal with the data related to protocol

due to hardware limit mentioned above

It sounds like offset would be a better name. Please add a comment describing how this variable is used.

jenswi-linaro · 2024-09-05T07:21:58Z

Acked-by: Jens Wiklander <[email protected]>

yuzexiyzx · 2024-09-06T02:03:05Z

@etienne-lms All comments have been solved

add operation of X25519 and X448 algorithm, including alloc_keypair, gen_keypar and shared_secret Signed-off-by: yuzexi <[email protected]> Acked-by: Jens Wiklander <[email protected]> Acked-by: Etienne Carriere <[email protected]>

etienne-lms

Looks good to me.

yuzexiyzx · 2024-09-09T07:55:56Z

@jforissier All comments have been solved.

jenswi-linaro reviewed Aug 20, 2024

View reviewed changes

etienne-lms reviewed Aug 22, 2024

View reviewed changes

yuzexiyzx force-pushed the master branch 3 times, most recently from 062673d to dab1242 Compare August 26, 2024 07:22

etienne-lms reviewed Aug 26, 2024

View reviewed changes

yuzexiyzx force-pushed the master branch 4 times, most recently from df95b85 to 5d2eb2f Compare August 27, 2024 09:23

yuzexiyzx force-pushed the master branch from 5d2eb2f to 67cabd0 Compare August 27, 2024 11:03

jenswi-linaro reviewed Aug 29, 2024

View reviewed changes

yuzexiyzx force-pushed the master branch from 67cabd0 to bd4b22a Compare August 30, 2024 09:11

etienne-lms reviewed Sep 2, 2024

View reviewed changes

yuzexiyzx force-pushed the master branch from bd4b22a to 808fd8f Compare September 2, 2024 11:46

jenswi-linaro reviewed Sep 3, 2024

View reviewed changes

yuzexiyzx force-pushed the master branch 2 times, most recently from eb4c4a9 to 1f5bc64 Compare September 5, 2024 02:03

driver: crypto: hisilicon: add montgomery algorithm

ff2600a

add operation of X25519 and X448 algorithm, including alloc_keypair, gen_keypar and shared_secret Signed-off-by: yuzexi <[email protected]> Acked-by: Jens Wiklander <[email protected]> Acked-by: Etienne Carriere <[email protected]>

yuzexiyzx force-pushed the master branch from 1f5bc64 to ff2600a Compare September 9, 2024 01:52

etienne-lms approved these changes Sep 9, 2024

View reviewed changes

jforissier merged commit a420305 into OP-TEE:master Sep 9, 2024
7 of 8 checks passed

driver: crypto: hisilicon: add X25519 and X448 algorithm #7001

driver: crypto: hisilicon: add X25519 and X448 algorithm #7001

Conversation

yuzexiyzx commented Aug 19, 2024

yuzexiyzx commented Aug 20, 2024

jenswi-linaro left a 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

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms Aug 21, 2024 • edited Loading

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

yuzexiyzx commented Aug 27, 2024

yuzexiyzx commented Aug 29, 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

yuzexiyzx commented Aug 30, 2024

yuzexiyzx commented Sep 2, 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

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 Sep 5, 2024

yuzexiyzx commented Sep 6, 2024

etienne-lms left a comment

Choose a reason for hiding this comment

yuzexiyzx commented Sep 9, 2024

etienne-lms Aug 21, 2024 •

edited

Loading