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

Lukasz Wojciechowski l.wojciechow at partner.samsung.com
Fri Apr 3 20:36:11 CEST 2020
Previous message: [dpdk-dev] [PATCH v2 0/4] introduce multi-function processing support
Next message: [dpdk-dev] [EXT] Re: [PATCH 01/13] librte_security: fix verification of parameters
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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 at dpdk.org> On Behalf Of Lukasz Wojciechowski
>> Sent: Thursday, March 12, 2020 8:47 PM
>> To: dev at 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 at 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

-- 

Lukasz Wojciechowski
Principal Software Engineer

Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciechow at partner.samsung.com
Previous message: [dpdk-dev] [PATCH v2 0/4] introduce multi-function processing support
Next message: [dpdk-dev] [EXT] Re: [PATCH 01/13] librte_security: fix verification of parameters
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the dev mailing list