patches for DPDK stable branches
 help / color / mirror / Atom feed
* [dpdk-stable] [PATCH] lib: fix strcat with equivalent logic
@ 2019-02-14  9:30 Chaitanya Babu Talluri
  2019-02-14 14:00 ` [dpdk-stable] [dpdk-dev] " Bruce Richardson
  2019-03-08 12:45 ` [dpdk-stable] [PATCH v2] lib/cfgfile: replace strcat with strlcat Chaitanya Babu Talluri
  0 siblings, 2 replies; 7+ messages in thread
From: Chaitanya Babu Talluri @ 2019-02-14  9:30 UTC (permalink / raw)
  To: dev
  Cc: reshma.pattan, cristian.dumitrescu, jananeex.m.parthasarathy,
	Chaitanya Babu Talluri, stable

Replace strcat with concatenation logic to avoid buffer overflow.

Fixes: a6a47ac9c2 ("cfgfile: rework load function")
Cc: stable@dpdk.org

Signed-off-by: Chaitanya Babu Talluri <tallurix.chaitanya.babu@intel.com>
---
 lib/librte_cfgfile/rte_cfgfile.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 7d8c941ea..7616742f7 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -227,7 +227,15 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 			while (end != NULL) {
 				if (*(end+1) == params->comment_character) {
 					*end = '\0';
-					strcat(split[1], end+1);
+					int index = strlen(split[1]);
+					char *temp = end+1;
+					while (*temp != '\0') {
+						split[1][index] = *temp;
+						index++;
+						temp++;
+					}
+					split[1][index] = '\0';
+
 				} else
 					end++;
 				end = memchr(end, '\\', strlen(end));
-- 
2.17.2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH] lib: fix strcat with equivalent logic
  2019-02-14  9:30 [dpdk-stable] [PATCH] lib: fix strcat with equivalent logic Chaitanya Babu Talluri
@ 2019-02-14 14:00 ` Bruce Richardson
  2019-03-08 12:45 ` [dpdk-stable] [PATCH v2] lib/cfgfile: replace strcat with strlcat Chaitanya Babu Talluri
  1 sibling, 0 replies; 7+ messages in thread
From: Bruce Richardson @ 2019-02-14 14:00 UTC (permalink / raw)
  To: Chaitanya Babu Talluri
  Cc: dev, reshma.pattan, cristian.dumitrescu,
	jananeex.m.parthasarathy, stable

On Thu, Feb 14, 2019 at 09:30:31AM +0000, Chaitanya Babu Talluri wrote:
> Replace strcat with concatenation logic to avoid buffer overflow.
> 
> Fixes: a6a47ac9c2 ("cfgfile: rework load function")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chaitanya Babu Talluri <tallurix.chaitanya.babu@intel.com>
> ---
>  lib/librte_cfgfile/rte_cfgfile.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index 7d8c941ea..7616742f7 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -227,7 +227,15 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
>  			while (end != NULL) {
>  				if (*(end+1) == params->comment_character) {
>  					*end = '\0';
> -					strcat(split[1], end+1);
> +					int index = strlen(split[1]);
> +					char *temp = end+1;
> +					while (*temp != '\0') {
> +						split[1][index] = *temp;
> +						index++;
> +						temp++;
> +					}
> +					split[1][index] = '\0';
> +

I don't see how this change fixes the problem. From what I see, you have
just inlined the strcat function into the code above, without changing any
of the logic. To prevent buffer overflows using strcat, you need to
identify the total buffer length and use that when copying. I suggest
reworking this code to use strlcat.

/Bruce

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [dpdk-stable] [PATCH v2] lib/cfgfile: replace strcat with strlcat
  2019-02-14  9:30 [dpdk-stable] [PATCH] lib: fix strcat with equivalent logic Chaitanya Babu Talluri
  2019-02-14 14:00 ` [dpdk-stable] [dpdk-dev] " Bruce Richardson
@ 2019-03-08 12:45 ` Chaitanya Babu Talluri
  2019-03-08 14:02   ` Bruce Richardson
  1 sibling, 1 reply; 7+ messages in thread
From: Chaitanya Babu Talluri @ 2019-03-08 12:45 UTC (permalink / raw)
  To: dev
  Cc: reshma.pattan, jananeex.m.parthasarathy, cristian.dumitrescu,
	ferruh.yigit, bruce.richardson, Chaitanya Babu Talluri, stable

Replace strcat with strlcat to avoid buffer overflow.

Fixes: a6a47ac9c2 ("cfgfile: rework load function")
Cc: stable@dpdk.org

Signed-off-by: Chaitanya Babu Talluri <tallurix.chaitanya.babu@intel.com>
---
v2: Instead of strcat, used strlcat.
---
 lib/librte_cfgfile/rte_cfgfile.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
index 7d8c941ea..3296bb6f8 100644
--- a/lib/librte_cfgfile/rte_cfgfile.c
+++ b/lib/librte_cfgfile/rte_cfgfile.c
@@ -8,6 +8,7 @@
 #include <ctype.h>
 #include <errno.h>
 #include <rte_common.h>
+#include <rte_string_fns.h>
 
 #include "rte_cfgfile.h"
 
@@ -224,10 +225,11 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
 			_strip(split[1], strlen(split[1]));
 			char *end = memchr(split[1], '\\', strlen(split[1]));
 
+			size_t split_len = strlen(split[1]) + 1;
 			while (end != NULL) {
 				if (*(end+1) == params->comment_character) {
 					*end = '\0';
-					strcat(split[1], end+1);
+					strlcat(split[1], end+1, split_len);
 				} else
 					end++;
 				end = memchr(end, '\\', strlen(end));
-- 
2.17.2

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-stable] [PATCH v2] lib/cfgfile: replace strcat with strlcat
  2019-03-08 12:45 ` [dpdk-stable] [PATCH v2] lib/cfgfile: replace strcat with strlcat Chaitanya Babu Talluri
@ 2019-03-08 14:02   ` Bruce Richardson
  2019-03-08 17:30     ` Ferruh Yigit
  0 siblings, 1 reply; 7+ messages in thread
From: Bruce Richardson @ 2019-03-08 14:02 UTC (permalink / raw)
  To: Chaitanya Babu Talluri
  Cc: dev, reshma.pattan, jananeex.m.parthasarathy,
	cristian.dumitrescu, ferruh.yigit, stable

On Fri, Mar 08, 2019 at 12:45:50PM +0000, Chaitanya Babu Talluri wrote:
> Replace strcat with strlcat to avoid buffer overflow.
> 
> Fixes: a6a47ac9c2 ("cfgfile: rework load function")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chaitanya Babu Talluri <tallurix.chaitanya.babu@intel.com>
> ---
> v2: Instead of strcat, used strlcat.
> ---
>  lib/librte_cfgfile/rte_cfgfile.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
> index 7d8c941ea..3296bb6f8 100644
> --- a/lib/librte_cfgfile/rte_cfgfile.c
> +++ b/lib/librte_cfgfile/rte_cfgfile.c
> @@ -8,6 +8,7 @@
>  #include <ctype.h>
>  #include <errno.h>
>  #include <rte_common.h>
> +#include <rte_string_fns.h>
>  
>  #include "rte_cfgfile.h"
>  
> @@ -224,10 +225,11 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
>  			_strip(split[1], strlen(split[1]));
>  			char *end = memchr(split[1], '\\', strlen(split[1]));
>  
> +			size_t split_len = strlen(split[1]) + 1;
>  			while (end != NULL) {
>  				if (*(end+1) == params->comment_character) {
>  					*end = '\0';
> -					strcat(split[1], end+1);
> +					strlcat(split[1], end+1, split_len);

I don't think this will do what you want. Remember that strlcat takes the
total length of the buffer, which means that if split_len is set to the
current length (as you do before the while statement), then passing that
as the length parameter will cause strlcat to do nothing, since it sees the
buffer as already full.

/Bruce

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-stable] [PATCH v2] lib/cfgfile: replace strcat with strlcat
  2019-03-08 14:02   ` Bruce Richardson
@ 2019-03-08 17:30     ` Ferruh Yigit
       [not found]       ` <761FB0F2AB727F4FA9CE98D18810B0151B1F18AA@BGSMSX103.gar.corp.intel.com>
  0 siblings, 1 reply; 7+ messages in thread
From: Ferruh Yigit @ 2019-03-08 17:30 UTC (permalink / raw)
  To: Bruce Richardson, Chaitanya Babu Talluri
  Cc: dev, reshma.pattan, jananeex.m.parthasarathy,
	cristian.dumitrescu, stable

On 3/8/2019 2:02 PM, Bruce Richardson wrote:
> On Fri, Mar 08, 2019 at 12:45:50PM +0000, Chaitanya Babu Talluri wrote:
>> Replace strcat with strlcat to avoid buffer overflow.
>>
>> Fixes: a6a47ac9c2 ("cfgfile: rework load function")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chaitanya Babu Talluri <tallurix.chaitanya.babu@intel.com>
>> ---
>> v2: Instead of strcat, used strlcat.
>> ---
>>  lib/librte_cfgfile/rte_cfgfile.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
>> index 7d8c941ea..3296bb6f8 100644
>> --- a/lib/librte_cfgfile/rte_cfgfile.c
>> +++ b/lib/librte_cfgfile/rte_cfgfile.c
>> @@ -8,6 +8,7 @@
>>  #include <ctype.h>
>>  #include <errno.h>
>>  #include <rte_common.h>
>> +#include <rte_string_fns.h>
>>  
>>  #include "rte_cfgfile.h"
>>  
>> @@ -224,10 +225,11 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
>>  			_strip(split[1], strlen(split[1]));
>>  			char *end = memchr(split[1], '\\', strlen(split[1]));
>>  
>> +			size_t split_len = strlen(split[1]) + 1;
>>  			while (end != NULL) {
>>  				if (*(end+1) == params->comment_character) {
>>  					*end = '\0';
>> -					strcat(split[1], end+1);
>> +					strlcat(split[1], end+1, split_len);
> 
> I don't think this will do what you want. Remember that strlcat takes the
> total length of the buffer, which means that if split_len is set to the
> current length (as you do before the while statement), then passing that
> as the length parameter will cause strlcat to do nothing, since it sees the
> buffer as already full.

The logic doesn't lengthen the 'split[1]' content, indeed it reduces the initial
size although it uses string concatenation, that is why it should be OK to use
'split_len' here.

What code does is, it finds specific char in 'split' buffer and removes it by
shifting remaining chars one byte to the left. So it shouldn't pass the initial
size of the buffer.

There is a overlapping strings concern, which 'strcat' & 'strlcat' don't
support, but I guess it is OK here since we are sure that strings are separated
by a NULL, so where a char read and written should be different although overall
dst and src buffers overlap.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-stable] [PATCH v2] lib/cfgfile: replace strcat with strlcat
       [not found]       ` <761FB0F2AB727F4FA9CE98D18810B0151B1F18AA@BGSMSX103.gar.corp.intel.com>
@ 2019-03-27 11:37         ` Ferruh Yigit
  2019-04-05 12:53           ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon
  0 siblings, 1 reply; 7+ messages in thread
From: Ferruh Yigit @ 2019-03-27 11:37 UTC (permalink / raw)
  To: Chaitanya Babu, TalluriX, Richardson, Bruce
  Cc: dev, Pattan, Reshma, Parthasarathy, JananeeX M, Dumitrescu,
	Cristian, stable

On 3/26/2019 10:04 AM, Chaitanya Babu, TalluriX wrote:
> Hi 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Friday, March 8, 2019 11:01 PM
>> To: Richardson, Bruce <bruce.richardson@intel.com>; Chaitanya Babu, TalluriX
>> <tallurix.chaitanya.babu@intel.com>
>> Cc: dev@dpdk.org; Pattan, Reshma <reshma.pattan@intel.com>;
>> Parthasarathy, JananeeX M <jananeex.m.parthasarathy@intel.com>;
>> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>; stable@dpdk.org
>> Subject: Re: [PATCH v2] lib/cfgfile: replace strcat with strlcat
>>
>> On 3/8/2019 2:02 PM, Bruce Richardson wrote:
>>> On Fri, Mar 08, 2019 at 12:45:50PM +0000, Chaitanya Babu Talluri wrote:
>>>> Replace strcat with strlcat to avoid buffer overflow.
>>>>
>>>> Fixes: a6a47ac9c2 ("cfgfile: rework load function")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Chaitanya Babu Talluri
>>>> <tallurix.chaitanya.babu@intel.com>
>>>> ---
>>>> v2: Instead of strcat, used strlcat.
>>>> ---
>>>>  lib/librte_cfgfile/rte_cfgfile.c | 4 +++-
>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/librte_cfgfile/rte_cfgfile.c
>>>> b/lib/librte_cfgfile/rte_cfgfile.c
>>>> index 7d8c941ea..3296bb6f8 100644
>>>> --- a/lib/librte_cfgfile/rte_cfgfile.c
>>>> +++ b/lib/librte_cfgfile/rte_cfgfile.c
>>>> @@ -8,6 +8,7 @@
>>>>  #include <ctype.h>
>>>>  #include <errno.h>
>>>>  #include <rte_common.h>
>>>> +#include <rte_string_fns.h>
>>>>
>>>>  #include "rte_cfgfile.h"
>>>>
>>>> @@ -224,10 +225,11 @@ rte_cfgfile_load_with_params(const char
>> *filename, int flags,
>>>>  			_strip(split[1], strlen(split[1]));
>>>>  			char *end = memchr(split[1], '\\', strlen(split[1]));
>>>>
>>>> +			size_t split_len = strlen(split[1]) + 1;
>>>>  			while (end != NULL) {
>>>>  				if (*(end+1) == params->comment_character)
>> {
>>>>  					*end = '\0';
>>>> -					strcat(split[1], end+1);
>>>> +					strlcat(split[1], end+1, split_len);
>>>
>>> I don't think this will do what you want. Remember that strlcat takes
>>> the total length of the buffer, which means that if split_len is set
>>> to the current length (as you do before the while statement), then
>>> passing that as the length parameter will cause strlcat to do nothing,
>>> since it sees the buffer as already full.
>>
>> The logic doesn't lengthen the 'split[1]' content, indeed it reduces the initial
>> size although it uses string concatenation, that is why it should be OK to use
>> 'split_len' here.
>>
>> What code does is, it finds specific char in 'split' buffer and removes it by
>> shifting remaining chars one byte to the left. So it shouldn't pass the initial size
>> of the buffer.
>>
>> There is a overlapping strings concern, which 'strcat' & 'strlcat' don't support,
>> but I guess it is OK here since we are sure that strings are separated by a
>> NULL, so where a char read and written should be different although overall
>> dst and src buffers overlap.
> 
> Yes, although the same string is manipulated the split string (*end = '\0') is separated with NULL.
> Strlcat works fine here and expected concatenation  is happening.
> If there are no further comments request for ACK please.

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [dpdk-stable] [dpdk-dev] [PATCH v2] lib/cfgfile: replace strcat with strlcat
  2019-03-27 11:37         ` Ferruh Yigit
@ 2019-04-05 12:53           ` Thomas Monjalon
  0 siblings, 0 replies; 7+ messages in thread
From: Thomas Monjalon @ 2019-04-05 12:53 UTC (permalink / raw)
  To: Chaitanya Babu, TalluriX
  Cc: dev, Ferruh Yigit, Richardson, Bruce, Pattan, Reshma,
	Parthasarathy, JananeeX M, Dumitrescu, Cristian, stable

27/03/2019 12:37, Ferruh Yigit:
> On 3/26/2019 10:04 AM, Chaitanya Babu, TalluriX wrote:
> > From: Yigit, Ferruh
> >> On 3/8/2019 2:02 PM, Bruce Richardson wrote:
> >>> On Fri, Mar 08, 2019 at 12:45:50PM +0000, Chaitanya Babu Talluri wrote:
> >>>> Replace strcat with strlcat to avoid buffer overflow.
> >>>>
> >>>> Fixes: a6a47ac9c2 ("cfgfile: rework load function")
> >>>> Cc: stable@dpdk.org
> >>>>
> >>>> Signed-off-by: Chaitanya Babu Talluri
> >>>> <tallurix.chaitanya.babu@intel.com>
> >>>> ---
> >>>> @@ -224,10 +225,11 @@ rte_cfgfile_load_with_params(const char
> >> *filename, int flags,
> >>>>  			_strip(split[1], strlen(split[1]));
> >>>>  			char *end = memchr(split[1], '\\', strlen(split[1]));
> >>>>
> >>>> +			size_t split_len = strlen(split[1]) + 1;
> >>>>  			while (end != NULL) {
> >>>>  				if (*(end+1) == params->comment_character)
> >> {
> >>>>  					*end = '\0';
> >>>> -					strcat(split[1], end+1);
> >>>> +					strlcat(split[1], end+1, split_len);
> >>>
> >>> I don't think this will do what you want. Remember that strlcat takes
> >>> the total length of the buffer, which means that if split_len is set
> >>> to the current length (as you do before the while statement), then
> >>> passing that as the length parameter will cause strlcat to do nothing,
> >>> since it sees the buffer as already full.
> >>
> >> The logic doesn't lengthen the 'split[1]' content, indeed it reduces the initial
> >> size although it uses string concatenation, that is why it should be OK to use
> >> 'split_len' here.
> >>
> >> What code does is, it finds specific char in 'split' buffer and removes it by
> >> shifting remaining chars one byte to the left. So it shouldn't pass the initial size
> >> of the buffer.
> >>
> >> There is a overlapping strings concern, which 'strcat' & 'strlcat' don't support,
> >> but I guess it is OK here since we are sure that strings are separated by a
> >> NULL, so where a char read and written should be different although overall
> >> dst and src buffers overlap.
> > 
> > Yes, although the same string is manipulated the split string (*end = '\0') is separated with NULL.
> > Strlcat works fine here and expected concatenation  is happening.
> > If there are no further comments request for ACK please.
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Applied, thanks




^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-04-05 12:53 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-14  9:30 [dpdk-stable] [PATCH] lib: fix strcat with equivalent logic Chaitanya Babu Talluri
2019-02-14 14:00 ` [dpdk-stable] [dpdk-dev] " Bruce Richardson
2019-03-08 12:45 ` [dpdk-stable] [PATCH v2] lib/cfgfile: replace strcat with strlcat Chaitanya Babu Talluri
2019-03-08 14:02   ` Bruce Richardson
2019-03-08 17:30     ` Ferruh Yigit
     [not found]       ` <761FB0F2AB727F4FA9CE98D18810B0151B1F18AA@BGSMSX103.gar.corp.intel.com>
2019-03-27 11:37         ` Ferruh Yigit
2019-04-05 12:53           ` [dpdk-stable] [dpdk-dev] " Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).