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

[Akhil Goyal <gakhil@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
>
ok
 
> > >
> > > 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 ?
You can remove this dev_info get from here and add capability check in a separate patch for all cases.


> > > +
> > > +	/* 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 ?
Ok you can submit a separate patch for it.

> >
> > > +
> > > +	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.
> 
You can add a comment here in the code to explain this thing.