Re: [dpdk-dev] [PATCH 01/13] librte_security: fix verification of parameters

[Anoob Joseph <anoobj@marvell.com>]

Hi Lukasz,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Lukasz Wojciechowski
> Sent: Thursday, March 12, 2020 8:47 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 01/13] librte_security: fix verification of parameters

[Anoob] I believe the title has to be: "security: fix verification of parameters"

Also, you can add "Fixes" as well.
 
> 
> This patch adds verification of the parameters to the ret_security API functions.
> All required parameters are checked if they are not NULL.
> 
> Checks verify full chain of pointers, e.g. in case of verification of "instance->ops-
> >session_XXX", they check also "instance" and "instance->ops".
> 
> Signed-off-by: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Change-Id: I1724c926a1a0a13fd16d76f19842a0b40fbea1b2
> ---
>  lib/librte_security/rte_security.c | 58 +++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
> index bc81ce15d..40a0e9ce5 100644
> --- a/lib/librte_security/rte_security.c
> +++ b/lib/librte_security/rte_security.c
> @@ -1,6 +1,7 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
>   * Copyright 2017 NXP.
>   * Copyright(c) 2017 Intel Corporation.
> + * Copyright (c) 2020 Samsung Electronics Co., Ltd All Rights Reserved
>   */
> 
>  #include <rte_malloc.h>
> @@ -9,6 +10,12 @@
>  #include "rte_security.h"
>  #include "rte_security_driver.h"
> 
> +/* Macro to check for invalid pointers */
> +#define RTE_PTR_OR_ERR_RET(ptr, retval) do {	\
> +	if ((ptr) == NULL)			\
> +		return retval;			\
> +} while (0)
> +
>  struct rte_security_session *
>  rte_security_session_create(struct rte_security_ctx *instance,
>  			    struct rte_security_session_conf *conf, @@ -16,10
> +23,11 @@ rte_security_session_create(struct rte_security_ctx *instance,  {
>  	struct rte_security_session *sess = NULL;
> 
> -	if (conf == NULL)
> -		return NULL;
> -
> -	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_create, NULL);
> +	RTE_PTR_OR_ERR_RET(instance, NULL);
> +	RTE_PTR_OR_ERR_RET(instance->ops, NULL);
> +	RTE_PTR_OR_ERR_RET(instance->ops->session_create, NULL);

[Anoob] The above three lines are repeated for every op NULL check. Can we introduce one macro for doing all the three checks? In case if it doesn't come off well, we can stick to individual checks.
 
> +	RTE_PTR_OR_ERR_RET(conf, NULL);
> +	RTE_PTR_OR_ERR_RET(mp, NULL);
> 
>  	if (rte_mempool_get(mp, (void **)&sess))
>  		return NULL;
> @@ -38,14 +46,20 @@ rte_security_session_update(struct rte_security_ctx
> *instance,
>  			    struct rte_security_session *sess,
>  			    struct rte_security_session_conf *conf)  {
> -	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_update, -
> ENOTSUP);
> +	RTE_PTR_OR_ERR_RET(instance, -EINVAL);
> +	RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL);
> +	RTE_PTR_OR_ERR_RET(instance->ops->session_update, -ENOTSUP);
> +	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
> +	RTE_PTR_OR_ERR_RET(conf, -EINVAL);
>  	return instance->ops->session_update(instance->device, sess, conf);  }
> 
>  unsigned int
>  rte_security_session_get_size(struct rte_security_ctx *instance)  {
> -	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_get_size, 0);
> +	RTE_PTR_OR_ERR_RET(instance, 0);
> +	RTE_PTR_OR_ERR_RET(instance->ops, 0);
> +	RTE_PTR_OR_ERR_RET(instance->ops->session_get_size, 0);
>  	return instance->ops->session_get_size(instance->device);
>  }
> 
> @@ -54,7 +68,11 @@ rte_security_session_stats_get(struct rte_security_ctx
> *instance,
>  			       struct rte_security_session *sess,
>  			       struct rte_security_stats *stats)  {
> -	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_stats_get, -
> ENOTSUP);
> +	RTE_PTR_OR_ERR_RET(instance, -EINVAL);
> +	RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL);
> +	RTE_PTR_OR_ERR_RET(instance->ops->session_stats_get, -ENOTSUP);
> +	// Parameter sess can be NULL in case of getting global statistics.

[Anoob] Checkpatch error.
ERROR:C99_COMMENTS: do not use C99 // comments

> +	RTE_PTR_OR_ERR_RET(stats, -EINVAL);
>  	return instance->ops->session_stats_get(instance->device, sess, stats);
> }
> 
> @@ -64,7 +82,10 @@ rte_security_session_destroy(struct rte_security_ctx
> *instance,  {
>  	int ret;
> 
> -	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->session_destroy, -
> ENOTSUP);
> +	RTE_PTR_OR_ERR_RET(instance, -EINVAL);
> +	RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL);
> +	RTE_PTR_OR_ERR_RET(instance->ops->session_destroy, -ENOTSUP);
> +	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
> 
>  	if (instance->sess_cnt)
>  		instance->sess_cnt--;
> @@ -81,7 +102,11 @@ rte_security_set_pkt_metadata(struct rte_security_ctx
> *instance,
>  			      struct rte_security_session *sess,
>  			      struct rte_mbuf *m, void *params)  {
> -	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->set_pkt_metadata, -
> ENOTSUP);
> +	RTE_PTR_OR_ERR_RET(instance, -EINVAL);
> +	RTE_PTR_OR_ERR_RET(instance->ops, -EINVAL);
> +	RTE_PTR_OR_ERR_RET(instance->ops->set_pkt_metadata, -ENOTSUP);

[Anoob] set_pkt_metadata() and get_userdata() are datapath ops. So can you introduce a config option to enable/disable the checks.

Please check,
http://code.dpdk.org/dpdk/latest/source/lib/librte_ethdev/rte_ethdev.h#L4372
 
> +	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
> +	RTE_PTR_OR_ERR_RET(m, -EINVAL);
>  	return instance->ops->set_pkt_metadata(instance->device,
>  					       sess, m, params);
>  }
> @@ -91,7 +116,9 @@ rte_security_get_userdata(struct rte_security_ctx
> *instance, uint64_t md)  {
>  	void *userdata = NULL;
> 
> -	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->get_userdata, NULL);
> +	RTE_PTR_OR_ERR_RET(instance, NULL);
> +	RTE_PTR_OR_ERR_RET(instance->ops, NULL);
> +	RTE_PTR_OR_ERR_RET(instance->ops->get_userdata, NULL);
>  	if (instance->ops->get_userdata(instance->device, md, &userdata))
>  		return NULL;
> 
> @@ -101,7 +128,9 @@ rte_security_get_userdata(struct rte_security_ctx
> *instance, uint64_t md)  const struct rte_security_capability *
> rte_security_capabilities_get(struct rte_security_ctx *instance)  {
> -	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
> +	RTE_PTR_OR_ERR_RET(instance, NULL);
> +	RTE_PTR_OR_ERR_RET(instance->ops, NULL);
> +	RTE_PTR_OR_ERR_RET(instance->ops->capabilities_get, NULL);
>  	return instance->ops->capabilities_get(instance->device);
>  }
> 
> @@ -113,7 +142,10 @@ rte_security_capability_get(struct rte_security_ctx
> *instance,
>  	const struct rte_security_capability *capability;
>  	uint16_t i = 0;
> 
> -	RTE_FUNC_PTR_OR_ERR_RET(*instance->ops->capabilities_get, NULL);
> +	RTE_PTR_OR_ERR_RET(instance, NULL);
> +	RTE_PTR_OR_ERR_RET(instance->ops, NULL);
> +	RTE_PTR_OR_ERR_RET(instance->ops->capabilities_get, NULL);
> +	RTE_PTR_OR_ERR_RET(idx, NULL);
>  	capabilities = instance->ops->capabilities_get(instance->device);
> 
>  	if (capabilities == NULL)
> @@ -121,7 +153,7 @@ rte_security_capability_get(struct rte_security_ctx
> *instance,
> 
>  	while ((capability = &capabilities[i++])->action
>  			!= RTE_SECURITY_ACTION_TYPE_NONE) {
> -		if (capability->action  == idx->action &&
> +		if (capability->action == idx->action &&
>  				capability->protocol == idx->protocol) {
>  			if (idx->protocol == RTE_SECURITY_PROTOCOL_IPSEC)
> {
>  				if (capability->ipsec.proto ==
> --
> 2.17.1