[dpdk-dev] [PATCH] kvargs: fix a heap-buffer-overflow when detect list

[dpdk-dev] [PATCH] kvargs: fix a heap-buffer-overflow when detect list

[wangyunjian]

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

Re: [dpdk-dev] [PATCH] kvargs: fix a heap-buffer-overflow when detect list

[Olivier Matz]

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

[dpdk-dev] 答复: [PATCH] kvargs: fix a heap-buffer-overflow when detect list

[wangyunjian]

> -----邮件原件-----
> 发件人: 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

[dpdk-dev] [PATCH v2 0/3] kvargs fixes

[Olivier Matz]

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

[dpdk-dev] [PATCH v2 1/3] tests/kvargs: fix to consider empty elements as valid

[Olivier Matz]

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

[dpdk-dev] [PATCH v2 2/3] tests/kvargs: fix check of invalid cases

[Olivier Matz]

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

[dpdk-dev] [PATCH v2 3/3] kvargs: fix a heap buffer overflow when parsing list

[Olivier Matz]

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

Re: [dpdk-dev] [dpdk-stable] [PATCH v2 0/3] kvargs fixes

[David Marchand]

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

Re: [dpdk-dev] [dpdk-stable] [PATCH v2 0/3] kvargs fixes

[David Marchand]

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