RE: [v1, 2/3] test/test_cryptodev_asym: add SM2 tests

[Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>]

Hi Akhil,
> 
> If you have the reference from where the vectors are taken it can also be
> mentioned.
> 
I generated these test vectors in online. There are no reference test vectors quoted
In RFC except for recommended curve parameters, at the end.
https://datatracker.ietf.org/doc/html/draft-shen-sm2-ecdsa-02

> >
> > Signed-off-by: Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>
> > ---
> >  app/test/test_cryptodev_asym.c             | 506 +++++++++++++++++++++
> >  app/test/test_cryptodev_sm2_test_vectors.h | 120 +++++
> >  2 files changed, 626 insertions(+)
> >  create mode 100644 app/test/test_cryptodev_sm2_test_vectors.h
> >
> > diff --git a/app/test/test_cryptodev_asym.c
> > b/app/test/test_cryptodev_asym.c index 9236817650..bfaeedee27 100644
> > --- a/app/test/test_cryptodev_asym.c
> > +++ b/app/test/test_cryptodev_asym.c
> > @@ -21,6 +21,7 @@
> >  #include "test_cryptodev_ecpm_test_vectors.h"
> >  #include "test_cryptodev_mod_test_vectors.h"
> >  #include "test_cryptodev_rsa_test_vectors.h"
> > +#include "test_cryptodev_sm2_test_vectors.h"
> >  #include "test_cryptodev_asym_util.h"
> >  #include "test.h"
> >
> > @@ -2196,6 +2197,507 @@ test_ecpm_all_curve(void)
> >  	return overall_status;
> >  }
> >
> > +static int
> > +test_sm2_sign(void)
> > +{
> > +	struct crypto_testsuite_params_asym *ts_params = &testsuite_params;
> > +	struct crypto_testsuite_sm2_params input_params =
> > sm2_param_fp256;
> > +	struct rte_mempool *sess_mpool = ts_params->session_mpool;
> > +	struct rte_mempool *op_mpool = ts_params->op_mpool;
> > +	uint8_t dev_id = ts_params->valid_devs[0];
> > +	struct rte_crypto_op *result_op = NULL;
> > +	uint8_t output_buf_r[TEST_DATA_SIZE];
> > +	uint8_t output_buf_s[TEST_DATA_SIZE];
> > +	struct rte_crypto_asym_xform xform;
> > +	struct rte_crypto_asym_op *asym_op;
> > +	struct rte_cryptodev_info dev_info;
> > +	struct rte_crypto_op *op = NULL;
> > +	int ret, status = TEST_SUCCESS;
> > +	void *sess = NULL;
> > +
> > +	rte_cryptodev_info_get(dev_id, &dev_info);
> 
> dev_info is being unused. Not checking for capabilities?

It is the same case in other algorithms as well. Shall I collectively address this ?
> > +
> > +	/* Setup crypto op data structure */
> > +	op = rte_crypto_op_alloc(op_mpool,
> > RTE_CRYPTO_OP_TYPE_ASYMMETRIC);
> > +	if (op == NULL) {
> > +		RTE_LOG(ERR, USER1,
> > +				"line %u FAILED: %s", __LINE__,
> > +				"Failed to allocate asymmetric crypto "
> > +				"operation struct\n");
> > +		status = TEST_FAILED;
> > +		goto exit;
> > +	}
> > +	asym_op = op->asym;
> > +
> > +	/* Setup asym xform */
> > +	xform.next = NULL;
> > +	xform.xform_type = RTE_CRYPTO_ASYM_XFORM_SM2;
> > +	xform.sm2.pkey.data = input_params.pkey.data;
> > +	xform.sm2.pkey.length = input_params.pkey.length;
> > +	xform.sm2.q.x.data = input_params.pubkey_qx.data;
> > +	xform.sm2.q.x.length = input_params.pubkey_qx.length;
> > +	xform.sm2.q.y.data = input_params.pubkey_qy.data;
> > +	xform.sm2.q.y.length = input_params.pubkey_qy.length;
> > +
> > +	ret = rte_cryptodev_asym_session_create(dev_id, &xform, sess_mpool,
> > &sess);
> > +	if (ret < 0) {
> > +		RTE_LOG(ERR, USER1,
> > +				"line %u FAILED: %s", __LINE__,
> > +				"Session creation failed\n");
> > +		status = (ret == -ENOTSUP) ? TEST_SKIPPED : TEST_FAILED;
> > +		goto exit;
> > +	}
> > +
> > +	/* Attach asymmetric crypto session to crypto operations */
> > +	rte_crypto_op_attach_asym_session(op, sess);
> > +
> > +	/* Compute sign */
> > +
> > +	/* Populate op with operational details */
> > +	op->asym->sm2.op_type = RTE_CRYPTO_ASYM_OP_SIGN;
> > +	op->asym->sm2.message.data = input_params.message.data;
> > +	op->asym->sm2.message.length = input_params.message.length;
> > +	op->asym->sm2.id.data = input_params.id.data;
> > +	op->asym->sm2.id.length = input_params.id.length;
> > +
> > +	/* Init out buf */
> > +	op->asym->sm2.r.data = output_buf_r;
> > +	op->asym->sm2.s.data = output_buf_s;
> > +
> > +	RTE_LOG(DEBUG, USER1, "Process ASYM operation\n");
> > +
> > +	/* Process crypto operation */
> > +	if (rte_cryptodev_enqueue_burst(dev_id, 0, &op, 1) != 1) {
> > +		RTE_LOG(ERR, USER1,
> > +				"line %u FAILED: %s", __LINE__,
> > +				"Error sending packet for operation\n");
> > +		status = TEST_FAILED;
> > +		goto exit;
> > +	}
> > +
> > +	while (rte_cryptodev_dequeue_burst(dev_id, 0, &result_op, 1) == 0)
> > +		rte_pause();
> 
> Shouldn't this be a finite loop and mark test as failed after some retries?

It is the same case in other algorithms as well. Shall I collectively address this ?
> 
> > +
> > +	if (result_op == NULL) {
> > +		RTE_LOG(ERR, USER1,
> > +				"line %u FAILED: %s", __LINE__,
> > +				"Failed to process asym crypto op\n");
> > +		status = TEST_FAILED;
> > +		goto exit;
> > +	}
> > +
...
<cut>
...
> > +static int
> > +test_sm2_encrypt(void)
> > +{
> > +	struct crypto_testsuite_params_asym *ts_params = &testsuite_params;
> > +	struct crypto_testsuite_sm2_params input_params =
> > sm2_param_fp256;
> > +	struct rte_mempool *sess_mpool = ts_params->session_mpool;
> > +	struct rte_mempool *op_mpool = ts_params->op_mpool;
> > +	uint8_t output_buf[TEST_DATA_SIZE], *pbuf = NULL;
> > +	uint8_t dev_id = ts_params->valid_devs[0];
> > +	struct rte_crypto_op *result_op = NULL;
> > +	struct rte_crypto_asym_xform xform;
> > +	struct rte_crypto_asym_op *asym_op;
> > +	struct rte_cryptodev_info dev_info;
> > +	struct rte_crypto_op *op = NULL;
> > +	int ret, status = TEST_SUCCESS;
> > +	void *sess = NULL;
> > +
> > +	rte_cryptodev_info_get(dev_id, &dev_info);
> > +
> > +	/* Setup crypto op data structure */
> > +	op = rte_crypto_op_alloc(op_mpool,
> > RTE_CRYPTO_OP_TYPE_ASYMMETRIC);
> > +	if (op == NULL) {
> > +		RTE_LOG(ERR, USER1,
> > +				"line %u FAILED: %s", __LINE__,
> > +				"Failed to allocate asymmetric crypto "
> > +				"operation struct\n");
> > +		status = TEST_FAILED;
> > +		goto exit;
> > +	}
> > +	asym_op = op->asym;
> > +
> > +	/* Setup asym xform */
> > +	xform.next = NULL;
> > +	xform.xform_type = RTE_CRYPTO_ASYM_XFORM_SM2;
> > +	xform.sm2.pkey.data = input_params.pkey.data;
> > +	xform.sm2.pkey.length = input_params.pkey.length;
> > +	xform.sm2.q.x.data = input_params.pubkey_qx.data;
> > +	xform.sm2.q.x.length = input_params.pubkey_qx.length;
> > +	xform.sm2.q.y.data = input_params.pubkey_qy.data;
> > +	xform.sm2.q.y.length = input_params.pubkey_qy.length;
> > +
> > +	ret = rte_cryptodev_asym_session_create(dev_id, &xform, sess_mpool,
> > &sess);
> > +	if (ret < 0) {
> > +		RTE_LOG(ERR, USER1,
> > +				"line %u FAILED: %s", __LINE__,
> > +				"Session creation failed\n");
> > +		status = (ret == -ENOTSUP) ? TEST_SKIPPED : TEST_FAILED;
> > +		goto exit;
> > +	}
> > +
> > +	/* Attach asymmetric crypto session to crypto operations */
> > +	rte_crypto_op_attach_asym_session(op, sess);
> > +
> > +	/* Compute encrypt */
> > +
> > +	/* Populate op with operational details */
> > +	op->asym->sm2.op_type = RTE_CRYPTO_ASYM_OP_ENCRYPT;
> > +	op->asym->sm2.message.data = input_params.message.data;
> > +	op->asym->sm2.message.length = input_params.message.length;
> > +
> > +	/* Init out buf */
> > +	op->asym->sm2.cipher.data = output_buf;
> > +
> > +	RTE_LOG(DEBUG, USER1, "Process ASYM operation\n");
> > +
> > +	/* Process crypto operation */
> > +	if (rte_cryptodev_enqueue_burst(dev_id, 0, &op, 1) != 1) {
> > +		RTE_LOG(ERR, USER1,
> > +				"line %u FAILED: %s", __LINE__,
> > +				"Error sending packet for operation\n");
> > +		status = TEST_FAILED;
> > +		goto exit;
> > +	}
> > +
> > +	while (rte_cryptodev_dequeue_burst(dev_id, 0, &result_op, 1) == 0)
> > +		rte_pause();
> > +
> > +	if (result_op == NULL) {
> > +		RTE_LOG(ERR, USER1,
> > +				"line %u FAILED: %s", __LINE__,
> > +				"Failed to process asym crypto op\n");
> > +		status = TEST_FAILED;
> > +		goto exit;
> > +	}
> > +
> > +	if (result_op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> > +		RTE_LOG(ERR, USER1,
> > +				"line %u FAILED: %s", __LINE__,
> > +				"Failed to process asym crypto op\n");
> > +		status = TEST_FAILED;
> > +		goto exit;
> > +	}
> 
> Shouldn't we do content comparison of the cipher text with vectors here as
> well?
> Content is matched only after round trip which will not catch the hardware bug
> during encryption.

Due to random scalar value used (and known only to driver), we can't have expected
cipher string. I ll wait for review on current openssl driver implementation whether
to introduce this scalar value in crypto op param like in EC. Based on this, we could 
address this suggestion.

Thanks.
>