Re: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name comparison

[Anoob Joseph <anoobj@marvell.com>]

Hi Akhil, Declan, Pablo,

Can you review this patch and share your thoughts?

Thanks,
Anoob

> -----Original Message-----
> From: Trahe, Fiona <fiona.trahe@intel.com>
> Sent: Monday, February 25, 2019 5:22 PM
> To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal
> <akhil.goyal@nxp.com>; Doherty, Declan <declan.doherty@intel.com>; De
> Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Narayana Prasad Raju
> Athreya <pathreya@marvell.com>; dev@dpdk.org; Ankur Dwivedi
> <adwivedi@marvell.com>
> Subject: RE: [PATCH] lib/cryptodev: fix driver name comparison
> 
> Hi Anoob
> 
> > -----Original Message-----
> > From: Anoob Joseph [mailto:anoobj@marvell.com]
> > Sent: Saturday, February 23, 2019 6:12 AM
> > To: Trahe, Fiona <fiona.trahe@intel.com>; Akhil Goyal
> > <akhil.goyal@nxp.com>; Doherty, Declan <declan.doherty@intel.com>; De
> > Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>; Yigit, Ferruh
> > <ferruh.yigit@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> > Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Narayana Prasad
> > Raju Athreya <pathreya@marvell.com>; dev@dpdk.org; Ankur Dwivedi
> > <adwivedi@marvell.com>
> > Subject: RE: [PATCH] lib/cryptodev: fix driver name comparison
> >
> > Hi Fiona,
> >
> > > -----Original Message-----
> > > From: Trahe, Fiona <fiona.trahe@intel.com>
> > > Sent: Friday, February 22, 2019 9:09 PM
> > > To: Anoob Joseph <anoobj@marvell.com>; Akhil Goyal
> > > <akhil.goyal@nxp.com>; Doherty, Declan <declan.doherty@intel.com>;
> > > De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> > > Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Narayana Prasad
> > > Raju Athreya <pathreya@marvell.com>; dev@dpdk.org; Ankur Dwivedi
> > > <adwivedi@marvell.com>; Trahe, Fiona <fiona.trahe@intel.com>
> > > Subject: RE: [PATCH] lib/cryptodev: fix driver name comparison
> > >
> > > Hi Anoob,
> > >
> > > > > > > @@ -542,8 +543,8 @@ rte_cryptodev_get_dev_id(const char
> *name)
> > > > > > >  		return -1;
> > > > > > >
> > > > > > >  	for (i = 0; i < cryptodev_globals.nb_devs; i++)
> > > > > > > -		if ((strcmp(cryptodev_globals.devs[i].data->name,
> name)
> > > > > > > -				== 0) &&
> > > > > > > +		if ((strncmp(cryptodev_globals.devs[i].data->name,
> > > name,
> > > > > > > +				RTE_CRYPTODEV_NAME_MAX_LEN)
> > > == 0)
> > > > > &&
> > > > > [Fiona] Is this safe? The const passed to this may not be the
> > > > > full length of RTE_CRYPTODEV_NAME_MAX_LEN. Does this
> prototype
> > > > > need to specify that a full length const filled with trailing
> > > > > zeros must be passed in? And if so is this an ABI breakage?
> > > > >
> > > >
> > > > [Anoob] strcmp itself is not safe when we have buffers which are
> > > > not NULL terminated. Strncmp will make sure the check won't exceed
> > > RTE_CRYPTODEV_NAME_MAX_LEN.
> > > >
> > > > From man page, "The strncmp() function is similar, except it only
> > > > compares the first (at most) n bytes of
> > > > s1 and s2."
> > > >
> > > > The main issue here is the usage of strncmp with
> > > > strlen(driver_name), as in the below cases. Strlen will return
> > > > string length, which doesn't include \0. strcmp is good enough to
> > > > fix the issue. But usage of strcmp would assume that the const is
> > > > filled with trailing zero. IMO, none of
> > > these options are really safe. So please advise on what would be the
> > > best solution here. I'll revise the patch accordingly.
> > > [Fiona] I agree and think it is safest as you've coded it. However
> > > I'd suggest adding a comment on the relevant APIs saying that the
> > > string must be passed in in a buffer of size <use relevant #define> with
> trailing zeros.
> >
> > [Anoob] Do you want this patch to address that? And wouldn't
> > specifying something like that explicitly, be an ABI breakage?
> [Fiona] Yes, I think it should be in this patch as this patch is causing it.
> But it's up to the maintainers what's acceptable - it seems to me that it's an
> ABI breakage, avoiding saying it explicitly doesn't make it less so.
> 
> >
> > Also, I think the same is applicable for other similar functions
> > (rte_eth_dev_get_port_by_name() etc), wherever we expect a string.
> Please do share your thoughts on what all I should include in this patch.
> >
> > Thanks,
> > Anoob