Hi Fiona, Please see inline.
Thanks, Anoob > -----Original Message----- > From: Trahe, Fiona <[email protected]> > Sent: Thursday, February 21, 2019 10:33 PM > To: Anoob Joseph <[email protected]>; Akhil Goyal > <[email protected]>; Doherty, Declan <[email protected]>; De > Lara Guarch, Pablo <[email protected]> > Cc: Jerin Jacob Kollanukkaran <[email protected]>; Narayana Prasad Raju > Athreya <[email protected]>; [email protected]; Ankur Dwivedi > <[email protected]>; Trahe, Fiona <[email protected]> > Subject: RE: [PATCH] lib/cryptodev: fix driver name comparison > > > > > -----Original Message----- > > From: dev [mailto:[email protected]] On Behalf Of Anoob Joseph > > Sent: Wednesday, February 20, 2019 3:42 PM > > To: Akhil Goyal <[email protected]>; Doherty, Declan > > <[email protected]>; De Lara Guarch, Pablo > > <[email protected]> > > Cc: Jerin Jacob Kollanukkaran <[email protected]>; Narayana Prasad > > Raju Athreya <[email protected]>; [email protected]; Ankur Dwivedi > > <[email protected]> > > 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 <[email protected]>; Declan Doherty > > > <[email protected]>; Pablo de Lara > > > <[email protected]> > > > Cc: Anoob Joseph <[email protected]>; Jerin Jacob Kollanukkaran > > > <[email protected]>; Narayana Prasad Raju Athreya > > > <[email protected]>; [email protected]; Ankur Dwivedi > > > <[email protected]> > > > 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 <[email protected]> > > > Signed-off-by: Anoob Joseph <[email protected]> > > > --- > > > 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
