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

["Ananyev, Konstantin" <konstantin.ananyev@intel.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%23L43
> 72
> >
> > 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"

Really? So what we have to tell now to our customers?
"Yes, rte_security is broken and can easily crash your app.
But we might fix it in future versions... or maybe not.
For now just recompile our source with that flag enabled?"
Obviously this is not an option.
It is a bug and it is a stopper for 20.05 release.
It has to be fixed asap. 


> 
> >
> >> -----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