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

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

[Anoob Joseph]

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

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

[Anoob Joseph]

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

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

[Trahe, Fiona]

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

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

[Anoob Joseph]

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

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

[Trahe, Fiona]

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.

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

[Anoob Joseph]

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

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

[Trahe, Fiona]

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

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

[Anoob Joseph]

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

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

[Akhil Goyal]

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

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

[Anoob Joseph]

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?

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

[Akhil Goyal]

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

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

[Trahe, Fiona]

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

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

[Anoob Joseph]

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

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

[Anoob Joseph]

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

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

[Anoob Joseph]

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". For such cases,
'strlen + 1' is done, so that the NULL termination also would be
considered for the comparison.

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>
---
v2:
* Using strlen + 1, instead of RTE_CRYPTODEV_NAME_MAX_LEN for the comparison.
* Strcmp would not cause this issue. Touching only the places which
  would result in the issue.

 lib/librte_cryptodev/rte_cryptodev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
index 7009735..871d7dd 100644
--- a/lib/librte_cryptodev/rte_cryptodev.c
+++ b/lib/librte_cryptodev/rte_cryptodev.c
@@ -586,7 +586,7 @@ rte_cryptodev_devices_get(const char *driver_name, uint8_t *devices,
 
 			cmp = strncmp(devs[i].device->driver->name,
 					driver_name,
-					strlen(driver_name));
+					strlen(driver_name) + 1);
 
 			if (cmp == 0)
 				devices[count++] = devs[i].data->dev_id;
@@ -1691,7 +1691,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, strlen(driver_name) + 1) == 0)
 			return driver->id;
 	}
 	return -1;
-- 
2.7.4

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

[Trahe, Fiona]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Anoob Joseph
> Sent: Monday, March 11, 2019 5:56 AM
> To: Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan <declan.doherty@intel.com>; De Lara Guarch,
> Pablo <pablo.de.lara.guarch@intel.com>
> Cc: Anoob Joseph <anoobj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>; Jerin Jacob
> Kollanukkaran <jerinj@marvell.com>; Narayana Prasad Raju Athreya <pathreya@marvell.com>; Suheil
> Chandran <schandran@marvell.com>; dev@dpdk.org
> Subject: [dpdk-dev] [PATCH v2] 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". For such cases,
> 'strlen + 1' is done, so that the NULL termination also would be
> considered for the comparison.
> 
> 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>
Acked-by: Fiona Trahe <fiona.trahe@intel.com>

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

[Anoob Joseph]

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, March 11, 2019 4:11 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: Ankur Dwivedi <adwivedi@marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Narayana Prasad Raju Athreya
> <pathreya@marvell.com>; Suheil Chandran <schandran@marvell.com>;
> dev@dpdk.org
> Subject: RE: [PATCH v2] lib/cryptodev: fix driver name comparison
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Anoob Joseph
> > Sent: Monday, March 11, 2019 5:56 AM
> > To: Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> > <declan.doherty@intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>
> > Cc: Anoob Joseph <anoobj@marvell.com>; Ankur Dwivedi
> > <adwivedi@marvell.com>; Jerin Jacob Kollanukkaran
> > <jerinj@marvell.com>; Narayana Prasad Raju Athreya
> > <pathreya@marvell.com>; Suheil Chandran <schandran@marvell.com>;
> > dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2] 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". For such
> > cases, 'strlen + 1' is done, so that the NULL termination also would
> > be considered for the comparison.
> >
> > 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>
> Acked-by: Fiona Trahe <fiona.trahe@intel.com>

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

[Anoob Joseph]

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, March 11, 2019 4:11 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: Ankur Dwivedi <adwivedi@marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Narayana Prasad Raju Athreya
> <pathreya@marvell.com>; Suheil Chandran <schandran@marvell.com>;
> dev@dpdk.org
> Subject: RE: [PATCH v2] lib/cryptodev: fix driver name comparison
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Anoob Joseph
> > Sent: Monday, March 11, 2019 5:56 AM
> > To: Akhil Goyal <akhil.goyal@nxp.com>; Doherty, Declan
> > <declan.doherty@intel.com>; De Lara Guarch, Pablo
> > <pablo.de.lara.guarch@intel.com>
> > Cc: Anoob Joseph <anoobj@marvell.com>; Ankur Dwivedi
> > <adwivedi@marvell.com>; Jerin Jacob Kollanukkaran
> > <jerinj@marvell.com>; Narayana Prasad Raju Athreya
> > <pathreya@marvell.com>; Suheil Chandran <schandran@marvell.com>;
> > dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH v2] 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". For such
> > cases, 'strlen + 1' is done, so that the NULL termination also would
> > be considered for the comparison.
> >
> > 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>
> Acked-by: Fiona Trahe <fiona.trahe@intel.com>

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

[Akhil Goyal]

On 3/11/2019 11:25 AM, Anoob Joseph wrote:
> 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". For such cases,
> 'strlen + 1' is done, so that the NULL termination also would be
> considered for the comparison.
>
> 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>
> ---
> v2:
> * Using strlen + 1, instead of RTE_CRYPTODEV_NAME_MAX_LEN for the comparison.
> * Strcmp would not cause this issue. Touching only the places which
>    would result in the issue.
>
>   lib/librte_cryptodev/rte_cryptodev.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
> index 7009735..871d7dd 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.c
> +++ b/lib/librte_cryptodev/rte_cryptodev.c
> @@ -586,7 +586,7 @@ rte_cryptodev_devices_get(const char *driver_name, uint8_t *devices,
>   
>   			cmp = strncmp(devs[i].device->driver->name,
>   					driver_name,
> -					strlen(driver_name));
> +					strlen(driver_name) + 1);
>   
>   			if (cmp == 0)
>   				devices[count++] = devs[i].data->dev_id;
> @@ -1691,7 +1691,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, strlen(driver_name) + 1) == 0)
>   			return driver->id;
>   	}
>   	return -1;
Acked-by: Akhil Goyal <akhil.goyal@nxp.com>

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

[Akhil Goyal]

On 3/11/2019 11:25 AM, Anoob Joseph wrote:
> 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". For such cases,
> 'strlen + 1' is done, so that the NULL termination also would be
> considered for the comparison.
>
> 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>
> ---
> v2:
> * Using strlen + 1, instead of RTE_CRYPTODEV_NAME_MAX_LEN for the comparison.
> * Strcmp would not cause this issue. Touching only the places which
>    would result in the issue.
>
>   lib/librte_cryptodev/rte_cryptodev.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
> index 7009735..871d7dd 100644
> --- a/lib/librte_cryptodev/rte_cryptodev.c
> +++ b/lib/librte_cryptodev/rte_cryptodev.c
> @@ -586,7 +586,7 @@ rte_cryptodev_devices_get(const char *driver_name, uint8_t *devices,
>   
>   			cmp = strncmp(devs[i].device->driver->name,
>   					driver_name,
> -					strlen(driver_name));
> +					strlen(driver_name) + 1);
>   
>   			if (cmp == 0)
>   				devices[count++] = devs[i].data->dev_id;
> @@ -1691,7 +1691,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, strlen(driver_name) + 1) == 0)
>   			return driver->id;
>   	}
>   	return -1;
Acked-by: Akhil Goyal <akhil.goyal@nxp.com>

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

[Akhil Goyal]

On 3/19/2019 7:12 PM, Akhil Goyal wrote:
>
> On 3/11/2019 11:25 AM, Anoob Joseph wrote:
>> 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". For such cases,
>> 'strlen + 1' is done, so that the NULL termination also would be
>> considered for the comparison.
>>
>> 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>
>> ---
>> v2:
>> * Using strlen + 1, instead of RTE_CRYPTODEV_NAME_MAX_LEN for the comparison.
>> * Strcmp would not cause this issue. Touching only the places which
>>     would result in the issue.
>>
>>    lib/librte_cryptodev/rte_cryptodev.c | 4 ++--
>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
>> index 7009735..871d7dd 100644
>> --- a/lib/librte_cryptodev/rte_cryptodev.c
>> +++ b/lib/librte_cryptodev/rte_cryptodev.c
>> @@ -586,7 +586,7 @@ rte_cryptodev_devices_get(const char *driver_name, uint8_t *devices,
>>    
>>    			cmp = strncmp(devs[i].device->driver->name,
>>    					driver_name,
>> -					strlen(driver_name));
>> +					strlen(driver_name) + 1);
>>    
>>    			if (cmp == 0)
>>    				devices[count++] = devs[i].data->dev_id;
>> @@ -1691,7 +1691,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, strlen(driver_name) + 1) == 0)
>>    			return driver->id;
>>    	}
>>    	return -1;
> Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
Applied to dpdk-next-crypto

Thanks.

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

[Akhil Goyal]

On 3/19/2019 7:12 PM, Akhil Goyal wrote:
>
> On 3/11/2019 11:25 AM, Anoob Joseph wrote:
>> 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". For such cases,
>> 'strlen + 1' is done, so that the NULL termination also would be
>> considered for the comparison.
>>
>> 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>
>> ---
>> v2:
>> * Using strlen + 1, instead of RTE_CRYPTODEV_NAME_MAX_LEN for the comparison.
>> * Strcmp would not cause this issue. Touching only the places which
>>     would result in the issue.
>>
>>    lib/librte_cryptodev/rte_cryptodev.c | 4 ++--
>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/librte_cryptodev/rte_cryptodev.c b/lib/librte_cryptodev/rte_cryptodev.c
>> index 7009735..871d7dd 100644
>> --- a/lib/librte_cryptodev/rte_cryptodev.c
>> +++ b/lib/librte_cryptodev/rte_cryptodev.c
>> @@ -586,7 +586,7 @@ rte_cryptodev_devices_get(const char *driver_name, uint8_t *devices,
>>    
>>    			cmp = strncmp(devs[i].device->driver->name,
>>    					driver_name,
>> -					strlen(driver_name));
>> +					strlen(driver_name) + 1);
>>    
>>    			if (cmp == 0)
>>    				devices[count++] = devs[i].data->dev_id;
>> @@ -1691,7 +1691,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, strlen(driver_name) + 1) == 0)
>>    			return driver->id;
>>    	}
>>    	return -1;
> Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
Applied to dpdk-next-crypto

Thanks.