> -----Original Message-----
> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Anoob Joseph
> Sent: Wednesday, February 20, 2019 3:42 PM
> To: Akhil Goyal <akhil.go...@nxp.com>; Doherty, Declan
> <declan.dohe...@intel.com>; De Lara Guarch,
> Pablo <pablo.de.lara.gua...@intel.com>
> Cc: Jerin Jacob Kollanukkaran <jer...@marvell.com>; Narayana Prasad Raju
> Athreya
> <pathr...@marvell.com>; dev@dpdk.org; Ankur Dwivedi <adwiv...@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.go...@nxp.com>; Declan Doherty
> > <declan.dohe...@intel.com>; Pablo de Lara <pablo.de.lara.gua...@intel.com>
> > Cc: Anoob Joseph <ano...@marvell.com>; Jerin Jacob Kollanukkaran
> > <jer...@marvell.com>; Narayana Prasad Raju Athreya
> > <pathr...@marvell.com>; dev@dpdk.org; Ankur Dwivedi
> > <adwiv...@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 <adwiv...@marvell.com>
> > Signed-off-by: Anoob Joseph <ano...@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
