Re: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented ops

[Lukasz Wojciechowski <l.wojciechow@partner.samsung.com>]

W dniu 23.04.2020 o 06:07, Anoob Joseph pisze:
> Hi Konstantin,
>
> These are data path ops and so it will be better if we can avoid such checks in the datapath. The same is done in ethdev also.
>
> https://protect2.fireeye.com/url?k=d44931cf-89d2cdac-d448ba80-0cc47a31cdbc-8281a62b4c91d848&q=1&u=http%3A%2F%2Fcode.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Flib%2Flibrte_ethdev%2Frte_ethdev.h%23L4372
>
> Datapath functions in cryptodev (enqueue/dequeue) doesn't even have such checks.
> https://protect2.fireeye.com/url?k=51324200-0ca9be63-5133c94f-0cc47a31cdbc-11f88758fc12c996&q=1&u=http%3A%2F%2Fcode.dpdk.org%2Fdpdk%2Fv20.02%2Fsource%2Flib%2Flibrte_cryptodev%2Frte_cryptodev.h%23L962
>
>
> Thanks,
> Anoob

Hi Konstantine,

It's my fault. Sorry.

These checks need to be disabled in non-debug code, so they should be 
wrapped in a macro. It's just not the valid macro.
The discussion about rte_debug mode is ongoing 
(https://patchwork.dpdk.org/patch/68815/)
and currently the v2 version of patches is prepared to gather 
maintainers opinion.

After the rte_debug is introduced the proper macro to use will be 
RTE_DEBUG_SECURITY.

Until then, the RTE_DEBUG macro can stay as like Anoob mentioned the 
checks will have impact on dataplane performance.

If you want to enable this code, please use CFLAGS="-DRTE_DEBUG"

>
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Konstantin Ananyev
>> Sent: Thursday, April 23, 2020 5:22 AM
>> To: dev@dpdk.org
>> Cc: akhil.goyal@nxp.com; declan.doherty@intel.com; Konstantin Ananyev
>> <konstantin.ananyev@intel.com>
>> Subject: [dpdk-dev] [PATCH] security: fix crash at accessing non-implemented
>> ops
>>
>> Valid checks for optional function pointers inside dev-ops were disabled by
>> undefined macro.
>>
>> Fixes: b6ee98547847 ("security: fix verification of parameters")
>>
>> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>> ---
>>   lib/librte_security/rte_security.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/lib/librte_security/rte_security.c b/lib/librte_security/rte_security.c
>> index d475b0977..b65430ce2 100644
>> --- a/lib/librte_security/rte_security.c
>> +++ b/lib/librte_security/rte_security.c
>> @@ -107,11 +107,9 @@ rte_security_set_pkt_metadata(struct rte_security_ctx
>> *instance,
>>   			      struct rte_security_session *sess,
>>   			      struct rte_mbuf *m, void *params)  { -#ifdef
>> RTE_DEBUG
>>   	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, set_pkt_metadata, -
>> EINVAL,
>>   			-ENOTSUP);
>>   	RTE_PTR_OR_ERR_RET(sess, -EINVAL);
>> -#endif
>>   	return instance->ops->set_pkt_metadata(instance->device,
>>   					       sess, m, params);
>>   }
>> @@ -121,9 +119,7 @@ rte_security_get_userdata(struct rte_security_ctx
>> *instance, uint64_t md)  {
>>   	void *userdata = NULL;
>>
>> -#ifdef RTE_DEBUG
>>   	RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, get_userdata, NULL,
>> NULL); -#endif
>>   	if (instance->ops->get_userdata(instance->device, md, &userdata))
>>   		return NULL;
>>
>> --
>> 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