diff mbox series

lib/cryptodev: fix driver name comparison

Message ID 1549279528-10397-1-git-send-email-anoobj@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series lib/cryptodev: fix driver name comparison |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Anoob Joseph Feb. 4, 2019, 11:26 a.m. UTC 
  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

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

Comments

Anoob Joseph Feb. 20, 2019, 3:41 p.m. UTC | #1 
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
> 
> 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) &&
>  				(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);
> 
>  			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
Fiona Trahe Feb. 21, 2019, 5:03 p.m. UTC | #2 
> -----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
Anoob Joseph Feb. 22, 2019, 4:47 a.m. UTC | #3 
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
Fiona Trahe Feb. 22, 2019, 3:39 p.m. UTC | #4 
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 Joseph Feb. 23, 2019, 6:11 a.m. UTC | #5 
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?

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
Fiona Trahe Feb. 25, 2019, 11:52 a.m. UTC | #6 
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
Anoob Joseph Feb. 28, 2019, 6:48 a.m. UTC | #7 
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
Akhil Goyal Feb. 28, 2019, 8:51 a.m. UTC | #8 
Hi Anoob,

On 2/28/2019 12:18 PM, Anoob Joseph wrote:
> 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)

consider using "strlen(name) + 1" instead of RTE_CRYPTODEV_NAME_MAX_LEN. 
This will not cause any ABI breakage in my opinion and and will check 
till we get a null terminated string in both the strings.
What say?

-Akhil
Anoob Joseph Feb. 28, 2019, 9:27 a.m. UTC | #9 
Hi Akhil,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Thursday, February 28, 2019 2:22 PM
> To: Anoob Joseph <anoobj@marvell.com>; Trahe, Fiona
> <fiona.trahe@intel.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: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name comparison
> 
> Hi Anoob,
> 
> On 2/28/2019 12:18 PM, Anoob Joseph wrote:
> > 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)
> 
> consider using "strlen(name) + 1" instead of
> RTE_CRYPTODEV_NAME_MAX_LEN.
> This will not cause any ABI breakage in my opinion and and will check till we
> get a null terminated string in both the strings.
> What say?

[Anoob] In that  case, I'll restrict the patch to two places. Wherever strlen(name) is used, I'll make it strlen(name)+1. I won't touch strcmp ones as that would work as is. Shall I prepare a v2?
Akhil Goyal Feb. 28, 2019, 10:20 a.m. UTC | #10 
On 2/28/2019 2:57 PM, Anoob Joseph wrote:
> Hi Akhil,
>
> Please see inline.
>
> Thanks,
> Anoob
>
>> -----Original Message-----
>> From: Akhil Goyal <akhil.goyal@nxp.com>
>> Sent: Thursday, February 28, 2019 2:22 PM
>> To: Anoob Joseph <anoobj@marvell.com>; Trahe, Fiona
>> <fiona.trahe@intel.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: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name comparison
>>
>> Hi Anoob,
>>
>> On 2/28/2019 12:18 PM, Anoob Joseph wrote:
>>> 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)
>> consider using "strlen(name) + 1" instead of
>> RTE_CRYPTODEV_NAME_MAX_LEN.
>> This will not cause any ABI breakage in my opinion and and will check till we
>> get a null terminated string in both the strings.
>> What say?
> [Anoob] In that  case, I'll restrict the patch to two places. Wherever strlen(name) is used, I'll make it strlen(name)+1. I won't touch strcmp ones as that would work as is. Shall I prepare a v2?
I think it should be fine.

Fiona,
Any comments?

-Akhil
Fiona Trahe Feb. 28, 2019, 2:30 p.m. UTC | #11 
Hi Akhil, Anoob,

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Thursday, February 28, 2019 10:20 AM
> To: Anoob Joseph <anoobj@marvell.com>; Trahe, Fiona <fiona.trahe@intel.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: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name comparison
> 
> 
> 
> On 2/28/2019 2:57 PM, Anoob Joseph wrote:
> > Hi Akhil,
> >
> > Please see inline.
> >
> > Thanks,
> > Anoob
> >
> >> -----Original Message-----
> >> From: Akhil Goyal <akhil.goyal@nxp.com>
> >> Sent: Thursday, February 28, 2019 2:22 PM
> >> To: Anoob Joseph <anoobj@marvell.com>; Trahe, Fiona
> >> <fiona.trahe@intel.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: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name comparison
> >>
> >> Hi Anoob,
> >>
> >> On 2/28/2019 12:18 PM, Anoob Joseph wrote:
> >>> 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)
> >> consider using "strlen(name) + 1" instead of
> >> RTE_CRYPTODEV_NAME_MAX_LEN.
> >> This will not cause any ABI breakage in my opinion and and will check till we
> >> get a null terminated string in both the strings.
> >> What say?
> > [Anoob] In that  case, I'll restrict the patch to two places. Wherever strlen(name) is used, I'll make it
> strlen(name)+1. I won't touch strcmp ones as that would work as is. Shall I prepare a v2?
> I think it should be fine.
> 
> Fiona,
> Any comments?
[Fiona] Good idea. That should be ok. 
> 
> -Akhil
Anoob Joseph March 1, 2019, 6:24 a.m. UTC | #12 
Hi Fiona, Akhil,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Trahe, Fiona
> Sent: Thursday, February 28, 2019 8:00 PM
> To: Akhil Goyal <akhil.goyal@nxp.com>; Anoob Joseph
> <anoobj@marvell.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>; Trahe, Fiona <fiona.trahe@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name comparison
> 
> Hi Akhil, Anoob,
> 
> > -----Original Message-----
> > From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> > Sent: Thursday, February 28, 2019 10:20 AM
> > To: Anoob Joseph <anoobj@marvell.com>; Trahe, Fiona
> > <fiona.trahe@intel.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: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name
> > comparison
> >
> >
> >
> > On 2/28/2019 2:57 PM, Anoob Joseph wrote:
> > > Hi Akhil,
> > >
> > > Please see inline.
> > >
> > > Thanks,
> > > Anoob
> > >
> > >> -----Original Message-----
> > >> From: Akhil Goyal <akhil.goyal@nxp.com>
> > >> Sent: Thursday, February 28, 2019 2:22 PM
> > >> To: Anoob Joseph <anoobj@marvell.com>; Trahe, Fiona
> > >> <fiona.trahe@intel.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: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name
> > >> comparison
> > >>
> > >> Hi Anoob,
> > >>
> > >> On 2/28/2019 12:18 PM, Anoob Joseph wrote:
> > >>> 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)
> > >> consider using "strlen(name) + 1" instead of
> > >> RTE_CRYPTODEV_NAME_MAX_LEN.
> > >> This will not cause any ABI breakage in my opinion and and will
> > >> check till we get a null terminated string in both the strings.
> > >> What say?
> > > [Anoob] In that  case, I'll restrict the patch to two places.
> > > Wherever strlen(name) is used, I'll make it
> > strlen(name)+1. I won't touch strcmp ones as that would work as is. Shall I
> prepare a v2?
> > I think it should be fine.
> >
> > Fiona,
> > Any comments?
> [Fiona] Good idea. That should be ok.

[Anoob] Another thought. If we are fine with doing strlen of input buffer, then using strcmp would also do. That way the usage also would be uniform in the file.

Thanks,
Anoob
Anoob Joseph March 7, 2019, 5:51 a.m. UTC | #13 
Hi Akhil, Fiona,

Would the usage of strcmp be alright? Please check my comment inline.

Thanks,
Anoob

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Anoob Joseph
> Sent: Friday, March 1, 2019 11:55 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: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name comparison
> 
> Hi Fiona, Akhil,
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Trahe, Fiona
> > Sent: Thursday, February 28, 2019 8:00 PM
> > To: Akhil Goyal <akhil.goyal@nxp.com>; Anoob Joseph
> > <anoobj@marvell.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>; Trahe, Fiona <fiona.trahe@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name
> > comparison
> >
> > Hi Akhil, Anoob,
> >
> > > -----Original Message-----
> > > From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> > > Sent: Thursday, February 28, 2019 10:20 AM
> > > To: Anoob Joseph <anoobj@marvell.com>; Trahe, Fiona
> > > <fiona.trahe@intel.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: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name
> > > comparison
> > >
> > >
> > >
> > > On 2/28/2019 2:57 PM, Anoob Joseph wrote:
> > > > Hi Akhil,
> > > >
> > > > Please see inline.
> > > >
> > > > Thanks,
> > > > Anoob
> > > >
> > > >> -----Original Message-----
> > > >> From: Akhil Goyal <akhil.goyal@nxp.com>
> > > >> Sent: Thursday, February 28, 2019 2:22 PM
> > > >> To: Anoob Joseph <anoobj@marvell.com>; Trahe, Fiona
> > > >> <fiona.trahe@intel.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: [dpdk-dev] [PATCH] lib/cryptodev: fix driver name
> > > >> comparison
> > > >>
> > > >> Hi Anoob,
> > > >>
> > > >> On 2/28/2019 12:18 PM, Anoob Joseph wrote:
> > > >>> 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)
> > > >> consider using "strlen(name) + 1" instead of
> > > >> RTE_CRYPTODEV_NAME_MAX_LEN.
> > > >> This will not cause any ABI breakage in my opinion and and will
> > > >> check till we get a null terminated string in both the strings.
> > > >> What say?
> > > > [Anoob] In that  case, I'll restrict the patch to two places.
> > > > Wherever strlen(name) is used, I'll make it
> > > strlen(name)+1. I won't touch strcmp ones as that would work as is.
> > > Shall I
> > prepare a v2?
> > > I think it should be fine.
> > >
> > > Fiona,
> > > Any comments?
> > [Fiona] Good idea. That should be ok.
> 
> [Anoob] Another thought. If we are fine with doing strlen of input buffer,
> then using strcmp would also do. That way the usage also would be uniform
> in the file.
> 
> Thanks,
> Anoob
diff mbox series

Patch

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) &&
 				(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);
 
 			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;