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