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

["Trahe, Fiona" <fiona.trahe@intel.com>]

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

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


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

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