diff mbox series

[01/13] librte_security: fix verification of parameters

Message ID 20200312151654.7218-2-l.wojciechow@partner.samsung.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series Fixes and unit tests for librte_security |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/iol-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS

Commit Message

Lukasz Wojciechowski March 12, 2020, 3:16 p.m. UTC 
  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(-)

Comments

Anoob Joseph March 17, 2020, 12:59 p.m. UTC | #1 
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
Lukasz Wojciechowski April 3, 2020, 6:36 p.m. UTC | #2 
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?

>   
>> 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://protect2.fireeye.com/url?k=c52d8c32-98e14097-c52c077d-0cc47a30d446-c1b9d873e3e59cc4&u=http://code.dpdk.org/dpdk/latest/source/lib/librte_ethdev/rte_ethdev.h#L4372
Could you explain a bit further?

Do you propose to make checks inside #ifdef RTE_LIBRTE_SECURITY_DEBUG or so?
And do you have all checks or just sess and m on mind?

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

>   
>> +	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
Anoob Joseph April 5, 2020, 12:54 p.m. UTC | #3 
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
Lukasz Wojciechowski April 6, 2020, 6:49 p.m. UTC | #4 
Dear Anoob,

Thank you for your reply and hints.

Now I have patches ready to send as version 2, but I hesitate, because I 
don't like the idea of placing all checks in #ifdefs for 
RTE_LIBRTE_SECURITY_DEBUG config option.

Let me explain here why: The config flag as a debug one will be disabled 
by default, which means, that normally nobody will use the checks.
I believe that it is much better to have checks enabled as most of them 
will save the user of librte_security from segmentation faults, when 
trying to run instance->ops functions that are not supported or use 
invalid mempool object. I believe it will cause much less trouble to 
verify the error codes than to fight the segfault.
It is also mentioned in the API description in few places that specific 
codes are returned in case some operation is not supported. Can we make 
such a changes in API, changing the current behavior from an error 
return code to segmentation fault during execution?

That's why I would like to keep all of the checks enabled and not placed 
inside config option.

However it would be nice to add the RTE_LIBRTE_SECURITY_DEBUG flag that 
you mentioned for changing checks behavior, to additionally provide logs 
of checks. This way a devloper using libret_security won't get a 
segmentation faults but error codes. If [s]he wants to check the details 
he'll rebuild the library with debug config option enabled and will be 
able to see all the details in logs, so [s]he will be able to fix the code.

What do you think about such usage of the config debug flag?

Best regards

Lukasz


W dniu 05.04.2020 o 14:54, Anoob Joseph pisze:

> 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://protect2.fireeye.com/url?k=13e80549-4e769ea3-13e98e06-0cc47a6cba04-78f48282e990b416&q=1&u=https%3A%2F%2Fdoc.dpdk.org%2Fguides%2Fcontributing%2Fpatches.html%23commit-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://protect2.fireeye.com/url?k=eee8020a-b37699e0-eee98945-0cc47a6cba04-561954f3424eceea&q=1&u=https%3A%2F%2Furldefense.proofpoint.com%2Fv2%2Furl%3Fu%3Dhttps-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
Anoob Joseph April 7, 2020, 6:20 a.m. UTC | #5 
Hi Lukasz,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
> Sent: Tuesday, April 7, 2020 12:19 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: Re: [EXT] Re: [dpdk-dev] [PATCH 01/13] librte_security: fix verification
> of parameters
> 
> Dear Anoob,
> 
> Thank you for your reply and hints.

[Anoob] No mention!
 
> 
> Now I have patches ready to send as version 2, but I hesitate, because I don't
> like the idea of placing all checks in #ifdefs for RTE_LIBRTE_SECURITY_DEBUG
> config option.
> 
> Let me explain here why: The config flag as a debug one will be disabled by
> default, which means, that normally nobody will use the checks.
> I believe that it is much better to have checks enabled as most of them will save
> the user of librte_security from segmentation faults, when trying to run
> instance->ops functions that are not supported or use invalid mempool object. I
> believe it will cause much less trouble to verify the error codes than to fight the
> segfault.

[Anoob] No disagreement. In fact this is exactly what is done with control path APIs. But for datapath APIs, the penalty of such checks is not trivial. I believe that's the argument for having the DEBUG config option in ethdev data path ops. Also, for cases like instance->ops being NULL, the user would get a seg fault with the first call itself and he can enable the config option to debug the segfault he is observing.

As for invalid mempool object, I do agree that is one case where we are better off with per packet checks, but still perf impact would be there. In ethdev, the library doesn't do these checks and it would be upto the driver to have these checks if required. Same is the case with rte_cryptodev_enqueue_burst().
 
> It is also mentioned in the API description in few places that specific codes are
> returned in case some operation is not supported. Can we make such a changes
> in API, changing the current behavior from an error return code to segmentation
> fault during execution?

[Anoob] Did you mean, if we have to allow seg fault as a valid error case, better document it and remove the error codes getting returned? Again, I'm not sure whether we can document that seg fault is the error case. Atleast, in ethdev improper conditions can lead to seg fault but is not documented. May be, maintainers should comment on it.
 
> 
> That's why I would like to keep all of the checks enabled and not placed inside
> config option.
> 
> However it would be nice to add the RTE_LIBRTE_SECURITY_DEBUG flag that
> you mentioned for changing checks behavior, to additionally provide logs of
> checks. This way a devloper using libret_security won't get a segmentation
> faults but error codes. If [s]he wants to check the details he'll rebuild the library
> with debug config option enabled and will be able to see all the details in logs,
> so [s]he will be able to fix the code.
> 
> What do you think about such usage of the config debug flag?

[Anoob] I totally agree to your suggestions on preventing seg faults. But my only concern is the additional check and the corresponding perf penalty.

May be let's look at the APIs and discuss what need to be handled how,

int
rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
			      struct rte_security_session *sess,
			      struct rte_mbuf *mb, void *params);

instance -> NULL checks under config option
sess -> NULL checks under config option
mb -> possibly a datapath check. But can leave it to driver as well. (rte_cryptodev_enqueue_burst() also doesn't do these checks).
Params -> can be NULL (so no check required).

__rte_experimental
void *
rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md);

instance -> NULL checks under config option
md -> can be NULL (device specific values).

Does this make sense to you?
 
> 
> Best regards
> 
> Lukasz
> 
> 
> W dniu 05.04.2020 o 14:54, Anoob Joseph pisze:
> 
> > 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://urldefense.proofpoint.com/v2/url?u=https-3A__protect2.fireeye.
> > com_url-3Fk-3D13e80549-2D4e769ea3-2D13e98e06-2D0cc47a6cba04-
> 2D78f48282
> > e990b416-26q-3D1-26u-3Dhttps-253A-252F-252Fdoc.dpdk.org-252Fguides-
> 252
> > Fcontributing-252Fpatches.html-2523commit-2Dmessages-
> 2Dbody&d=DwIDaQ&c
> > =nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
> WYLn1v9SyTMrT5EQqh2TU&
> > m=fTGxo5Mh9jPA0-xv8XSAtZpHD9TZebCJWW9PGcElYmA&s=g2HlI0z81E0M5-
> txF2U5Ag
> > xEg0l3MHs4JT0O8AiHvm8&e=
> >
> >>>> 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-3Deee8020a-2Db37699e0-2Deee98945-
> 2D0cc47a6cba04-2D561954f3424eceea-26q-3D1-26u-3Dhttps-253A-252F-
> 252Furldefense.proofpoint.com-252Fv2-252Furl-253Fu-253Dhttps-2D3A-5F-
> 5Fprotect2.fireeye&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRS
> xyLWs2n6B-WYLn1v9SyTMrT5EQqh2TU&m=fTGxo5Mh9jPA0-
> xv8XSAtZpHD9TZebCJWW9PGcElYmA&s=vENm9fIBE5DENCAaeYTeYFwK__g06Jv
> K-lyOiJh-VL0&e= .
> >>> 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
> 
> --
> 
> Lukasz Wojciechowski
> Principal Software Engineer
> 
> Samsung R&D Institute Poland
> Samsung Electronics
> Office +48 22 377 88 25
> l.wojciechow@partner.samsung.com
Lukasz Wojciechowski April 8, 2020, 3:25 a.m. UTC | #6 
Hi,

I pushed the 2nd version of patches. I hope I understood all your 
comments well.

Many thanks,
Lukasz


W dniu 07.04.2020 o 08:20, Anoob Joseph pisze:
> Hi Lukasz,
>
> Please see inline.
>
> Thanks,
> Anoob
>
>> -----Original Message-----
>> From: Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>
>> Sent: Tuesday, April 7, 2020 12:19 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: Re: [EXT] Re: [dpdk-dev] [PATCH 01/13] librte_security: fix verification
>> of parameters
>>
>> Dear Anoob,
>>
>> Thank you for your reply and hints.
> [Anoob] No mention!
>   
>> Now I have patches ready to send as version 2, but I hesitate, because I don't
>> like the idea of placing all checks in #ifdefs for RTE_LIBRTE_SECURITY_DEBUG
>> config option.
>>
>> Let me explain here why: The config flag as a debug one will be disabled by
>> default, which means, that normally nobody will use the checks.
>> I believe that it is much better to have checks enabled as most of them will save
>> the user of librte_security from segmentation faults, when trying to run
>> instance->ops functions that are not supported or use invalid mempool object. I
>> believe it will cause much less trouble to verify the error codes than to fight the
>> segfault.
> [Anoob] No disagreement. In fact this is exactly what is done with control path APIs. But for datapath APIs, the penalty of such checks is not trivial. I believe that's the argument for having the DEBUG config option in ethdev data path ops. Also, for cases like instance->ops being NULL, the user would get a seg fault with the first call itself and he can enable the config option to debug the segfault he is observing.
>
> As for invalid mempool object, I do agree that is one case where we are better off with per packet checks, but still perf impact would be there. In ethdev, the library doesn't do these checks and it would be upto the driver to have these checks if required. Same is the case with rte_cryptodev_enqueue_burst().
>   
>> It is also mentioned in the API description in few places that specific codes are
>> returned in case some operation is not supported. Can we make such a changes
>> in API, changing the current behavior from an error return code to segmentation
>> fault during execution?
> [Anoob] Did you mean, if we have to allow seg fault as a valid error case, better document it and remove the error codes getting returned? Again, I'm not sure whether we can document that seg fault is the error case. Atleast, in ethdev improper conditions can lead to seg fault but is not documented. May be, maintainers should comment on it.
>   
>> That's why I would like to keep all of the checks enabled and not placed inside
>> config option.
>>
>> However it would be nice to add the RTE_LIBRTE_SECURITY_DEBUG flag that
>> you mentioned for changing checks behavior, to additionally provide logs of
>> checks. This way a devloper using libret_security won't get a segmentation
>> faults but error codes. If [s]he wants to check the details he'll rebuild the library
>> with debug config option enabled and will be able to see all the details in logs,
>> so [s]he will be able to fix the code.
>>
>> What do you think about such usage of the config debug flag?
> [Anoob] I totally agree to your suggestions on preventing seg faults. But my only concern is the additional check and the corresponding perf penalty.
>
> May be let's look at the APIs and discuss what need to be handled how,
>
> int
> rte_security_set_pkt_metadata(struct rte_security_ctx *instance,
> 			      struct rte_security_session *sess,
> 			      struct rte_mbuf *mb, void *params);
>
> instance -> NULL checks under config option
> sess -> NULL checks under config option
> mb -> possibly a datapath check. But can leave it to driver as well. (rte_cryptodev_enqueue_burst() also doesn't do these checks).
> Params -> can be NULL (so no check required).
>
> __rte_experimental
> void *
> rte_security_get_userdata(struct rte_security_ctx *instance, uint64_t md);
>
> instance -> NULL checks under config option
> md -> can be NULL (device specific values).
>
> Does this make sense to you?
>   
>> Best regards
>>
>> Lukasz
>>
>>
>> W dniu 05.04.2020 o 14:54, Anoob Joseph pisze:
>>
>>> 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://protect2.fireeye.com/url?k=457094cb-18a428a3-45711f84-0cc47a3356b2-a0d1cb1b92aa19e8&u=https://urldefense.proofpoint.com/v2/url?u=https-3A__protect2.fireeye.
>>> com_url-3Fk-3D13e80549-2D4e769ea3-2D13e98e06-2D0cc47a6cba04-
>> 2D78f48282
>>> e990b416-26q-3D1-26u-3Dhttps-253A-252F-252Fdoc.dpdk.org-252Fguides-
>> 252
>>> Fcontributing-252Fpatches.html-2523commit-2Dmessages-
>> 2Dbody&d=DwIDaQ&c
>>> =nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRSxyLWs2n6B-
>> WYLn1v9SyTMrT5EQqh2TU&
>>> m=fTGxo5Mh9jPA0-xv8XSAtZpHD9TZebCJWW9PGcElYmA&s=g2HlI0z81E0M5-
>> txF2U5Ag
>>> xEg0l3MHs4JT0O8AiHvm8&e=
>>>
>>>>>> 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-3Deee8020a-2Db37699e0-2Deee98945-
>> 2D0cc47a6cba04-2D561954f3424eceea-26q-3D1-26u-3Dhttps-253A-252F-
>> 252Furldefense.proofpoint.com-252Fv2-252Furl-253Fu-253Dhttps-2D3A-5F-
>> 5Fprotect2.fireeye&d=DwIDaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=jPfB8rwwviRS
>> xyLWs2n6B-WYLn1v9SyTMrT5EQqh2TU&m=fTGxo5Mh9jPA0-
>> xv8XSAtZpHD9TZebCJWW9PGcElYmA&s=vENm9fIBE5DENCAaeYTeYFwK__g06Jv
>> K-lyOiJh-VL0&e= .
>>>>> 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
>> --
>>
>> Lukasz Wojciechowski
>> Principal Software Engineer
>>
>> Samsung R&D Institute Poland
>> Samsung Electronics
>> Office +48 22 377 88 25
>> l.wojciechow@partner.samsung.com
diff mbox series

Patch

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);
+	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.
+	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);
+	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 ==