diff mbox

[dpdk-dev,v6,04/19] eal: fix wrong strnlen() return value in 32bit icc

Message ID 1423791501-1555-5-git-send-email-cunming.liang@intel.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Cunming Liang Feb. 13, 2015, 1:38 a.m. UTC
The problem is that strnlen() here may return invalid value with 32bit icc.
(actually it returns it’s second parameter,e.g: sysconf(_SC_ARG_MAX)).
It starts to manifest hwen max_len parameter is > 2M and using icc –m32 –O2 (or above).

Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Signed-off-by: Cunming Liang <cunming.liang@intel.com>
---
 v5 changes:
   using strlen instead of strnlen.

 lib/librte_eal/common/eal_common_options.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Neil Horman Feb. 13, 2015, 1:49 p.m. UTC | #1
On Fri, Feb 13, 2015 at 09:38:06AM +0800, Cunming Liang wrote:
> The problem is that strnlen() here may return invalid value with 32bit icc.
> (actually it returns it’s second parameter,e.g: sysconf(_SC_ARG_MAX)).
> It starts to manifest hwen max_len parameter is > 2M and using icc –m32 –O2 (or above).
> 
> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> ---
>  v5 changes:
>    using strlen instead of strnlen.
> 
>  lib/librte_eal/common/eal_common_options.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> index 178e303..9cf2faa 100644
> --- a/lib/librte_eal/common/eal_common_options.c
> +++ b/lib/librte_eal/common/eal_common_options.c
> @@ -167,7 +167,7 @@ eal_parse_coremask(const char *coremask)
>  	if (coremask[0] == '0' && ((coremask[1] == 'x')
>  		|| (coremask[1] == 'X')))
>  		coremask += 2;
> -	i = strnlen(coremask, PATH_MAX);
> +	i = strlen(coremask);
This is crash prone.  If coremask is passed in without a trailing null pointer,
strlen will return a huge value that can overrun the array.

This seems like a bug in icc, and should be fixed there.

Neil
Olivier Matz Feb. 13, 2015, 2:05 p.m. UTC | #2
Hi Neil,

On 02/13/2015 02:49 PM, Neil Horman wrote:
> On Fri, Feb 13, 2015 at 09:38:06AM +0800, Cunming Liang wrote:
>> The problem is that strnlen() here may return invalid value with 32bit icc.
>> (actually it returns it’s second parameter,e.g: sysconf(_SC_ARG_MAX)).
>> It starts to manifest hwen max_len parameter is > 2M and using icc –m32 –O2 (or above).
>>
>> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>> Signed-off-by: Cunming Liang <cunming.liang@intel.com>
>> ---
>>  v5 changes:
>>    using strlen instead of strnlen.
>>
>>  lib/librte_eal/common/eal_common_options.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
>> index 178e303..9cf2faa 100644
>> --- a/lib/librte_eal/common/eal_common_options.c
>> +++ b/lib/librte_eal/common/eal_common_options.c
>> @@ -167,7 +167,7 @@ eal_parse_coremask(const char *coremask)
>>  	if (coremask[0] == '0' && ((coremask[1] == 'x')
>>  		|| (coremask[1] == 'X')))
>>  		coremask += 2;
>> -	i = strnlen(coremask, PATH_MAX);
>> +	i = strlen(coremask);
> This is crash prone.  If coremask is passed in without a trailing null pointer,
> strlen will return a huge value that can overrun the array.

We discussed that in a previous thread:
http://dpdk.org/ml/archives/dev/2015-February/012552.html

coremask is always a valid nul-terminated string as it comes from
argv[] table.
It is not a memory fragment that is controlled by a user, so I don't
think using strnlen() instead of strlen() would solve any issue.

Regards,
Olivier
Neil Horman Feb. 13, 2015, 5:55 p.m. UTC | #3
On Fri, Feb 13, 2015 at 03:05:44PM +0100, Olivier MATZ wrote:
> Hi Neil,
> 
> On 02/13/2015 02:49 PM, Neil Horman wrote:
> > On Fri, Feb 13, 2015 at 09:38:06AM +0800, Cunming Liang wrote:
> >> The problem is that strnlen() here may return invalid value with 32bit icc.
> >> (actually it returns it’s second parameter,e.g: sysconf(_SC_ARG_MAX)).
> >> It starts to manifest hwen max_len parameter is > 2M and using icc –m32 –O2 (or above).
> >>
> >> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> >> Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> >> ---
> >>  v5 changes:
> >>    using strlen instead of strnlen.
> >>
> >>  lib/librte_eal/common/eal_common_options.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> >> index 178e303..9cf2faa 100644
> >> --- a/lib/librte_eal/common/eal_common_options.c
> >> +++ b/lib/librte_eal/common/eal_common_options.c
> >> @@ -167,7 +167,7 @@ eal_parse_coremask(const char *coremask)
> >>  	if (coremask[0] == '0' && ((coremask[1] == 'x')
> >>  		|| (coremask[1] == 'X')))
> >>  		coremask += 2;
> >> -	i = strnlen(coremask, PATH_MAX);
> >> +	i = strlen(coremask);
> > This is crash prone.  If coremask is passed in without a trailing null pointer,
> > strlen will return a huge value that can overrun the array.
> 
> We discussed that in a previous thread:
> http://dpdk.org/ml/archives/dev/2015-February/012552.html
> 
> coremask is always a valid nul-terminated string as it comes from
> argv[] table.
> It is not a memory fragment that is controlled by a user, so I don't
> think using strnlen() instead of strlen() would solve any issue.
> 
Thats absolutely false,  you can't in any way make that assertion.
eal_parse_common_option is a public API call.  An application can construct its
own string to pass into the parser.  The test applications all use the command
line functions so its not a visible issue from the test apps, but you can't
assume what the test apps do is what everyone will do.  It would be one thing if
you could make the parse_common_option function private, but with the current
layout you can't so you have to be ready for garbage input.

Neil

> Regards,
> Olivier
>
Konstantin Ananyev Feb. 13, 2015, 6:11 p.m. UTC | #4
> -----Original Message-----

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman

> Sent: Friday, February 13, 2015 5:55 PM

> To: Olivier MATZ

> Cc: dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH v6 04/19] eal: fix wrong strnlen() return value in 32bit icc

> 

> On Fri, Feb 13, 2015 at 03:05:44PM +0100, Olivier MATZ wrote:

> > Hi Neil,

> >

> > On 02/13/2015 02:49 PM, Neil Horman wrote:

> > > On Fri, Feb 13, 2015 at 09:38:06AM +0800, Cunming Liang wrote:

> > >> The problem is that strnlen() here may return invalid value with 32bit icc.

> > >> (actually it returns it’s second parameter,e.g: sysconf(_SC_ARG_MAX)).

> > >> It starts to manifest hwen max_len parameter is > 2M and using icc –m32 –O2 (or above).

> > >>

> > >> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> > >> Signed-off-by: Cunming Liang <cunming.liang@intel.com>

> > >> ---

> > >>  v5 changes:

> > >>    using strlen instead of strnlen.

> > >>

> > >>  lib/librte_eal/common/eal_common_options.c | 6 +++---

> > >>  1 file changed, 3 insertions(+), 3 deletions(-)

> > >>

> > >> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c

> > >> index 178e303..9cf2faa 100644

> > >> --- a/lib/librte_eal/common/eal_common_options.c

> > >> +++ b/lib/librte_eal/common/eal_common_options.c

> > >> @@ -167,7 +167,7 @@ eal_parse_coremask(const char *coremask)

> > >>  	if (coremask[0] == '0' && ((coremask[1] == 'x')

> > >>  		|| (coremask[1] == 'X')))

> > >>  		coremask += 2;

> > >> -	i = strnlen(coremask, PATH_MAX);

> > >> +	i = strlen(coremask);

> > > This is crash prone.  If coremask is passed in without a trailing null pointer,

> > > strlen will return a huge value that can overrun the array.

> >

> > We discussed that in a previous thread:

> > http://dpdk.org/ml/archives/dev/2015-February/012552.html

> >

> > coremask is always a valid nul-terminated string as it comes from

> > argv[] table.

> > It is not a memory fragment that is controlled by a user, so I don't

> > think using strnlen() instead of strlen() would solve any issue.

> >

> Thats absolutely false,  you can't in any way make that assertion.

> eal_parse_common_option is a public API call. 


Why do you consider is as public API?
It is declared in private librte_eal header file:
lib/librte_eal/common/eal_options.h
So yes, in theory it can be used by some other functions inside librte_eal.
But it shouldn't be visible outside librte_eal.
Konstantin

> An application can construct its

> own string to pass into the parser.  The test applications all use the command

> line functions so its not a visible issue from the test apps, but you can't

> assume what the test apps do is what everyone will do.  It would be one thing if

> you could make the parse_common_option function private, but with the current

> layout you can't so you have to be ready for garbage input.

> 

> Neil

> 

> > Regards,

> > Olivier

> >
Neil Horman Feb. 13, 2015, 8:21 p.m. UTC | #5
On Fri, Feb 13, 2015 at 06:11:02PM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman
> > Sent: Friday, February 13, 2015 5:55 PM
> > To: Olivier MATZ
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v6 04/19] eal: fix wrong strnlen() return value in 32bit icc
> > 
> > On Fri, Feb 13, 2015 at 03:05:44PM +0100, Olivier MATZ wrote:
> > > Hi Neil,
> > >
> > > On 02/13/2015 02:49 PM, Neil Horman wrote:
> > > > On Fri, Feb 13, 2015 at 09:38:06AM +0800, Cunming Liang wrote:
> > > >> The problem is that strnlen() here may return invalid value with 32bit icc.
> > > >> (actually it returns it’s second parameter,e.g: sysconf(_SC_ARG_MAX)).
> > > >> It starts to manifest hwen max_len parameter is > 2M and using icc –m32 –O2 (or above).
> > > >>
> > > >> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > >> Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > > >> ---
> > > >>  v5 changes:
> > > >>    using strlen instead of strnlen.
> > > >>
> > > >>  lib/librte_eal/common/eal_common_options.c | 6 +++---
> > > >>  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >>
> > > >> diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
> > > >> index 178e303..9cf2faa 100644
> > > >> --- a/lib/librte_eal/common/eal_common_options.c
> > > >> +++ b/lib/librte_eal/common/eal_common_options.c
> > > >> @@ -167,7 +167,7 @@ eal_parse_coremask(const char *coremask)
> > > >>  	if (coremask[0] == '0' && ((coremask[1] == 'x')
> > > >>  		|| (coremask[1] == 'X')))
> > > >>  		coremask += 2;
> > > >> -	i = strnlen(coremask, PATH_MAX);
> > > >> +	i = strlen(coremask);
> > > > This is crash prone.  If coremask is passed in without a trailing null pointer,
> > > > strlen will return a huge value that can overrun the array.
> > >
> > > We discussed that in a previous thread:
> > > http://dpdk.org/ml/archives/dev/2015-February/012552.html
> > >
> > > coremask is always a valid nul-terminated string as it comes from
> > > argv[] table.
> > > It is not a memory fragment that is controlled by a user, so I don't
> > > think using strnlen() instead of strlen() would solve any issue.
> > >
> > Thats absolutely false,  you can't in any way make that assertion.
> > eal_parse_common_option is a public API call. 
> 
> Why do you consider is as public API?
> It is declared in private librte_eal header file:
> lib/librte_eal/common/eal_options.h
> So yes, in theory it can be used by some other functions inside librte_eal.
> But it shouldn't be visible outside librte_eal.
> Konstantin
> 
I apologize, I misread the Makefile.  I thought I saw that it was symlinked as a
publically accessible header file, but thats not the case.  As such, it should
be ok.

Acked-by: Neil Horman <nhorman@tuxdriver.com>

> > An application can construct its
> > own string to pass into the parser.  The test applications all use the command
> > line functions so its not a visible issue from the test apps, but you can't
> > assume what the test apps do is what everyone will do.  It would be one thing if
> > you could make the parse_common_option function private, but with the current
> > layout you can't so you have to be ready for garbage input.
> > 
> > Neil
> > 
> > > Regards,
> > > Olivier
> > >
Cunming Liang Feb. 15, 2015, 1:32 a.m. UTC | #6
> -----Original Message-----

> From: Neil Horman [mailto:nhorman@tuxdriver.com]

> Sent: Saturday, February 14, 2015 1:55 AM

> To: Olivier MATZ

> Cc: Liang, Cunming; dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH v6 04/19] eal: fix wrong strnlen() return value in

> 32bit icc

> 

> On Fri, Feb 13, 2015 at 03:05:44PM +0100, Olivier MATZ wrote:

> > Hi Neil,

> >

> > On 02/13/2015 02:49 PM, Neil Horman wrote:

> > > On Fri, Feb 13, 2015 at 09:38:06AM +0800, Cunming Liang wrote:

> > >> The problem is that strnlen() here may return invalid value with 32bit icc.

> > >> (actually it returns it’s second parameter,e.g: sysconf(_SC_ARG_MAX)).

> > >> It starts to manifest hwen max_len parameter is > 2M and using icc –m32 –

> O2 (or above).

> > >>

> > >> Suggested-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> > >> Signed-off-by: Cunming Liang <cunming.liang@intel.com>

> > >> ---

> > >>  v5 changes:

> > >>    using strlen instead of strnlen.

> > >>

> > >>  lib/librte_eal/common/eal_common_options.c | 6 +++---

> > >>  1 file changed, 3 insertions(+), 3 deletions(-)

> > >>

> > >> diff --git a/lib/librte_eal/common/eal_common_options.c

> b/lib/librte_eal/common/eal_common_options.c

> > >> index 178e303..9cf2faa 100644

> > >> --- a/lib/librte_eal/common/eal_common_options.c

> > >> +++ b/lib/librte_eal/common/eal_common_options.c

> > >> @@ -167,7 +167,7 @@ eal_parse_coremask(const char *coremask)

> > >>  	if (coremask[0] == '0' && ((coremask[1] == 'x')

> > >>  		|| (coremask[1] == 'X')))

> > >>  		coremask += 2;

> > >> -	i = strnlen(coremask, PATH_MAX);

> > >> +	i = strlen(coremask);

> > > This is crash prone.  If coremask is passed in without a trailing null pointer,

> > > strlen will return a huge value that can overrun the array.

> >

> > We discussed that in a previous thread:

> > http://dpdk.org/ml/archives/dev/2015-February/012552.html

> >

> > coremask is always a valid nul-terminated string as it comes from

> > argv[] table.

> > It is not a memory fragment that is controlled by a user, so I don't

> > think using strnlen() instead of strlen() would solve any issue.

> >

> Thats absolutely false,  you can't in any way make that assertion.

> eal_parse_common_option is a public API call.  An application can construct its

> own string to pass into the parser.  The test applications all use the command

> line functions so its not a visible issue from the test apps, but you can't

> assume what the test apps do is what everyone will do.  It would be one thing if

> you could make the parse_common_option function private, but with the

> current

> layout you can't so you have to be ready for garbage input.

> 

> Neil

[LCM] It sounds reasonable to me. I'll rollback the code and use strnlen(coremask, ARG_MAX) instead.
> 

> > Regards,

> > Olivier

> >
Olivier Matz Feb. 16, 2015, 2:47 p.m. UTC | #7
Hi,

On 02/15/2015 02:32 AM, Liang, Cunming wrote:
>>>>> --- a/lib/librte_eal/common/eal_common_options.c
>>>>> +++ b/lib/librte_eal/common/eal_common_options.c
>>>>> @@ -167,7 +167,7 @@ eal_parse_coremask(const char *coremask)
>>>>>  	if (coremask[0] == '0' && ((coremask[1] == 'x')
>>>>>  		|| (coremask[1] == 'X')))
>>>>>  		coremask += 2;
>>>>> -	i = strnlen(coremask, PATH_MAX);
>>>>> +	i = strlen(coremask);
>>>> This is crash prone.  If coremask is passed in without a trailing null pointer,
>>>> strlen will return a huge value that can overrun the array.
>>>
>>> We discussed that in a previous thread:
>>> http://dpdk.org/ml/archives/dev/2015-February/012552.html
>>>
>>> coremask is always a valid nul-terminated string as it comes from
>>> argv[] table.
>>> It is not a memory fragment that is controlled by a user, so I don't
>>> think using strnlen() instead of strlen() would solve any issue.
>>>
>> Thats absolutely false,  you can't in any way make that assertion.
>> eal_parse_common_option is a public API call.  An application can construct its
>> own string to pass into the parser.  The test applications all use the command
>> line functions so its not a visible issue from the test apps, but you can't
>> assume what the test apps do is what everyone will do.  It would be one thing if
>> you could make the parse_common_option function private, but with the
>> current
>> layout you can't so you have to be ready for garbage input.
>>
>> Neil
> [LCM] It sounds reasonable to me. I'll rollback the code and use strnlen(coremask, ARG_MAX) instead.

I still don't agree that we should use strnlen(coremask, ARG_MAX).

The API of eal_parse_coremask() requires that a valid string is passed
as an argument, so strlen() is perfectly fine. It's up to the caller to
ensure that the string is valid.

Using strnlen(coremask, ARG_MAX) in eal_parse_coremask() with an
arbitrary length does not protect from having a segfault in case the
string is invalid and the caller's buffer length is < ARG_MAX.

This would still be true even if eal_parse_coremask() is public.

Regards,
Olivier
diff mbox

Patch

diff --git a/lib/librte_eal/common/eal_common_options.c b/lib/librte_eal/common/eal_common_options.c
index 178e303..9cf2faa 100644
--- a/lib/librte_eal/common/eal_common_options.c
+++ b/lib/librte_eal/common/eal_common_options.c
@@ -167,7 +167,7 @@  eal_parse_coremask(const char *coremask)
 	if (coremask[0] == '0' && ((coremask[1] == 'x')
 		|| (coremask[1] == 'X')))
 		coremask += 2;
-	i = strnlen(coremask, PATH_MAX);
+	i = strlen(coremask);
 	while ((i > 0) && isblank(coremask[i - 1]))
 		i--;
 	if (i == 0)
@@ -227,7 +227,7 @@  eal_parse_corelist(const char *corelist)
 	/* Remove all blank characters ahead and after */
 	while (isblank(*corelist))
 		corelist++;
-	i = strnlen(corelist, sysconf(_SC_ARG_MAX));
+	i = strlen(corelist);
 	while ((i > 0) && isblank(corelist[i - 1]))
 		i--;
 
@@ -472,7 +472,7 @@  eal_parse_lcores(const char *lcores)
 	/* Remove all blank characters ahead and after */
 	while (isblank(*lcores))
 		lcores++;
-	i = strnlen(lcores, sysconf(_SC_ARG_MAX));
+	i = strlen(lcores);
 	while ((i > 0) && isblank(lcores[i - 1]))
 		i--;