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

[Anoob Joseph <anoobj@marvell.com>]

Hi Fiona,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Trahe, Fiona <fiona.trahe@intel.com>
> Sent: Thursday, February 21, 2019 10:33 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
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Anoob Joseph
> > Sent: Wednesday, February 20, 2019 3:42 PM
> > To: 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>
> > Subject: Re: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name
> > comparison
> >
> > Hi Akhil, Declan, Pablo,
> >
> > Can you review this patch and share your thoughts?
> >
> > Thanks,
> > Anoob
> >
> > > -----Original Message-----
> > > From: Anoob Joseph
> > > Sent: Monday, February 4, 2019 4:56 PM
> > > To: Akhil Goyal <akhil.goyal@nxp.com>; Declan Doherty
> > > <declan.doherty@intel.com>; Pablo de Lara
> > > <pablo.de.lara.guarch@intel.com>
> > > Cc: Anoob Joseph <anoobj@marvell.com>; Jerin Jacob Kollanukkaran
> > > <jerinj@marvell.com>; Narayana Prasad Raju Athreya
> > > <pathreya@marvell.com>; dev@dpdk.org; Ankur Dwivedi
> > > <adwivedi@marvell.com>
> > > Subject: [PATCH] lib/cryptodev: fix driver name comparison
> > >
> > > The string compare to the length of driver name might give false
> > > positives when there are drivers with similar names (one being the
> subset of another).
> > >
> > > Following is such a naming which could result in false positive.
> > > 1. crypto_driver
> > > 2. crypto_driver1
> [Fiona] This patch changes compare for both driver and device names.
> Update description to mention device names too.

[Anoob] Will update that.
 
> 
> > > When strncmp with len = strlen("crypto_driver") is done, it could
> > > give a false positive when compared against "crypto_driver1".
> > >
> > > Fixes: d11b0f30df88 ("cryptodev: introduce API and framework for
> > > crypto
> > > devices")
> > >
> > > Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> > > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > > ---
> > >  lib/librte_cryptodev/rte_cryptodev.c | 11 ++++++-----
> > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/lib/librte_cryptodev/rte_cryptodev.c
> > > b/lib/librte_cryptodev/rte_cryptodev.c
> > > index 7009735..b743c60 100644
> > > --- a/lib/librte_cryptodev/rte_cryptodev.c
> > > +++ b/lib/librte_cryptodev/rte_cryptodev.c
> > > @@ -510,7 +510,8 @@ rte_cryptodev_pmd_get_named_dev(const char
> *name)
> > >  		dev = &cryptodev_globals.devs[i];
> > >
> > >  		if ((dev->attached == RTE_CRYPTODEV_ATTACHED) &&
> > > -				(strcmp(dev->data->name, name) == 0))
> > > +				(strncmp(dev->data->name, name,
> > > +					 RTE_CRYPTODEV_NAME_MAX_LEN)
> > > == 0))
> > >  			return dev;
> > >  	}
> > >
> > > @@ -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.

> 
> > >  				(cryptodev_globals.devs[i].attached ==
> > >
> 	RTE_CRYPTODEV_ATTACHED))
> > >  			return i;
> > > @@ -586,7 +587,7 @@ rte_cryptodev_devices_get(const char
> > > *driver_name, uint8_t *devices,
> > >
> > >  			cmp = strncmp(devs[i].device->driver->name,
> > >  					driver_name,
> > > -					strlen(driver_name));
> > > +					RTE_CRYPTODEV_NAME_MAX_LEN);
> [Fiona] Is this safe? Same comment as for device name.
> Also assumes RTE_CRYPTODEV_NAME_MAX_LEN is same as
> RTE_DEV_NAME_MAX_LEN. They're both 64 at the moment so not currently
> an issue.
> 

[Anoob] Will fix this with v2.

> > >
> > >  			if (cmp == 0)
> > >  				devices[count++] = devs[i].data->dev_id;
> @@ -
> > > 1691,7 +1692,7 @@ rte_cryptodev_driver_id_get(const char *name)
> > >
> > >  	TAILQ_FOREACH(driver, &cryptodev_driver_list, next) {
> > >  		driver_name = driver->driver->name;
> > > -		if (strncmp(driver_name, name, strlen(driver_name)) == 0)
> > > +		if (strncmp(driver_name, name,
> > > RTE_CRYPTODEV_NAME_MAX_LEN) == 0)
> > >  			return driver->id;
> > >  	}
> > >  	return -1;
> > > --
> > > 2.7.4