Re: [iproute PATCH v2 1/7] ipntable: Make sure filter.name is NULL-terminated

On Fri, Aug 18, 2017 at 04:32:47PM +0000, David Laight wrote:

From: Phil Sutter

quoted

Sent: 18 August 2017 11:52
On Fri, Aug 18, 2017 at 09:19:16AM +0000, David Laight wrote:

quoted

From: Phil Sutter

quoted

Sent: 17 August 2017 18:09
To: Stephen Hemminger
Cc: <redacted>
Subject: [iproute PATCH v2 1/7] ipntable: Make sure filter.name is NULL-terminated

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 ip/ipntable.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ip/ipntable.c b/ip/ipntable.c
index 879626ee4f491..7be1f04d33d90 100644
--- a/ip/ipntable.c
+++ b/ip/ipntable.c

@@ -633,7 +633,8 @@ static int ipntable_show(int argc, char **argv)
 		} else if (strcmp(*argv, "name") == 0) {
 			NEXT_ARG();

-			strncpy(filter.name, *argv, sizeof(filter.name));
+			strncpy(filter.name, *argv, sizeof(filter.name) - 1);
+			filter.name[sizeof(filter.name) - 1] = '\0';

Why not check for overflow instead?
			if (filter.name[sizeof(filter.name) - 1])
				usage("filer name too long");

sizeof(filter.name) is 1024, which is maybe a bit over the top for
something a user would input. So I found a better way avoiding all this
at once: I made filter.name a const char *, then just assigned *argv to
it. This should be safe since rtnl_dump_filter() and therefore
print_ntable() callback is called from inside ipntable_show() so *argv
is not accessed outside of it's scope.

What do you think?

There isn't a scope problem, *argv is program data (written to unusable
stack space by the kernel during exec.

Ah, thanks for the info!

If the filter is done in userpace it is ok, but I'd have thought
it would be passed to kernel later on?

No, it just calls sends RTM_GETNEIGHTBL to kernel and throws away
anything that doesn't match the filter. So filtering happens in
user space exclusively.

Thanks, Phil