RE: [EXT] [RFC] examples/fips_validation: add fips 140-3 acvp aes-cbc test

[Gowrishankar Muthukrishnan <gmuthukrishn@marvell.com>]

Hi Brian,
Thanks for the RFC and PFA inline my comments.

> -----Original Message-----
> From: Brian Dooley <brian.dooley@intel.com>
> Sent: Friday, May 20, 2022 9:36 PM
> To: dev@dpdk.org
> Cc: Akhil Goyal <gakhil@marvell.com>; Gowrishankar Muthukrishnan
> <gmuthukrishn@marvell.com>; Brian Dooley <brian.dooley@intel.com>; Fan
> Zhang <roy.fan.zhang@intel.com>
> Subject: [EXT] [RFC] examples/fips_validation: add fips 140-3 acvp aes-cbc
> test
> 
> External Email
> 
> ----------------------------------------------------------------------
> This RFC patch showcases an alternative approach to enable FIPS 140-3
> support in the DPDK fips_validation sample application.
> 
> The approach presented here takes advantage of an existing open source
> library, libacvp,(https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__github.com_cisco_libacvp&d=DwIDAg&c=nKjWec2b6R0mOyPaz7xtfQ&r
> =EAtr-g7yUFhtOio8r2Rtm13Aqe4WVp_S_gHpcu6KFVo&m=Or6tpEsPX87-
> j0caOkxdpMd9nEJRSlAb4JusQjipu1g09R4MIPOTbu_XGke63me_&s=ZnDDms
> 0KubRYj4TCliLrDQ9IO5S27LVUJunrx3vbpUA&e= ) to generate the parsed test
> cases instead of creating and maintaining a DPDK FIPS test file parser. In
> addition call back functions are added to the dpdk-fips_validation application
> to process the parsed test vectors and write back the results.
> 

libacvp carries its source code under Apache License v2. Would there be
any license conflict to consume it as parser for fips_validation in DPDK ?
https://github.com/cisco/libacvp/blob/main/LICENSE

> This initial RFC patch contains the code to parse the FIPS 140-3 test files with
> libacvp library, and the AES-CBC test runner callback function implementation
> with most test types covered apart from MCT test.
> 
> Although a different approach has been proposed
> (https://urldefense.proofpoint.com/v2/url?u=http-
> 3A__patches.dpdk.org_project_dpdk_list_-3Fseries-
> 3D22738&d=DwIDAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=EAtr-
> g7yUFhtOio8r2Rtm13Aqe4WVp_S_gHpcu6KFVo&m=Or6tpEsPX87-
> j0caOkxdpMd9nEJRSlAb4JusQjipu1g09R4MIPOTbu_XGke63me_&s=OlrgMzf
> QefOQwOunfPxgtFcKOid-8-mw1yQVv0_Ncv8&e= ), it has the disadvantage
> of having to maintain the parsing code.
> 

We already have stabilized code and good number of parsers from Brandon series 
(plus the one I am also topping up - AES_CBC, SHA etc), we can always extend the parsers 
availability as and when needed. Extension also would not be too difficult IMHO as its 
base functions are ready for all sort of extensions (where the real work could only be to
manipulate with key words for test vectors).

I am also thinking that, NIST will only make changes in a pace that its eco system can also
follow. So, we can certainly catch up in our releases.

> For the sake of this RFC here is some information on how to modify and run a
> test case.
> 
> The libacvp library can be installed by following the Building instructions in the
> README of the libacvp repo as linked above.
> 

Also libacvp is not available in any of OS distributions (https://pkgs.org/search/?q=acvp).
Though README is absolutely helping, proper documentation in the context of
consuming it for fips_validation can help better. For eg,

      (a) What configure options required (at bare minimum) to support fips app.
      (b) Versions of libcurl and openssl that is supported by libacvp compilation (for fips app.)
      (c) cross compilation notes
      (d) source code url to download from etc.

Cross compiling with dependent libs is difficult at times when the integrity is not tested 
properly. Hence, DPDK fips_validation would require special instructions apart from 
README IMHO.

> An example test vector which we can call as our input.req:
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__github.com_cisco_libacvp_blob_main_test_json_aes_aes.json&d=DwI
> DAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=EAtr-
> g7yUFhtOio8r2Rtm13Aqe4WVp_S_gHpcu6KFVo&m=Or6tpEsPX87-
> j0caOkxdpMd9nEJRSlAb4JusQjipu1g09R4MIPOTbu_XGke63me_&s=JR1nXo6
> 9fg3FQ64fRyNYYYTs1-YU8ziuU5epo0VdRcg&e=
> 
> The header acvVersion metadata at the start needs to been updated to the
> following:
> 
>     {
>       "acvVersion": "1.0",
>       "jwt": "NA",
>       "url": "NA",
>       "isSample": false,
>       "vectorSetUrls": ["NA"]
>     }

Can we manipulate these fields within the app itself instead ?
For eg, url should be NA anyway for the scope of using this library here.

> 
> With this RFC patch applied you can then run the dpdk-fips_validation as
> follows:
> 
>     ./dpdk-fips_validation --vdev=crypto_aesni_mb -- \
>         --req-file input.req --rsp-file output.rsp --acvp
> 
> Please note the MCT tests in the input.req file will generate an incorrect
> result set as the IV update part has yet to be implemented.
> This will be addressed in next revision.
> 

Please give a check on entire results including AFT as well. I did json diff and learnt 
atleast 1500 out of 2156 tests failed for AES_CBC while using this patch
(i.e post tcId 285, many of tests failed including AFT). If it is not the case for you,
I can cross check once again if anything else going wrong, as none of tests fail
in same env and with same req file, while using native parser from Brandon.

> Signed-off-by: Brian Dooley <brian.dooley@intel.com>
> ---
>  examples/fips_validation/fips_test_acvp.c  | 183
> +++++++++++++++++++++  examples/fips_validation/fips_validation.h |  34
> ++++
>  examples/fips_validation/main.c            |  59 ++++---
>  examples/fips_validation/meson.build       |   9 +
>  4 files changed, 260 insertions(+), 25 deletions(-)  create mode 100644
> examples/fips_validation/fips_test_acvp.c
> 
> diff --git a/examples/fips_validation/fips_test_acvp.c
> b/examples/fips_validation/fips_test_acvp.c
> new file mode 100644
> index 0000000000..55f6b35a7d
> --- /dev/null
> +++ b/examples/fips_validation/fips_test_acvp.c
> @@ -0,0 +1,183 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2022 Intel Corporation
> + */
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <acvp/acvp.h>
> +#include <intel-ipsec-mb.h>

I find it not required for this patch, do we need this header for any other purpose ?

> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <ctype.h>
> +
> +#include <rte_cryptodev.h>
> +#include <rte_malloc.h>
> +
> +#include "fips_validation.h"
> +
> +#define IV_OFF (sizeof(struct rte_crypto_op) + sizeof(struct
> +rte_crypto_sym_op))
> +
> +static ACVP_RESULT logger(char *msg)
> +{
> +	printf("%s", msg);
> +	return ACVP_SUCCESS;
> +}
> +
> +static int aes_cbc_handler(ACVP_TEST_CASE *test_case) {
> +	ACVP_SYM_CIPHER_TC *tc;
> +	struct rte_crypto_sym_xform xform = { 0 };
> +	const struct rte_cryptodev_symmetric_capability *cap;
> +	struct rte_cryptodev_sym_capability_idx cap_idx;
> +	struct rte_crypto_cipher_xform *cipher_xform = &xform.cipher;
> +	struct rte_crypto_sym_op *sym = env.op->sym;
> +	struct fips_val val = {NULL, 0};
> +	uint8_t *iv = rte_crypto_op_ctod_offset(env.op, uint8_t *, IV_OFF);
> +	uint16_t n_deqd;
> +	int ret;
> +
> +	memset(&xform, 0, sizeof(xform));
> +
> +	tc = test_case->tc.symmetric;
> +
> +	xform.type = RTE_CRYPTO_SYM_XFORM_CIPHER;
> +
> +	cipher_xform->algo = RTE_CRYPTO_CIPHER_AES_CBC;
> +	cipher_xform->op = (tc->direction ==
> ACVP_SYM_CIPH_DIR_ENCRYPT) ?
> +		RTE_CRYPTO_AEAD_OP_ENCRYPT :
> +		RTE_CRYPTO_AEAD_OP_DECRYPT;
> +	cipher_xform->key.data = tc->key;
> +	/* Check support for 128 bit key*/
> +	if (tc->key_len % 8 == 0)
> +		cipher_xform->key.length = 16;
> +	else
> +		cipher_xform->key.length = tc->key_len;
> +
> +	if (cipher_xform->algo == RTE_CRYPTO_CIPHER_AES_CBC) {
> +		cipher_xform->iv.length = tc->iv_len;
> +		cipher_xform->iv.offset = IV_OFF;
> +	} else {
> +		cipher_xform->iv.length = 0;
> +		cipher_xform->iv.offset = 0;
> +	}
> +	cap_idx.algo.cipher = cipher_xform->algo;
> +	cap_idx.type = RTE_CRYPTO_SYM_XFORM_CIPHER;
> +
> +	cap = 	(env.dev_id, &cap_idx);
> +	if (!cap) {
> +		RTE_LOG(ERR, USER1, "Failed to get capability for cdev
> %u\n",
> +				env.dev_id);
> +		return -EINVAL;
> +	}
> +
> +	if (rte_cryptodev_sym_capability_check_cipher(cap,
> +			cipher_xform->key.length,
> +			cipher_xform->iv.length) != 0) {
> +		RTE_LOG(ERR, USER1, "PMD %s key length %u IV length
> %u\n",
> +				info.device_name, cipher_xform-
> >key.length,
> +				cipher_xform->iv.length);
> +		return -EPERM;
> +	}
> +
> +	env.sess = rte_cryptodev_sym_session_create(env.sess_mpool);
> +	if (!env.sess)
> +		return -ENOMEM;
> +
> +	ret = rte_cryptodev_sym_session_init(env.dev_id, env.sess,
> &xform,
> +	env.sess_priv_mpool);
> +	if (ret < 0) {
> +		RTE_LOG(ERR, PMD, "Error %i: Init session\n", ret);
> +		return ret;
> +	}
> +
> +	if (env.op)
> +		rte_crypto_op_free(env.op);
> +
> +	env.op = rte_crypto_op_alloc(env.op_pool,
> +			RTE_CRYPTO_OP_TYPE_SYMMETRIC);
> +	if (!env.op)
> +		ret = -ENOMEM;
> +
> +	memcpy(iv, tc->iv, tc->iv_len);
> +
> +	if (tc->direction == ACVP_SYM_CIPH_DIR_ENCRYPT) {
> +		val.val = tc->pt;
> +		val.len = tc->pt_len;
> +	} else { /* Decrypt */
> +		val.val = tc->ct;
> +		val.len = tc->ct_len;
> +	}
> +
> +	ret = prepare_data_mbufs(&val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (tc->direction == ACVP_SYM_CIPH_DIR_ENCRYPT) {
> +		sym->cipher.data.length = tc->pt_len;
> +	} else { /* Decrypt */
> +		sym->cipher.data.length = tc->ct_len;
> +	}
> +
> +	rte_crypto_op_attach_sym_session(env.op, env.sess);
> +
> +	sym->m_src = env.mbuf;
> +	sym->cipher.data.offset = 0;
> +
> +	if (rte_cryptodev_enqueue_burst(env.dev_id, 0, &env.op, 1) < 1) {
> +		RTE_LOG(ERR, USER1, "Error: Failed enqueue\n");
> +		return ret = -1;
> +	}
> +
> +	do {
> +		struct rte_crypto_op *deqd_op[1];
> +
> +		n_deqd = rte_cryptodev_dequeue_burst(env.dev_id, 0,
> deqd_op,
> +				1);
> +	} while (n_deqd == 0);
> +
> +	rte_cryptodev_sym_session_clear(env.dev_id, env.sess);
> +	rte_cryptodev_sym_session_free(env.sess);
> +
> +	val.val = NULL;
> +
> +	ret = get_writeback_data(&val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (tc->direction == ACVP_SYM_CIPH_DIR_ENCRYPT) {
> +		memcpy(tc->ct, val.val, val.len);
> +		tc->ct_len = val.len;
> +	} else { /* Decrypt */
> +		memcpy(tc->pt, val.val, val.len);
> +		tc->pt_len = val.len;
> +	}
> +
> +	return ret;
> +}
> +

Can we have common aes_cbc handler common for above prepare xform, session creation and run test,
writeback_data etc so that, these can be shared between native and library parsers.

Finally, If an end user of fips_validation app is reluctant to proceed with libacvp for any reason,
we will not have any parser to help him for FIPS 140-3, instead we can keep Brandon approach as a 
native fallback option and this patch can add support using external libacvp library (as an option).

Thanks,
Gowrishankar