DPDK-dev Archive on lore.kernel.org
 help / color / Atom feed
* [dpdk-dev] [PATCH] kvargs: fix a heap-buffer-overflow when detect list
@ 2020-03-19  4:38 wangyunjian
  2020-03-25 16:31 ` Olivier Matz
  2020-03-27  8:09 ` [dpdk-dev] [PATCH v2 0/3] kvargs fixes Olivier Matz
  0 siblings, 2 replies; 9+ messages in thread
From: wangyunjian @ 2020-03-19  4:38 UTC (permalink / raw)
  To: dev; +Cc: olivier.matz, jerry.lilijun, xudingke, Yunjian Wang, stable

From: Yunjian Wang <wangyunjian@huawei.com>

When an input params'value is '[', leading to the 'str' over read
or heap-buffer-overflow. So we can check the 'ctx1' length to avoid
this problem.

Fixes: cc0579f2339a ("kvargs: support list value")
Cc: stable@dpdk.org

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
---
 lib/librte_kvargs/rte_kvargs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c
index d39332999..a1144b90b 100644
--- a/lib/librte_kvargs/rte_kvargs.c
+++ b/lib/librte_kvargs/rte_kvargs.c
@@ -48,7 +48,8 @@ rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params)
 		str = kvlist->pairs[i].value;
 		if (str[0] == '[') {
 			/* Find the end of the list. */
-			while (str[strlen(str) - 1] != ']') {
+			while ((str[strlen(str) - 1] != ']') &&
+			       (strlen(ctx1) > 0)) {
 				/* Restore the comma erased by strtok_r(). */
 				str[strlen(str)] = ',';
 				/* Parse until next comma. */
-- 
2.19.1



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

* Re: [dpdk-dev] [PATCH] kvargs: fix a heap-buffer-overflow when detect list
  2020-03-19  4:38 [dpdk-dev] [PATCH] kvargs: fix a heap-buffer-overflow when detect list wangyunjian
@ 2020-03-25 16:31 ` Olivier Matz
  2020-03-26 10:01   ` [dpdk-dev] 答复: " wangyunjian
  2020-03-27  8:09 ` [dpdk-dev] [PATCH v2 0/3] kvargs fixes Olivier Matz
  1 sibling, 1 reply; 9+ messages in thread
From: Olivier Matz @ 2020-03-25 16:31 UTC (permalink / raw)
  To: wangyunjian; +Cc: dev, jerry.lilijun, xudingke, stable

Hi,

On Thu, Mar 19, 2020 at 12:38:00PM +0800, wangyunjian wrote:
> From: Yunjian Wang <wangyunjian@huawei.com>
> 
> When an input params'value is '[', leading to the 'str' over read
> or heap-buffer-overflow. So we can check the 'ctx1' length to avoid
> this problem.
> 
> Fixes: cc0579f2339a ("kvargs: support list value")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> ---
>  lib/librte_kvargs/rte_kvargs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c
> index d39332999..a1144b90b 100644
> --- a/lib/librte_kvargs/rte_kvargs.c
> +++ b/lib/librte_kvargs/rte_kvargs.c
> @@ -48,7 +48,8 @@ rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params)
>  		str = kvlist->pairs[i].value;
>  		if (str[0] == '[') {
>  			/* Find the end of the list. */
> -			while (str[strlen(str) - 1] != ']') {
> +			while ((str[strlen(str) - 1] != ']') &&
> +			       (strlen(ctx1) > 0)) {
>  				/* Restore the comma erased by strtok_r(). */
>  				str[strlen(str)] = ',';
>  				/* Parse until next comma. */

I would prefer to keep the while condition as is, like this:

                                /* Restore the comma erased by strtok_r(). */
+                               if (ctx1[0] == '\0')
+                                       return -1; /* no closing bracket */
                                str[strlen(str)] = ',';

It avoids an uneeded call to strlen(), and ensure we are returning
an error in that case.

I also wanted to add a test case, but I realized that kvargs unit tests
are broken now. I have done 2 patches to fix them.

Do you mind if I send a patchset with these 2 patches + your patch
(keeping your signed-off and doing the modification described above),
to ensure there is no implicit dependency?

Thanks,
Olivier

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

* [dpdk-dev] 答复:  [PATCH] kvargs: fix a heap-buffer-overflow when detect list
  2020-03-25 16:31 ` Olivier Matz
@ 2020-03-26 10:01   ` " wangyunjian
  0 siblings, 0 replies; 9+ messages in thread
From: wangyunjian @ 2020-03-26 10:01 UTC (permalink / raw)
  To: Olivier Matz; +Cc: dev, Lilijun (Jerry), xudingke, stable



> -----邮件原件-----
> 发件人: Olivier Matz [mailto:olivier.matz@6wind.com]
> 发送时间: 2020年3月26日 0:32
> 收件人: wangyunjian <wangyunjian@huawei.com>
> 抄送: dev@dpdk.org; Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke
> <xudingke@huawei.com>; stable@dpdk.org
> 主题: Re: [dpdk-dev] [PATCH] kvargs: fix a heap-buffer-overflow when detect
> list
> 
> Hi,
> 
> On Thu, Mar 19, 2020 at 12:38:00PM +0800, wangyunjian wrote:
> > From: Yunjian Wang <wangyunjian@huawei.com>
> >
> > When an input params'value is '[', leading to the 'str' over read or
> > heap-buffer-overflow. So we can check the 'ctx1' length to avoid this
> > problem.
> >
> > Fixes: cc0579f2339a ("kvargs: support list value")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
> > ---
> >  lib/librte_kvargs/rte_kvargs.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_kvargs/rte_kvargs.c
> > b/lib/librte_kvargs/rte_kvargs.c index d39332999..a1144b90b 100644
> > --- a/lib/librte_kvargs/rte_kvargs.c
> > +++ b/lib/librte_kvargs/rte_kvargs.c
> > @@ -48,7 +48,8 @@ rte_kvargs_tokenize(struct rte_kvargs *kvlist, const
> char *params)
> >  		str = kvlist->pairs[i].value;
> >  		if (str[0] == '[') {
> >  			/* Find the end of the list. */
> > -			while (str[strlen(str) - 1] != ']') {
> > +			while ((str[strlen(str) - 1] != ']') &&
> > +			       (strlen(ctx1) > 0)) {
> >  				/* Restore the comma erased by strtok_r(). */
> >  				str[strlen(str)] = ',';
> >  				/* Parse until next comma. */
> 
> I would prefer to keep the while condition as is, like this:
> 
>                                 /* Restore the comma erased by
> strtok_r(). */
> +                               if (ctx1[0] == '\0')
> +                                       return -1; /* no closing
> bracket
> + */
>                                 str[strlen(str)] = ',';
> 
> It avoids an uneeded call to strlen(), and ensure we are returning an error in
> that case.
> 
> I also wanted to add a test case, but I realized that kvargs unit tests are broken
> now. I have done 2 patches to fix them.
> 
> Do you mind if I send a patchset with these 2 patches + your patch (keeping
> your signed-off and doing the modification described above), to ensure there is
> no implicit dependency?

No, I don’t mind. I agree with your view.

Thanks
Yunjian
> 
> Thanks,
> Olivier

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

* [dpdk-dev] [PATCH v2 0/3] kvargs fixes
  2020-03-19  4:38 [dpdk-dev] [PATCH] kvargs: fix a heap-buffer-overflow when detect list wangyunjian
  2020-03-25 16:31 ` Olivier Matz
@ 2020-03-27  8:09 ` Olivier Matz
  2020-03-27  8:09   ` [dpdk-dev] [PATCH v2 1/3] tests/kvargs: fix to consider empty elements as valid Olivier Matz
                     ` (3 more replies)
  1 sibling, 4 replies; 9+ messages in thread
From: Olivier Matz @ 2020-03-27  8:09 UTC (permalink / raw)
  To: wangyunjian; +Cc: dev, jerry.lilijun, olivier.matz, stable, xudingke

This patchset fixes a buffer overflow in kvargs when parsing an invalid
string, and fixes the kvargs unit tests.

Olivier Matz (2):
  tests/kvargs: fix to consider empty elements as valid
  tests/kvargs: fix check of invalid cases

Yunjian Wang (1):
  kvargs: fix a heap buffer overflow when parsing list

 app/test/test_kvargs.c         | 40 +++++++++++++++++++++++++++++++---
 lib/librte_kvargs/rte_kvargs.c |  2 ++
 2 files changed, 39 insertions(+), 3 deletions(-)

-- 
2.25.1


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

* [dpdk-dev] [PATCH v2 1/3] tests/kvargs: fix to consider empty elements as valid
  2020-03-27  8:09 ` [dpdk-dev] [PATCH v2 0/3] kvargs fixes Olivier Matz
@ 2020-03-27  8:09   ` Olivier Matz
  2020-03-27  8:09   ` [dpdk-dev] [PATCH v2 2/3] tests/kvargs: fix check of invalid cases Olivier Matz
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Olivier Matz @ 2020-03-27  8:09 UTC (permalink / raw)
  To: wangyunjian; +Cc: dev, jerry.lilijun, olivier.matz, stable, xudingke

Empty elements passed to the kvargs parser are silently
ignored. Examples of valid strings:
  ""
  ","
  ",,,,,,key=val,,,,"

Fix the unit tests to conform to this behavior.

Note: the test_invalid_kvargs() function is currently broken, which
explain why the tests were not failing. It is fixed in the next commit.

Fixes: e495f5435524 ("kvargs: add test case in app/test")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_kvargs.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/app/test/test_kvargs.c b/app/test/test_kvargs.c
index a42056f36..d3db88a57 100644
--- a/app/test/test_kvargs.c
+++ b/app/test/test_kvargs.c
@@ -142,7 +142,7 @@ static int test_valid_kvargs(void)
 	valid_keys = valid_keys_list;
 	kvlist = rte_kvargs_parse(args, valid_keys);
 	if (kvlist == NULL) {
-		printf("rte_kvargs_parse() error");
+		printf("rte_kvargs_parse() error\n");
 		goto fail;
 	}
 	if (strcmp(kvlist->pairs[0].value, "[0,1]") != 0) {
@@ -157,6 +157,40 @@ static int test_valid_kvargs(void)
 	}
 	rte_kvargs_free(kvlist);
 
+	/* test using empty string (it is valid) */
+	args = "";
+	kvlist = rte_kvargs_parse(args, NULL);
+	if (kvlist == NULL) {
+		printf("rte_kvargs_parse() error\n");
+		goto fail;
+	}
+	if (rte_kvargs_count(kvlist, NULL) != 0) {
+		printf("invalid count value\n");
+		goto fail;
+	}
+	rte_kvargs_free(kvlist);
+
+	/* test using empty elements (it is valid) */
+	args = "foo=1,,check=value2,,";
+	kvlist = rte_kvargs_parse(args, NULL);
+	if (kvlist == NULL) {
+		printf("rte_kvargs_parse() error\n");
+		goto fail;
+	}
+	if (rte_kvargs_count(kvlist, NULL) != 2) {
+		printf("invalid count value\n");
+		goto fail;
+	}
+	if (rte_kvargs_count(kvlist, "foo") != 1) {
+		printf("invalid count value for 'foo'\n");
+		goto fail;
+	}
+	if (rte_kvargs_count(kvlist, "check") != 1) {
+		printf("invalid count value for 'check'\n");
+		goto fail;
+	}
+	rte_kvargs_free(kvlist);
+
 	return 0;
 
  fail:
@@ -179,7 +213,6 @@ static int test_invalid_kvargs(void)
 	const char *args_list[] = {
 		"wrong-key=x",     /* key not in valid_keys_list */
 		"foo=1,foo=",      /* empty value */
-		"foo=1,,foo=2",    /* empty key/value */
 		"foo=1,foo",       /* no value */
 		"foo=1,=2",        /* no key */
 		"foo=[1,2",        /* no closing bracket in value */
-- 
2.25.1


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

* [dpdk-dev] [PATCH v2 2/3] tests/kvargs: fix check of invalid cases
  2020-03-27  8:09 ` [dpdk-dev] [PATCH v2 0/3] kvargs fixes Olivier Matz
  2020-03-27  8:09   ` [dpdk-dev] [PATCH v2 1/3] tests/kvargs: fix to consider empty elements as valid Olivier Matz
@ 2020-03-27  8:09   ` Olivier Matz
  2020-03-27  8:09   ` [dpdk-dev] [PATCH v2 3/3] kvargs: fix a heap buffer overflow when parsing list Olivier Matz
  2020-03-27 15:16   ` [dpdk-dev] [dpdk-stable] [PATCH v2 0/3] kvargs fixes David Marchand
  3 siblings, 0 replies; 9+ messages in thread
From: Olivier Matz @ 2020-03-27  8:09 UTC (permalink / raw)
  To: wangyunjian; +Cc: dev, jerry.lilijun, olivier.matz, stable, xudingke

The return was not properly placed, and only the first test case
was validated.

Fixes: e495f5435524 ("kvargs: add test case in app/test")
Cc: stable@dpdk.org

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_kvargs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test/test_kvargs.c b/app/test/test_kvargs.c
index d3db88a57..f823b771f 100644
--- a/app/test/test_kvargs.c
+++ b/app/test/test_kvargs.c
@@ -230,8 +230,8 @@ static int test_invalid_kvargs(void)
 			rte_kvargs_free(kvlist);
 			goto fail;
 		}
-		return 0;
 	}
+	return 0;
 
  fail:
 	printf("while processing <%s>", *args);
-- 
2.25.1


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

* [dpdk-dev] [PATCH v2 3/3] kvargs: fix a heap buffer overflow when parsing list
  2020-03-27  8:09 ` [dpdk-dev] [PATCH v2 0/3] kvargs fixes Olivier Matz
  2020-03-27  8:09   ` [dpdk-dev] [PATCH v2 1/3] tests/kvargs: fix to consider empty elements as valid Olivier Matz
  2020-03-27  8:09   ` [dpdk-dev] [PATCH v2 2/3] tests/kvargs: fix check of invalid cases Olivier Matz
@ 2020-03-27  8:09   ` Olivier Matz
  2020-03-27 15:16   ` [dpdk-dev] [dpdk-stable] [PATCH v2 0/3] kvargs fixes David Marchand
  3 siblings, 0 replies; 9+ messages in thread
From: Olivier Matz @ 2020-03-27  8:09 UTC (permalink / raw)
  To: wangyunjian; +Cc: dev, jerry.lilijun, olivier.matz, stable, xudingke

From: Yunjian Wang <wangyunjian@huawei.com>

When the input string is "key=[", the ending '\0' is replaced
by a ',', leading to a heap buffer overflow.

Check the content of ctx1 to avoid this problem.

Fixes: cc0579f2339a ("kvargs: support list value")
Cc: stable@dpdk.org

Signed-off-by: Yunjian Wang <wangyunjian@huawei.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_kvargs.c         | 1 +
 lib/librte_kvargs/rte_kvargs.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/app/test/test_kvargs.c b/app/test/test_kvargs.c
index f823b771f..2a2dae43a 100644
--- a/app/test/test_kvargs.c
+++ b/app/test/test_kvargs.c
@@ -217,6 +217,7 @@ static int test_invalid_kvargs(void)
 		"foo=1,=2",        /* no key */
 		"foo=[1,2",        /* no closing bracket in value */
 		",=",              /* also test with a smiley */
+		"foo=[",           /* no value in list and no closing bracket */
 		NULL };
 	const char **args;
 	const char *valid_keys_list[] = { "foo", "check", NULL };
diff --git a/lib/librte_kvargs/rte_kvargs.c b/lib/librte_kvargs/rte_kvargs.c
index d39332999..1d815dcd9 100644
--- a/lib/librte_kvargs/rte_kvargs.c
+++ b/lib/librte_kvargs/rte_kvargs.c
@@ -50,6 +50,8 @@ rte_kvargs_tokenize(struct rte_kvargs *kvlist, const char *params)
 			/* Find the end of the list. */
 			while (str[strlen(str) - 1] != ']') {
 				/* Restore the comma erased by strtok_r(). */
+				if (ctx1[0] == '\0')
+					return -1; /* no closing bracket */
 				str[strlen(str)] = ',';
 				/* Parse until next comma. */
 				str = strtok_r(NULL, RTE_KVARGS_PAIRS_DELIM, &ctx1);
-- 
2.25.1


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

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2 0/3] kvargs fixes
  2020-03-27  8:09 ` [dpdk-dev] [PATCH v2 0/3] kvargs fixes Olivier Matz
                     ` (2 preceding siblings ...)
  2020-03-27  8:09   ` [dpdk-dev] [PATCH v2 3/3] kvargs: fix a heap buffer overflow when parsing list Olivier Matz
@ 2020-03-27 15:16   ` David Marchand
  2020-03-27 17:12     ` David Marchand
  3 siblings, 1 reply; 9+ messages in thread
From: David Marchand @ 2020-03-27 15:16 UTC (permalink / raw)
  To: Olivier Matz; +Cc: wangyunjian, dev, Lilijun (Jerry), dpdk stable, xudingke

On Fri, Mar 27, 2020 at 9:10 AM Olivier Matz <olivier.matz@6wind.com> wrote:
>
> This patchset fixes a buffer overflow in kvargs when parsing an invalid
> string, and fixes the kvargs unit tests.
>
> Olivier Matz (2):
>   tests/kvargs: fix to consider empty elements as valid
>   tests/kvargs: fix check of invalid cases
>
> Yunjian Wang (1):
>   kvargs: fix a heap buffer overflow when parsing list

For the series,
Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand


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

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2 0/3] kvargs fixes
  2020-03-27 15:16   ` [dpdk-dev] [dpdk-stable] [PATCH v2 0/3] kvargs fixes David Marchand
@ 2020-03-27 17:12     ` David Marchand
  0 siblings, 0 replies; 9+ messages in thread
From: David Marchand @ 2020-03-27 17:12 UTC (permalink / raw)
  To: Olivier Matz; +Cc: wangyunjian, dev, Lilijun (Jerry), dpdk stable, xudingke

On Fri, Mar 27, 2020 at 4:16 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Mar 27, 2020 at 9:10 AM Olivier Matz <olivier.matz@6wind.com> wrote:
> >
> > This patchset fixes a buffer overflow in kvargs when parsing an invalid
> > string, and fixes the kvargs unit tests.
> >
> > Olivier Matz (2):
> >   tests/kvargs: fix to consider empty elements as valid
> >   tests/kvargs: fix check of invalid cases
> >
> > Yunjian Wang (1):
> >   kvargs: fix a heap buffer overflow when parsing list
>
> For the series,
> Reviewed-by: David Marchand <david.marchand@redhat.com>

Applied, thanks.


-- 
David Marchand


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

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-19  4:38 [dpdk-dev] [PATCH] kvargs: fix a heap-buffer-overflow when detect list wangyunjian
2020-03-25 16:31 ` Olivier Matz
2020-03-26 10:01   ` [dpdk-dev] 答复: " wangyunjian
2020-03-27  8:09 ` [dpdk-dev] [PATCH v2 0/3] kvargs fixes Olivier Matz
2020-03-27  8:09   ` [dpdk-dev] [PATCH v2 1/3] tests/kvargs: fix to consider empty elements as valid Olivier Matz
2020-03-27  8:09   ` [dpdk-dev] [PATCH v2 2/3] tests/kvargs: fix check of invalid cases Olivier Matz
2020-03-27  8:09   ` [dpdk-dev] [PATCH v2 3/3] kvargs: fix a heap buffer overflow when parsing list Olivier Matz
2020-03-27 15:16   ` [dpdk-dev] [dpdk-stable] [PATCH v2 0/3] kvargs fixes David Marchand
2020-03-27 17:12     ` David Marchand

DPDK-dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dpdk-dev/0 dpdk-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dpdk-dev dpdk-dev/ https://lore.kernel.org/dpdk-dev \
		dev@dpdk.org
	public-inbox-index dpdk-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git