[patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check

[patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

One cannot depend on *argv being null in case of no arg is left on the
command line. For example in batch mode, this is not always true. Check
argc instead to prevent crash.

Reported-by: Alex Kushnarov <alexanderk@mellanox.com>
Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies")
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 tc/m_action.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tc/m_action.c b/tc/m_action.c
index ab6bc0ad28ff..0f9c3a27795d 100644
--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -222,7 +222,7 @@ done0:
 				goto bad_val;
 			}
 
-			if (*argv && strcmp(*argv, "cookie") == 0) {
+			if (argc && strcmp(*argv, "cookie") == 0) {
 				size_t slen;
 
 				NEXT_ARG();
-- 
2.21.0

[patch iproute2 2/2] tc: batch: fix line/line_next processing in batch

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

When getcmdline fails, there is no valid string in line_next.
So change the flow and don't process it. Alongside with that,
free the previous line buffer and prevent memory leak.

Fixes: 485d0c6001c4 ("tc: Add batchsize feature for filter and actions")
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 tc/tc.c | 42 ++++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/tc/tc.c b/tc/tc.c
index 64e342dd85bf..8abc82aedcf8 100644
--- a/tc/tc.c
+++ b/tc/tc.c
@@ -325,11 +325,10 @@ static int batch(const char *name)
 {
 	struct batch_buf *head = NULL, *tail = NULL, *buf_pool = NULL;
 	char *largv[100], *largv_next[100];
-	char *line, *line_next = NULL;
+	char *line = NULL, *line_next = NULL;
 	bool bs_enabled = false;
 	bool lastline = false;
 	int largc, largc_next;
-	bool bs_enabled_saved;
 	bool bs_enabled_next;
 	int batchsize = 0;
 	size_t len = 0;
@@ -360,11 +359,13 @@ static int batch(const char *name)
 	largc = makeargs(line, largv, 100);
 	bs_enabled = batchsize_enabled(largc, largv);
 	do {
-		if (getcmdline(&line_next, &len, stdin) == -1)
+		if (getcmdline(&line_next, &len, stdin) == -1) {
 			lastline = true;
-
-		largc_next = makeargs(line_next, largv_next, 100);
-		bs_enabled_next = batchsize_enabled(largc_next, largv_next);
+			bs_enabled_next = false;
+		} else {
+			largc_next = makeargs(line_next, largv_next, 100);
+			bs_enabled_next = batchsize_enabled(largc_next, largv_next);
+		}
 		if (bs_enabled) {
 			struct batch_buf *buf;
 
@@ -389,17 +390,8 @@ static int batch(const char *name)
 		else
 			send = true;
 
-		line = line_next;
-		line_next = NULL;
-		len = 0;
-		bs_enabled_saved = bs_enabled;
-		bs_enabled = bs_enabled_next;
-
-		if (largc == 0) {
-			largc = largc_next;
-			memcpy(largv, largv_next, largc * sizeof(char *));
-			continue;	/* blank line */
-		}
+		if (largc == 0)
+			goto to_next_line;	/* blank line */
 
 		err = do_cmd(largc, largv, tail == NULL ? NULL : tail->buf,
 			     tail == NULL ? 0 : sizeof(tail->buf));
@@ -411,10 +403,8 @@ static int batch(const char *name)
 			if (!force)
 				break;
 		}
-		largc = largc_next;
-		memcpy(largv, largv_next, largc * sizeof(char *));
 
-		if (send && bs_enabled_saved) {
+		if (send && bs_enabled) {
 			struct iovec *iov, *iovs;
 			struct batch_buf *buf;
 			struct nlmsghdr *n;
@@ -438,6 +428,18 @@ static int batch(const char *name)
 			}
 			batchsize = 0;
 		}
+
+to_next_line:
+		if (lastline)
+			continue;
+		free(line);
+		line = line_next;
+		line_next = NULL;
+		len = 0;
+		bs_enabled = bs_enabled_next;
+		largc = largc_next;
+		memcpy(largv, largv_next, largc * sizeof(char *));
+
 	} while (!lastline);
 
 	free_batch_bufs(&buf_pool);
-- 
2.21.0

Re: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check

[Stephen Hemminger]

On Tue, 23 Jul 2019 13:25:37 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> From: Jiri Pirko <jiri@mellanox.com>
> 
> One cannot depend on *argv being null in case of no arg is left on the
> command line. For example in batch mode, this is not always true. Check
> argc instead to prevent crash.
> 
> Reported-by: Alex Kushnarov <alexanderk@mellanox.com>
> Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  tc/m_action.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tc/m_action.c b/tc/m_action.c
> index ab6bc0ad28ff..0f9c3a27795d 100644
> --- a/tc/m_action.c
> +++ b/tc/m_action.c
> @@ -222,7 +222,7 @@ done0:
>  				goto bad_val;
>  			}
>  
> -			if (*argv && strcmp(*argv, "cookie") == 0) {
> +			if (argc && strcmp(*argv, "cookie") == 0) {
>  				size_t slen;
>  
>  				NEXT_ARG();

Ok, but we should also fix batch mode to null last argv

Re: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check

[Stephen Hemminger]

On Tue, 23 Jul 2019 13:25:37 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> From: Jiri Pirko <jiri@mellanox.com>
> 
> One cannot depend on *argv being null in case of no arg is left on the
> command line. For example in batch mode, this is not always true. Check
> argc instead to prevent crash.
> 
> Reported-by: Alex Kushnarov <alexanderk@mellanox.com>
> Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Actually makeargs does NULL terminate the last arg so what input
to batchmode is breaking this?

Re: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check

[Jiri Pirko]

Tue, Jul 23, 2019 at 07:54:01PM CEST, stephen@networkplumber.org wrote:
>On Tue, 23 Jul 2019 13:25:37 +0200
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> One cannot depend on *argv being null in case of no arg is left on the
>> command line. For example in batch mode, this is not always true. Check
>> argc instead to prevent crash.
>> 
>> Reported-by: Alex Kushnarov <alexanderk@mellanox.com>
>> Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies")
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>
>Actually makeargs does NULL terminate the last arg so what input
>to batchmode is breaking this?

Interesting, there must be another but out there then.

My input is:
filter add dev testdummy parent ffff: protocol all prio 11000 flower action drop
filter add dev testdummy parent ffff: protocol ipv4 prio 1 flower dst_mac 11:22:33:44:55:66 action drop

RE: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check

[David Laight]

From: Stephen Hemminger
> Sent: 23 July 2019 18:54
> 
> On Tue, 23 Jul 2019 13:25:37 +0200
> Jiri Pirko <jiri@resnulli.us> wrote:
> 
> > From: Jiri Pirko <jiri@mellanox.com>
> >
> > One cannot depend on *argv being null in case of no arg is left on the
> > command line. For example in batch mode, this is not always true. Check
> > argc instead to prevent crash.

Hmmm... expecting the increments of argv and decrements of argc to match
it probably wishful thinking....
A lot of parsers don't even look at argc.

> Actually makeargs does NULL terminate the last arg so what input
> to batchmode is breaking this?

The 'usual' problem is an extra increment of argv because the last entry
was something that 'eats' two or more entries.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check

[Stephen Hemminger]

On Tue, 23 Jul 2019 21:36:00 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> Tue, Jul 23, 2019 at 07:54:01PM CEST, stephen@networkplumber.org wrote:
> >On Tue, 23 Jul 2019 13:25:37 +0200
> >Jiri Pirko <jiri@resnulli.us> wrote:
> >  
> >> From: Jiri Pirko <jiri@mellanox.com>
> >> 
> >> One cannot depend on *argv being null in case of no arg is left on the
> >> command line. For example in batch mode, this is not always true. Check
> >> argc instead to prevent crash.
> >> 
> >> Reported-by: Alex Kushnarov <alexanderk@mellanox.com>
> >> Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies")
> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com>  
> >
> >Actually makeargs does NULL terminate the last arg so what input
> >to batchmode is breaking this?  
> 
> Interesting, there must be another but out there then.
> 
> My input is:
> filter add dev testdummy parent ffff: protocol all prio 11000 flower action drop
> filter add dev testdummy parent ffff: protocol ipv4 prio 1 flower dst_mac 11:22:33:44:55:66 action drop

This maybe related. Looks like the batchsize patches had issues.

# valgrind ./tc/tc -batch filter.bat 
==27348== Memcheck, a memory error detector
==27348== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==27348== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==27348== Command: ./tc/tc -batch filter.bat
==27348== 
==27348== Conditional jump or move depends on uninitialised value(s)
==27348==    at 0x4EE9C0C: getdelim (iogetdelim.c:59)
==27348==    by 0x152A37: getline (stdio.h:120)
==27348==    by 0x152A37: getcmdline (utils.c:1311)
==27348==    by 0x115543: batch (tc.c:358)
==27348==    by 0x4E9D09A: (below main) (libc-start.c:308)
==27348== 
==27348== Conditional jump or move depends on uninitialised value(s)
==27348==    at 0x152BE4: makeargs (utils.c:1359)
==27348==    by 0x115614: batch (tc.c:366)
==27348==    by 0x4E9D09A: (below main) (libc-start.c:308)
==27348== 
==27348== Conditional jump or move depends on uninitialised value(s)
==27348==    at 0x11EBFD: parse_action (m_action.c:225)
==27348==    by 0x13633E: flower_parse_opt (f_flower.c:1285)
==27348==    by 0x1190EB: tc_filter_modify (tc_filter.c:217)
==27348==    by 0x115674: batch (tc.c:404)
==27348==    by 0x4E9D09A: (below main) (libc-start.c:308)
==27348== 
==27348== Use of uninitialised value of size 8
==27348==    at 0x11EC0B: parse_action (m_action.c:225)
==27348==    by 0x13633E: flower_parse_opt (f_flower.c:1285)
==27348==    by 0x1190EB: tc_filter_modify (tc_filter.c:217)
==27348==    by 0x115674: batch (tc.c:404)
==27348==    by 0x4E9D09A: (below main) (libc-start.c:308)
==27348== 
Error: Parent Qdisc doesn't exists.
Error: Parent Qdisc doesn't exists.
Command failed filter.bat:1

Re: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check

[Stephen Hemminger]

On Tue, 23 Jul 2019 13:25:37 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> From: Jiri Pirko <jiri@mellanox.com>
> 
> One cannot depend on *argv being null in case of no arg is left on the
> command line. For example in batch mode, this is not always true. Check
> argc instead to prevent crash.
> 
> Reported-by: Alex Kushnarov <alexanderk@mellanox.com>
> Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
>  tc/m_action.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tc/m_action.c b/tc/m_action.c
> index ab6bc0ad28ff..0f9c3a27795d 100644
> --- a/tc/m_action.c
> +++ b/tc/m_action.c
> @@ -222,7 +222,7 @@ done0:
>  				goto bad_val;
>  			}
>  
> -			if (*argv && strcmp(*argv, "cookie") == 0) {
> +			if (argc && strcmp(*argv, "cookie") == 0) {
>  				size_t slen;
>  
>  				NEXT_ARG();


The logic here is broken at end of file.

	do {
		if (getcmdline(&line_next, &len, stdin) == -1)
			lastline = true;

		largc_next = makeargs(line_next, largv_next, 100);
		bs_enabled_next = batchsize_enabled(largc_next, largv_next);
		if (bs_enabled) {
			struct batch_


getcmdline() will return -1 at end of file.
The code will call make_args on an uninitialized pointer.

I see lots of other unnecessary complexity in the whole batch logic.
It needs to be rewritten.

Rather than me fixing the code, I am probably going to revert.

commit 485d0c6001c4aa134b99c86913d6a7089b7b2ab0
Author: Chris Mi <chrism@mellanox.com>
Date:   Fri Jan 12 14:13:16 2018 +0900

    tc: Add batchsize feature for filter and actions

Re: [patch iproute2 2/2] tc: batch: fix line/line_next processing in batch

[Stephen Hemminger]

On Tue, 23 Jul 2019 13:25:38 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> From: Jiri Pirko <jiri@mellanox.com>
> 
> When getcmdline fails, there is no valid string in line_next.
> So change the flow and don't process it. Alongside with that,
> free the previous line buffer and prevent memory leak.
> 
> Fixes: 485d0c6001c4 ("tc: Add batchsize feature for filter and actions")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>


This is not sufficient to avoid valgrind detected uninitialized memory.
The following changes to your patch (#2 alone) is enough to fix
the issue.

The logic here is still a mess and needs to be cleaned up to avoid
future related bugs.

From bbcc22899566556ced9692e4811aea2a38817834 Mon Sep 17 00:00:00 2001
From: Jiri Pirko <jiri@mellanox.com>
Date: Tue, 23 Jul 2019 13:25:38 +0200
Subject: [PATCH] tc: batch: fix line/line_next processing in batch

When getcmdline fails, there is no valid string in line_next.
So change the flow and don't process it. Alongside with that,
free the previous line buffer and prevent memory leak.

Also, avoid passing uninitialized memory to filters.

Fixes: 485d0c6001c4 ("tc: Add batchsize feature for filter and actions")
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 tc/tc.c | 84 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 43 insertions(+), 41 deletions(-)

diff --git a/tc/tc.c b/tc/tc.c
index 64e342dd85bf..95e5481955ad 100644
--- a/tc/tc.c
+++ b/tc/tc.c
@@ -325,11 +325,10 @@ static int batch(const char *name)
 {
 	struct batch_buf *head = NULL, *tail = NULL, *buf_pool = NULL;
 	char *largv[100], *largv_next[100];
-	char *line, *line_next = NULL;
+	char *line = NULL, *line_next = NULL;
 	bool bs_enabled = false;
 	bool lastline = false;
 	int largc, largc_next;
-	bool bs_enabled_saved;
 	bool bs_enabled_next;
 	int batchsize = 0;
 	size_t len = 0;
@@ -360,47 +359,40 @@ static int batch(const char *name)
 	largc = makeargs(line, largv, 100);
 	bs_enabled = batchsize_enabled(largc, largv);
 	do {
-		if (getcmdline(&line_next, &len, stdin) == -1)
+		if (getcmdline(&line_next, &len, stdin) == -1) {
 			lastline = true;
-
-		largc_next = makeargs(line_next, largv_next, 100);
-		bs_enabled_next = batchsize_enabled(largc_next, largv_next);
-		if (bs_enabled) {
-			struct batch_buf *buf;
-
-			buf = get_batch_buf(&buf_pool, &head, &tail);
-			if (!buf) {
-				fprintf(stderr,
-					"failed to allocate batch_buf\n");
-				return -1;
-			}
-			++batchsize;
-		}
-
-		/*
-		 * In batch mode, if we haven't accumulated enough commands
-		 * and this is not the last command and this command & next
-		 * command both support the batchsize feature, don't send the
-		 * message immediately.
-		 */
-		if (!lastline && bs_enabled && bs_enabled_next
-		    && batchsize != MSG_IOV_MAX)
-			send = false;
-		else
 			send = true;
+		} else {
+			largc_next = makeargs(line_next, largv_next, 100);
+			bs_enabled_next = batchsize_enabled(largc_next, largv_next);
+			if (bs_enabled) {
+				struct batch_buf *buf;
+
+				buf = get_batch_buf(&buf_pool, &head, &tail);
+				if (!buf) {
+					fprintf(stderr,
+						"failed to allocate batch_buf\n");
+					return -1;
+				}
+				++batchsize;
+			}
 
-		line = line_next;
-		line_next = NULL;
-		len = 0;
-		bs_enabled_saved = bs_enabled;
-		bs_enabled = bs_enabled_next;
-
-		if (largc == 0) {
-			largc = largc_next;
-			memcpy(largv, largv_next, largc * sizeof(char *));
-			continue;	/* blank line */
+			/*
+			 * In batch mode, if we haven't accumulated enough
+			 * commands and this is not the last command and this
+			 * command & next command both support the batchsize
+			 * feature, don't send the message immediately.
+			 */
+			if (bs_enabled && bs_enabled_next
+			    && batchsize != MSG_IOV_MAX)
+				send = false;
+			else
+				send = true;
 		}
 
+		if (largc == 0)
+			goto to_next_line;	/* blank line */
+
 		err = do_cmd(largc, largv, tail == NULL ? NULL : tail->buf,
 			     tail == NULL ? 0 : sizeof(tail->buf));
 		fflush(stdout);
@@ -411,10 +403,8 @@ static int batch(const char *name)
 			if (!force)
 				break;
 		}
-		largc = largc_next;
-		memcpy(largv, largv_next, largc * sizeof(char *));
 
-		if (send && bs_enabled_saved) {
+		if (send && bs_enabled) {
 			struct iovec *iov, *iovs;
 			struct batch_buf *buf;
 			struct nlmsghdr *n;
@@ -438,6 +428,18 @@ static int batch(const char *name)
 			}
 			batchsize = 0;
 		}
+
+to_next_line:
+		if (lastline)
+			continue;
+		free(line);
+		line = line_next;
+		line_next = NULL;
+		len = 0;
+		bs_enabled = bs_enabled_next;
+		largc = largc_next;
+		memcpy(largv, largv_next, largc * sizeof(char *));
+
 	} while (!lastline);
 
 	free_batch_bufs(&buf_pool);
-- 
2.20.1

Re: [patch iproute2 1/2] tc: action: fix crash caused by incorrect *argv check

[Jiri Pirko]

Fri, Jul 26, 2019 at 09:47:07PM CEST, stephen@networkplumber.org wrote:
>On Tue, 23 Jul 2019 13:25:37 +0200
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> One cannot depend on *argv being null in case of no arg is left on the
>> command line. For example in batch mode, this is not always true. Check
>> argc instead to prevent crash.
>> 
>> Reported-by: Alex Kushnarov <alexanderk@mellanox.com>
>> Fixes: fd8b3d2c1b9b ("actions: Add support for user cookies")
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>>  tc/m_action.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/tc/m_action.c b/tc/m_action.c
>> index ab6bc0ad28ff..0f9c3a27795d 100644
>> --- a/tc/m_action.c
>> +++ b/tc/m_action.c
>> @@ -222,7 +222,7 @@ done0:
>>  				goto bad_val;
>>  			}
>>  
>> -			if (*argv && strcmp(*argv, "cookie") == 0) {
>> +			if (argc && strcmp(*argv, "cookie") == 0) {
>>  				size_t slen;
>>  
>>  				NEXT_ARG();
>
>
>The logic here is broken at end of file.
>
>	do {
>		if (getcmdline(&line_next, &len, stdin) == -1)
>			lastline = true;
>
>		largc_next = makeargs(line_next, largv_next, 100);
>		bs_enabled_next = batchsize_enabled(largc_next, largv_next);
>		if (bs_enabled) {
>			struct batch_
>
>
>getcmdline() will return -1 at end of file.
>The code will call make_args on an uninitialized pointer.
>
>I see lots of other unnecessary complexity in the whole batch logic.
>It needs to be rewritten.
>
>Rather than me fixing the code, I am probably going to revert.

I agree. This is a mess :(


>
>commit 485d0c6001c4aa134b99c86913d6a7089b7b2ab0
>Author: Chris Mi <chrism@mellanox.com>
>Date:   Fri Jan 12 14:13:16 2018 +0900
>
>    tc: Add batchsize feature for filter and actions