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

[Anoob Joseph <anoobj@marvell.com>]

Hi Lukasz,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Sent: Saturday, April 4, 2020 12:06 AM
> To: Anoob Joseph <anoobj@marvell.com>; dev@dpdk.org
> Cc: Narayana Prasad Raju Athreya <pathreya@marvell.com>; Lukas Bartosik [C]
> <lbartosik@marvell.com>
> Subject: [EXT] Re: [dpdk-dev] [PATCH 01/13] librte_security: fix verification of
> parameters
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Anoob,
> 
> Thank you very much for your review.
> Please see my answers inline.
> 
> Best regards,
> Lukasz
> 
> 
> W dniu 17.03.2020 o 13:59, Anoob Joseph pisze:
> > 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.
> 
> I changed the title and will push the new on in 2nd version of the paches after I'll
> fix all other issues.
> 
> How do you add a "Fixes" tag to a patch?

[Anoob] 

Check the below link. It explains the format of the patch with fixes.
https://doc.dpdk.org/guides/contributing/patches.html#commit-messages-body
 
> 
> >
> >> 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.
> >
> Done. Will appear in 2nd version of patches.
> >> +	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
> Done. Will appear in 2nd version of patches.
> >
> >> +	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,
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__protect2.fireeye.
> > com_url-3Fk-3Dc52d8c32-2D98e14097-2Dc52c077d-2D0cc47a30d446-
> 2Dc1b9d873
> > e3e59cc4-26u-3Dhttp-3A__code.dpdk.org_dpdk_latest_source_lib_librte-5F
> > ethdev_rte-5Fethdev.h-
> 23L4372&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB
> > 8rwwviRSxyLWs2n6B-
> WYLn1v9SyTMrT5EQqh2TU&m=aTo18FDvqHQBghOAhbi7x0f6EuX7
> >
> wZHTUtsRRloZ9Bw&s=TXpv6uQZW1WwB_Av3vCaHeUaibQzA0ypUUqnPy5aQlE
> &e=
> Could you explain a bit further?
> 
> Do you propose to make checks inside #ifdef RTE_LIBRTE_SECURITY_DEBUG or
> so?

[Anoob] Yes. You will need to introduce a new config flag (RTE_LIBRTE_SECURITY_DEBUG) and based on that, the error checks can be enabled/disabled.
 
> And do you have all checks or just sess and m on mind?

[Anoob] I think we should have all checks under the config option.

> 
> The instance->ops->function checks were already there without any config
> options in all API functions.

[Anoob] Must have slipped through. Thanks for pointing it out.

> 
> >
> >> +	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
> 
> --
> 
> Lukasz Wojciechowski
> Principal Software Engineer
> 
> Samsung R&D Institute Poland
> Samsung Electronics
> Office +48 22 377 88 25
> l.wojciechow@partner.samsung.com