driver: crypto: hisilicon: add ECC gen_keypair and ECDH #6807

yuzexiyzx · 2024-04-28T02:56:39Z

add ECC gen_keypair and shared secret

jenswi-linaro · 2024-05-03T09:12:02Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+};
+
+/* NID_X9_62_prime192v1 */
+static uint8_t g_prime192v1_p[] = {


static const?

cannot add const

Is it possible to change struct hpre_ecc_curve to use const pointers instead or is that struct sometimes used for output too?

It would be advantageous to make this data const since it will be protected from accidental overwrite.

jenswi-linaro · 2024-05-03T09:13:44Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+			return false;
+	}
+
+	EMSG("Error: %s all zero", p_name);


It looks like all these EMSG() should be DMSG() instead.

I think this is error msg, and we use EMSG() in other files such as dh.c

jenswi-linaro · 2024-05-03T09:15:50Z

core/drivers/crypto/hisilicon/hpre_ecc.c

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


Please use SHIFT_U32(0x1, HPRE_DONE_SHIFT)

jenswi-linaro · 2024-05-03T09:16:17Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+
+	if (msg->alg_type == HPRE_ALG_SM2_SIGN ||
+	    msg->alg_type == HPRE_ALG_SM2_ENC)
+		sqe->ext1 |= (0x1 << HPRE_SQE_SM2_KSEL_SHIFT);


SHIFT_U32()

jenswi-linaro · 2024-05-03T09:18:43Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+	return TEE_SUCCESS;
+}
+
+static bool is_size_support(size_t key_bits)


key_size_is_supported()?

jenswi-linaro · 2024-05-03T09:21:34Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+		return TEE_ERROR_BAD_PARAMETERS;
+	}
+
+	rand_k = (uint8_t *)malloc(size);


No need to cast void *

jenswi-linaro · 2024-05-03T09:22:56Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+		return TEE_ERROR_BAD_PARAMETERS;
+	}
+
+	ecc_key = (struct ecc_keypair *)sdata->key_priv;


No need to cast void *, same below.

jenswi-linaro · 2024-05-03T09:24:16Z

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

+	uint32_t sm2_mlen;
+	bool sm2_sp;
+	union {
+	struct hpre_ecc_dh ecc_dh;


Please indent these.

jenswi-linaro · 2024-05-03T09:25:57Z

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

+#ifndef __HPRE_ECC_H__
+#define __HPRE_ECC_H__
+
+#define ECC_DH_IN_PARAM_NUM	2


Do you expect these to be used outside hpre_ecc.c or could they be moved into that C file?

jenswi-linaro · 2024-05-03T09:27:15Z

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

+
+struct hpre_ecc_msg {
+	uint8_t *key;
+	uintptr_t key_dma;


paddr_t? Same below.

etienne-lms · 2024-05-03T09:43:32Z

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

+
+#ifndef __HPRE_ECC_H__
+#define __HPRE_ECC_H__
+


#include <stdbool.h> #include <stdint.h> #include <tee_api_type.h> #include <types_ext.h> // for paddr_t suggested by @jenswi-linaro

etienne-lms · 2024-05-03T09:44:32Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+		.x = g_prime192v1_gx,
+		.y = g_prime192v1_gy,
+		.n = g_prime192v1_n,
+	}, {


etienne-lms · 2024-05-03T09:45:20Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+	uint32_t i = 0;
+
+	for (i = 0; i < size; i++) {
+		if (data[i] > 0)


nitpicking: if (data[i])

etienne-lms · 2024-05-03T09:47:16Z

core/drivers/crypto/hisilicon/hpre_ecc.c

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


EMSG("HPRE do ecc fail! done=0x%"PRIx16", etype=0x%"PRIx16", etype1=0x%"RPIx16,

etienne-lms · 2024-05-03T09:47:51Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+static TEE_Result hpre_do_ecc_task(void *msg)
+{
+	struct hisi_qp *ecc_qp = NULL;
+	int32_t ret = TEE_SUCCESS;


enum hisi_drv_status res= HISI_QM_DRVCRYPT_<smoething>;

etienne-lms · 2024-05-03T09:57:36Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+	if (!msg->key_bytes)
+		return TEE_ERROR_BAD_PARAMETERS;
+
+	ret = hpre_ecc_dh_params_alloc(msg);


the function return a enum hisi_drv_status value.

etienne-lms · 2024-05-03T09:59:44Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+
+	curve = get_curve_from_list(key->curve);
+	if (!curve) {
+		EMSG("Fail to get valid curve, id %u.", key->curve);


... %"PRIu32, key->curve);

Ditto at line 914.

etienne-lms · 2024-05-03T10:00:21Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+	}
+
+	if (BITS_TO_BYTES(size_bits) != BITS_TO_BYTES(curve->key_bits)) {
+		EMSG("Invalid size_bits %lu.", size_bits);


... %zu.", size_bits);

Ditto at line 919.

etienne-lms · 2024-05-03T10:00:57Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+
+	ret = hpre_do_ecc_task(&msg);
+	if (ret) {
+		EMSG("Fail to do ecc key pair task! ret = 0x%x.", ret);


... ret = 0x%"PRIx32, ret);

ditto at line 931.

etienne-lms · 2024-05-03T10:04:20Z

core/drivers/crypto/hisilicon/hpre_ecc.c

+	if (ret != TEE_SUCCESS)
+		EMSG("Hpre ecc register to crypto fail");
+
+	return ret;


In case of error, OP-TEE will only print a message and continue initialization. If you want the platform to refuse to boot (and not fallback to a software ECC implementation, you should explicitly panic here.

yuzexiyzx · 2024-05-10T01:55:23Z

@etienne-lms Could you please tell me how to deal with warning like this？

Should I move struct hpre_ecc_curve to next line? I don't know how to deal with alignment like this.

add ECC gen_keypair and shared secret Signed-off-by: yuzexi <yuzexi@hisilicon.com>

etienne-lms · 2024-05-13T15:03:08Z

Hi @yuzexiyzx,
My suggestion:

-static enum hisi_drv_status hpre_ecc_dh_params_fill(struct hpre_ecc_curve *curve,
-						    struct hpre_ecc_msg *msg,
-						    struct bignum *privkey,
-						    struct ecc_public_key *pubkey)
+static enum hisi_drv_status
+hpre_ecc_dh_params_fill(struct hpre_ecc_curve *curve, struct hpre_ecc_msg *msg,
+                        struct bignum *privkey, struct ecc_public_key *pubkey)
 {

jenswi-linaro reviewed May 3, 2024

View reviewed changes

etienne-lms reviewed May 3, 2024

View reviewed changes

yuzexiyzx force-pushed the master branch 3 times, most recently from 7f718ff to 63a2a14 Compare May 10, 2024 01:48

yuzexiyzx force-pushed the master branch from 63a2a14 to 917834d Compare May 10, 2024 06:24

driver: crypto: hisilicon: add ECC gen_keypair and ECDH

3aa8c9b

add ECC gen_keypair and shared secret Signed-off-by: yuzexi <yuzexi@hisilicon.com>

yuzexiyzx force-pushed the master branch from 917834d to 3aa8c9b Compare May 10, 2024 07:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

driver: crypto: hisilicon: add ECC gen_keypair and ECDH #6807

driver: crypto: hisilicon: add ECC gen_keypair and ECDH #6807

yuzexiyzx commented Apr 28, 2024

jenswi-linaro May 3, 2024

yuzexiyzx May 10, 2024

jenswi-linaro May 14, 2024

jenswi-linaro May 3, 2024

yuzexiyzx May 10, 2024

jenswi-linaro May 3, 2024

jenswi-linaro May 3, 2024

jenswi-linaro May 3, 2024

jenswi-linaro May 3, 2024

jenswi-linaro May 3, 2024

jenswi-linaro May 3, 2024

jenswi-linaro May 3, 2024

jenswi-linaro May 3, 2024

etienne-lms May 3, 2024

etienne-lms May 3, 2024

etienne-lms May 3, 2024

etienne-lms May 3, 2024

etienne-lms May 3, 2024

etienne-lms May 3, 2024

etienne-lms May 3, 2024

etienne-lms May 3, 2024

etienne-lms May 3, 2024

etienne-lms May 3, 2024

yuzexiyzx commented May 10, 2024

etienne-lms commented May 13, 2024


		#ifndef __HPRE_ECC_H__
		#define __HPRE_ECC_H__

driver: crypto: hisilicon: add ECC gen_keypair and ECDH #6807

Are you sure you want to change the base?

driver: crypto: hisilicon: add ECC gen_keypair and ECDH #6807

Conversation

yuzexiyzx commented Apr 28, 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

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 May 10, 2024

etienne-lms commented May 13, 2024