driver: crypto: hisilicon: add rsa algorithm #7123

yuzexiyzx · 2024-11-13T03:42:17Z

add rsa algorithm

jenswi-linaro

The commit message needs to be fixed up. It looks like it was removed by mistake.

jenswi-linaro · 2024-11-13T15:34:25Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	switch (msg->alg_type) {
+	case HPRE_ALG_KG_CRT:
+		memzero_explicit(msg->in, msg->key_bytes << 1);
+		free(msg->in);


Can free_wipe() be used instead of memzero_explicit()+free()?

jenswi-linaro · 2024-11-14T09:39:28Z

core/drivers/crypto/hisilicon/hpre_rsa.h

+	paddr_t pubkey_dma;
+	uint8_t *prikey;
+	paddr_t prikey_dma;
+	uint8_t *in;


It would be nice with a summary of all the expected sizes of these buffers and how they relate.

It is different in encrypt and decrypt，also in crt and ncrt. Expected size of these buffers and how they relate can be seen in alloc function of every mode, such as hpre_rsa_crt_decrypt_alloc.

It sounds like it's even more important since it makes it harder to work out backward.

Would it make sense to describe the buffer layout for HPRE_RSA_CRT_TOTAL_BUF_SIZE() and HPRE_RSA_NCRT_TOTAL_BUF_SIZE() above?

jenswi-linaro · 2024-11-14T09:39:41Z

core/drivers/crypto/hisilicon/hpre_rsa.h

+#include <tee_api_types.h>
+#include <types_ext.h>
+
+#define PKCS_V1_5_MSG_MIN_LEN 11


Please align all the values

jenswi-linaro · 2024-11-14T09:40:15Z

core/drivers/crypto/hisilicon/hpre_rsa.h

@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: BSD-2-Clause */
+/*
+ * Copyright (c) 2022, HiSilicon Technologies Co., Ltd.


It's 2024 now.

jenswi-linaro · 2024-11-14T09:40:33Z

core/drivers/crypto/hisilicon/sub.mk

@@ -8,3 +8,5 @@ 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
+srcs-$(CFG_HISILICON_ACC_V3) += hpre_rsa.c
+


Unnecessary empty line.

You missed this review comment.

etienne-lms · 2024-11-13T09:24:09Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	mbedtls_mpi_free(&G);
+	mbedtls_mpi_free(&L);
+
+	return ret;


ret is always 0. Could change this to return 0?

etienne-lms · 2024-11-13T09:24:46Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+
+		if (mbedtls_mpi_bitlen(&rsa.D) <= ((nbits + 1) >> 1))
+			continue;
+		break;


suggestion:

if (mbedtls_mpi_bitlen(&rsa.D) > ((nbits + 1) >> 1)) break;

etienne-lms · 2024-11-13T09:26:59Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	else if (key_bits <= 4096)
+		size = BITS_TO_BYTES(4096);
+	else
+		EMSG("Invalid key_bits[%"PRIu64"]", key_bits);


s/%"PRIu64"/%zu/

etienne-lms · 2024-11-14T14:21:02Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+#include <string_ext.h>
+#include <trace.h>
+#include <mbedtls/rsa.h>
+#include <tee/tee_cryp_utl.h>


etienne-lms · 2024-11-14T14:22:23Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+		return HISI_QM_DRVCRYPT_NO_ERR;
+	case HPRE_ALG_NC_NCRT:
+	case HPRE_ALG_NC_CRT:
+		if (msg->is_private) { /* DECRYPT */


prefer

if (msg->is_private) { /* DECRYPT */ ... } else { /* ENCRYPT */ ... }

etienne-lms · 2024-11-14T14:29:45Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	TEE_Result ret = TEE_SUCCESS;
+
+	if (rsa_data->message.length > n_bytes) {
+		EMSG("Invalid msg length[%"PRIu64"]", rsa_data->message.length);


%zu for size_t type.

etienne-lms · 2024-11-14T14:32:18Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	struct drvcrypt_rsa_ed rsa_enc_info = { };
+	TEE_Result ret = TEE_SUCCESS;
+
+	memcpy(&rsa_enc_info, rsa_data, sizeof(rsa_enc_info));


prefer rsa_enc_info = *rsa_data;
or simply struct drvcrypt_rsa_ed rsa_enc_info = *rsa_data; at line 717.

Ditto at lines 845, 1191 and 1327.

etienne-lms · 2024-11-14T14:35:25Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	if (ret)
+		return ret;
+
+	(void)hw_get_random_bytes(seed, lhash_len);


For consistency, prefer test the return code, even if hw_get_random_bytes() from hi16xx_rng.c always succeeds.

yuzexiyzx · 2024-11-28T09:13:54Z

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

etienne-lms

Could you fix the build issues reported by CI / make?

etienne-lms · 2024-11-28T11:35:00Z

core/drivers/crypto/hisilicon/sub.mk

@@ -8,3 +8,5 @@ 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
+srcs-$(CFG_HISILICON_ACC_V3) += hpre_rsa.c
+


You missed this review comment.

yuzexiyzx · 2024-11-30T03:39:06Z

@etienne-lms Sorry, I do not know how to deal with this. I can find member P and Q in mbedtls code, and there is no such error when I compile it. Could you please give me some suggestion? Thank you very much

jenswi-linaro · 2024-12-02T09:14:32Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+static int32_t hpre_rsa_gen_p_q(size_t nbits, struct rsa_keypair *key)
+{
+	int32_t prime_quality = 0;
+	mbedtls_rsa_context rsa = { };


You should use the crypto API from <crypto/crypto.h> as an abstraction instead of reaching directly into mbedtls. sw_crypto_acipher_gen_rsa_key() might be useful here.

But I just want to generate p and q here, and then generate rsa_keypair from p and q by hardware.
sw_crypto_acipher_gen_rsa_key() might generate the whole rsa_keypair directly.

You've already generated a keypair before calling hpre_rsa_fill_p_q(), only the CRT parameters remain.
The heavy lifting has already been done, even if you decide to throw away D and E. What is hpre_rsa_fill_p_q() supposed to do? Recreate D and E and then deduce the CRT parameters?
By the way, hpre_rsa_gen_p_q() looks like a slightly hacked-up copy of mbedtls_rsa_gen_key().

When we call hpre_rsa_gen_keypiar, we already have key->e.
We hope hpre_rsa_keypair _init only generate key->p and key->q,The function of hpre_rsa_fill_p_q is to copy p and q to rsa_keypair.
And then, hpre_rsa_init and hpre_rsa_do_task use hardware to calculate N,D(ncrt mode) or dP,dQ ,qInv,N,D(crt mode).
The input is key->e,key->p and key->q.

Do you mean I can use mbedtls_rsa_gen_key() instead of hpre_rsa_gen_p_q, and just throw away D and E?

When we call hpre_rsa_gen_keypiar, we already have key->e.

You're right, that's an input parameter.

We hope hpre_rsa_keypair _init only generate key->p and key->q,The function of hpre_rsa_fill_p_q is to copy p and q to rsa_keypair. And then, hpre_rsa_init and hpre_rsa_do_task use hardware to calculate N,D(ncrt mode) or dP,dQ ,qInv,N,D(crt mode). The input is key->e,key->p and key->q.

The process to generate P and Q involves a few checks to see that the values are good. This is an iterative process that is restarted if any of the tests fail. Checking that the produced D is large enough is one of the checks. So the assumption that you can ask for just two good values for P and Q leaves a few holes.

What is the purpose of using hardware to calculate N, D, and the CRT parameters? Is it more secure? Is it faster?

Without trying to benchmark hpre_rsa_gen_p_q() I believe that most of the time is spent in the calls to mbedtls_mpi_gen_prime() when generating the key.

Do you mean I can use mbedtls_rsa_gen_key() instead of hpre_rsa_gen_p_q, and just throw away D and E?

No, but if that's the best you can do with the hardware when generating an RSA key why bother with the hardware?

So, I should do rsa_gen_keypair just with software, not implement this mode with hardware?

I mean I should just not register hpre_rsa_genkeypair or call mbedtls_rsa_gen_key() in hpre_rsa_genkeypair without hardware.

So, I should do rsa_gen_keypair just with software, not implement this mode with hardware?

Yes, we can do that for now. We can improve that later if someone comes up with something better.

The easiest should be to register .gen_keypair = sw_crypto_acipher_gen_rsa_key. Please look at the other sw_crypto_acipher_*() functions. It looks like for instance sw_crypto_acipher_alloc_rsa_keypair() and sw_crypto_acipher_free_rsa_keypair() does the same thing as hpre_rsa_allocate_keypair() and hpre_rsa_free_keypair().

Get it. Thank you very much.

jenswi-linaro

Please fix the build error and the checkpatch warning that isn't about a print.

jenswi-linaro · 2024-12-10T07:38:54Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	return drvcrypt_xor_mod_n(&xor_mod);
+}
+
+static TEE_Result hpre_rsa_allocate_publickey(struct rsa_public_key *key,


sw_crypto_acipher_alloc_rsa_public_key() can be used instead of this function.

jenswi-linaro · 2024-12-10T07:39:12Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	return TEE_SUCCESS;
+}
+
+static void hpre_rsa_free_publickey(struct rsa_public_key *key)


sw_crypto_acipher_free_rsa_public_key() can be used instead of this function.

jenswi-linaro · 2024-12-10T07:43:57Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	return ret;
+}
+
+static int32_t hpre_rsa_is_crt_mod(struct rsa_keypair *key)


Please use bool instead of int32_t.

jenswi-linaro · 2024-12-10T07:44:06Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	    key->dp && crypto_bignum_num_bits(key->dp) &&
+	    key->dq && crypto_bignum_num_bits(key->dq) &&
+	    key->qp && crypto_bignum_num_bits(key->qp))
+		is_crt = 1;


return true;

jenswi-linaro · 2024-12-10T07:44:16Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	    key->qp && crypto_bignum_num_bits(key->qp))
+		is_crt = 1;
+
+	return is_crt;


return false;

jenswi-linaro · 2024-12-10T08:01:28Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+		msg->alg_type = HPRE_ALG_NC_CRT;
+		ret = hpre_rsa_crt_decrypt_alloc(msg);
+		if (ret)
+			return TEE_ERROR_BAD_STATE;


Why TEE_ERROR_BAD_STATE and not TEE_ERROR_OUT_OF_MEMORY?

You missed this comment.

jenswi-linaro · 2024-12-10T08:18:02Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	msg->pubkey = data;
+	msg->pubkey_dma = virt_to_phys(msg->pubkey);
+
+	msg->in = data + (msg->key_bytes << 1);


Why do you use << 1 instead of * 2?
I prefer * 2 since we care about the value here, not the position of the bits.

jenswi-linaro · 2024-12-10T08:19:09Z

core/drivers/crypto/hisilicon/sub.mk

@@ -8,3 +8,4 @@ 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
+srcs-$(CFG_HISILICON_ACC_V3) += hpre_rsa.c


Please add the missing newline at the end of the file.

jenswi-linaro · 2024-12-10T08:45:44Z

core/drivers/crypto/hisilicon/hpre_rsa.h

+#define OAEP_MAX_HASH_LEN			64
+#define OAEP_MAX_DB_LEN				512
+#define PRIME_QUALITY_FLAG			1024
+#define HPRE_RSA_GEN_TOTAL_BUF_SIZE(kbytes)	((kbytes) * 7)


This macro is unused. Will it be needed in later patches?

jenswi-linaro · 2024-12-10T08:48:40Z

core/drivers/crypto/hisilicon/hpre_rsa.h

+	paddr_t pubkey_dma;
+	uint8_t *prikey;
+	paddr_t prikey_dma;
+	uint8_t *in;


Would it make sense to describe the buffer layout for HPRE_RSA_CRT_TOTAL_BUF_SIZE() and HPRE_RSA_NCRT_TOTAL_BUF_SIZE() above?

etienne-lms

In commit message header line: s/rsa/RSA/
In commit message body: prefer a more explicit description (even if it rephrases the header line), e.g. Add RSA support in HiSilicon crypto drivers.

etienne-lms · 2024-12-10T08:41:34Z

core/drivers/crypto/hisilicon/hpre_rsa.h

+ */
+
+#ifndef __TEE_HPRE_RSA_H__
+#define __TEE_HPRE_RSA_H__


Prefer __HPRE_RSA_H__ or __HISILICON_HPRE_RSA_H__ as per header file name.

etienne-lms · 2024-12-10T08:49:13Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	rsa_qp->parse_sqe = hpre_rsa_parse_sqe;
+	ret = hisi_qp_send(rsa_qp, msg);
+	if (ret) {
+		EMSG("Fail to send task, ret=%d", ret);


ret=%x"PRIx32, ret);
Ditto at line 103

etienne-lms · 2024-12-10T08:50:41Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+
+static void hpre_rsa_params_free(struct hpre_rsa_msg *msg)
+{
+	size_t len = 0;


len is not used (see CI / make (multi-platform) results).

etienne-lms · 2024-12-10T08:55:31Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+{
+	size_t lhash_len = rsa_data->digest_size;
+	uint8_t db[OAEP_MAX_DB_LEN] = { };
+	uint8_t seed[OAEP_MAX_HASH_LEN];


etienne-lms · 2024-12-10T08:57:41Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+
+	msg->prikey = data;
+	msg->prikey_dma = virt_to_phys(msg->prikey);
+	if (!msg->prikey_dma) {


virt_to_phys() cannot fail on a non NULL address returned by calloc().
Ditto at line 614.

etienne-lms · 2024-12-10T09:04:07Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	msg_len = db_len - lp_len - 1;
+	if (msg_len > rsa_data->message.length) {
+		EMSG("Message space is not enough");
+		return TEE_ERROR_SHORT_BUFFER;


Set *out_len = msg_len to help caller to get to required size.

I would replace EMSG() with DMSG() at line 918 since this may not be a error case but an output buffer length query from the caller.

etienne-lms · 2024-12-10T09:06:40Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+		 */
+		.ssa_sign = NULL,
+		.ssa_verify = NULL,
+	},


These explicit NULL inits are not required. But maybe you prefer to have them for the reader information.

yuzexiyzx · 2024-12-16T02:57:06Z

@jenswi-linaro @etienne-lms All comments have been solved.
The errors in code style are all about print.

add RSA support in Hisilicon crypto drivers Signed-off-by: yuzexi <[email protected]>

etienne-lms · 2024-12-16T07:26:37Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+						      struct hpre_sqe *sqe)
+{
+	switch (msg->alg_type) {
+	case HPRE_ALG_KG_CRT: /* KEY GEN */


case HPRE_ALG_KG_CRT: /* KEY GEN */ (...)

etienne-lms · 2024-12-16T07:33:23Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+		msg->alg_type = HPRE_ALG_NC_CRT;
+		ret = hpre_rsa_crt_decrypt_alloc(msg);
+		if (ret)
+			return TEE_ERROR_BAD_STATE;


You missed this comment.

etienne-lms · 2024-12-16T07:38:06Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+	msg_len = db_len - lp_len - 1;
+	if (msg_len > rsa_data->message.length) {
+		EMSG("Message space is not enough");
+		return TEE_ERROR_SHORT_BUFFER;


I would replace EMSG() with DMSG() at line 918 since this may not be a error case but an output buffer length query from the caller.

etienne-lms · 2024-12-16T07:49:45Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+{
+	switch (msg->alg_type) {
+	case HPRE_ALG_KG_CRT:
+		free_wipe(msg->in);


I don't see where msg->in is an allocated pointer. I think at this stage, msg->in is always NULL.

etienne-lms · 2024-12-16T07:50:50Z

core/drivers/crypto/hisilicon/hpre_rsa.c

+
+	ret = hpre_rsa_encrypt_alloc(msg);
+	if (ret)
+		return TEE_ERROR_BAD_STATE;


TEE_ERROR_OUT_OF_MEMORY?

yuzexiyzx force-pushed the master branch from b696293 to d330562 Compare November 13, 2024 06:13

jenswi-linaro reviewed Nov 14, 2024

View reviewed changes

etienne-lms reviewed Nov 15, 2024

View reviewed changes

yuzexiyzx force-pushed the master branch from d330562 to c1800e0 Compare November 20, 2024 09:12

etienne-lms reviewed Nov 28, 2024

View reviewed changes

jenswi-linaro reviewed Dec 2, 2024

View reviewed changes

yuzexiyzx force-pushed the master branch 2 times, most recently from c57b4ca to 3a3e653 Compare December 10, 2024 03:29

jenswi-linaro reviewed Dec 10, 2024

View reviewed changes

etienne-lms reviewed Dec 10, 2024

View reviewed changes

yuzexiyzx force-pushed the master branch 3 times, most recently from ac80ae4 to 887f761 Compare December 16, 2024 02:48

driver: crypto: hisilicon: add RSA algorithm

2298fec

add RSA support in Hisilicon crypto drivers Signed-off-by: yuzexi <[email protected]>

yuzexiyzx force-pushed the master branch from 887f761 to 2298fec Compare December 16, 2024 07:20

etienne-lms reviewed Dec 16, 2024

View reviewed changes

driver: crypto: hisilicon: add rsa algorithm #7123

Are you sure you want to change the base?

driver: crypto: hisilicon: add rsa algorithm #7123

Conversation

yuzexiyzx commented Nov 13, 2024 • edited Loading

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

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

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 Nov 28, 2024

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuzexiyzx commented Nov 30, 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

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

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

etienne-lms 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

yuzexiyzx commented Nov 13, 2024 •

edited

Loading