[iproute PATCH v2 0/5] Covscan: Fix potential NULL pointer dereferences

[iproute PATCH v2 0/5] Covscan: Fix potential NULL pointer dereferences

[Phil Sutter]

This series collects patches from v1 which eliminate possible cases of
NULL pointer dereferences.

No changes to the actual patches, just splitting into smaller series.

Phil Sutter (5):
  ifstat, nstat: Check fdopen() return value
  nstat: Fix for potential NULL pointer dereference
  tc/q_netem: Don't dereference possibly NULL pointer
  tc/tc_filter: Make sure filter name is not empty
  tipc/bearer: Prevent NULL pointer dereference

 misc/ifstat.c  | 16 +++++++++++-----
 misc/nstat.c   | 18 ++++++++++++------
 tc/q_netem.c   |  4 +++-
 tc/tc_filter.c |  3 +++
 tipc/bearer.c  |  4 ++--
 5 files changed, 31 insertions(+), 14 deletions(-)

-- 
2.13.1

[iproute PATCH v2 1/5] ifstat, nstat: Check fdopen() return value

[Phil Sutter]

Prevent passing NULL FILE pointer to fgets() later.

Fix both tools in a single patch since the code changes are basically
identical.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ifstat.c | 16 +++++++++++-----
 misc/nstat.c  | 16 +++++++++++-----
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/misc/ifstat.c b/misc/ifstat.c
index 1be21703bf14c..ac3eff6b870a9 100644
--- a/misc/ifstat.c
+++ b/misc/ifstat.c
@@ -992,12 +992,18 @@ int main(int argc, char *argv[])
 	    && verify_forging(fd) == 0) {
 		FILE *sfp = fdopen(fd, "r");
 
-		load_raw_table(sfp);
-		if (hist_db && source_mismatch) {
-			fprintf(stderr, "ifstat: history is stale, ignoring it.\n");
-			hist_db = NULL;
+		if (!sfp) {
+			fprintf(stderr, "ifstat: fdopen failed: %s\n",
+				strerror(errno));
+			close(fd);
+		} else  {
+			load_raw_table(sfp);
+			if (hist_db && source_mismatch) {
+				fprintf(stderr, "ifstat: history is stale, ignoring it.\n");
+				hist_db = NULL;
+			}
+			fclose(sfp);
 		}
-		fclose(sfp);
 	} else {
 		if (fd >= 0)
 			close(fd);
diff --git a/misc/nstat.c b/misc/nstat.c
index 1212b1f2c8128..a4dd405d43a93 100644
--- a/misc/nstat.c
+++ b/misc/nstat.c
@@ -706,12 +706,18 @@ int main(int argc, char *argv[])
 	    && verify_forging(fd) == 0) {
 		FILE *sfp = fdopen(fd, "r");
 
-		load_good_table(sfp);
-		if (hist_db && source_mismatch) {
-			fprintf(stderr, "nstat: history is stale, ignoring it.\n");
-			hist_db = NULL;
+		if (!sfp) {
+			fprintf(stderr, "nstat: fdopen failed: %s\n",
+				strerror(errno));
+			close(fd);
+		} else {
+			load_good_table(sfp);
+			if (hist_db && source_mismatch) {
+				fprintf(stderr, "nstat: history is stale, ignoring it.\n");
+				hist_db = NULL;
+			}
+			fclose(sfp);
 		}
-		fclose(sfp);
 	} else {
 		if (fd >= 0)
 			close(fd);
-- 
2.13.1

[iproute PATCH v2 2/5] nstat: Fix for potential NULL pointer dereference

[Phil Sutter]

If the string at 'p' contains neither space not newline, 'p' will become
NULL. Make sure this isn't the case before dereferencing it.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/nstat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/misc/nstat.c b/misc/nstat.c
index a4dd405d43a93..23e1569d7872b 100644
--- a/misc/nstat.c
+++ b/misc/nstat.c
@@ -198,7 +198,7 @@ static void load_ugly_table(FILE *fp)
 		off = p - buf;
 		p += 2;
 
-		while (*p) {
+		while (p && *p) {
 			char *next;
 
 			if ((next = strchr(p, ' ')) != NULL)
-- 
2.13.1

[iproute PATCH v2 3/5] tc/q_netem: Don't dereference possibly NULL pointer

[Phil Sutter]

Assuming 'opt' might be NULL, move the call to RTA_PAYLOAD to after the
check since it dereferences its parameter.

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

diff --git a/tc/q_netem.c b/tc/q_netem.c
index 0975ae111de97..7e3330512041a 100644
--- a/tc/q_netem.c
+++ b/tc/q_netem.c
@@ -538,7 +538,7 @@ static int netem_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	int *ecn = NULL;
 	struct tc_netem_qopt qopt;
 	const struct tc_netem_rate *rate = NULL;
-	int len = RTA_PAYLOAD(opt) - sizeof(qopt);
+	int len;
 	__u64 rate64 = 0;
 
 	SPRINT_BUF(b1);
@@ -546,6 +546,8 @@ static int netem_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	if (opt == NULL)
 		return 0;
 
+	len = RTA_PAYLOAD(opt) - sizeof(qopt);
+
 	if (len < 0) {
 		fprintf(stderr, "options size error\n");
 		return -1;
-- 
2.13.1

[iproute PATCH v2 4/5] tc/tc_filter: Make sure filter name is not empty

[Phil Sutter]

The later check for 'k[0] != 0' requires a non-empty filter name,
otherwise NULL pointer dereference in 'q' might happen.

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

diff --git a/tc/tc_filter.c b/tc/tc_filter.c
index b13fb9185d4fd..a799edb35886d 100644
--- a/tc/tc_filter.c
+++ b/tc/tc_filter.c
@@ -412,6 +412,9 @@ static int tc_filter_get(int cmd, unsigned int flags, int argc, char **argv)
 			usage();
 			return 0;
 		} else {
+			if (!strlen(*argv))
+				invarg("invalid filter name", *argv);
+
 			strncpy(k, *argv, sizeof(k)-1);
 
 			q = get_filter_kind(k);
-- 
2.13.1

[iproute PATCH v2 5/5] tipc/bearer: Prevent NULL pointer dereference

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tipc/bearer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tipc/bearer.c b/tipc/bearer.c
index c3d4491f8f6ef..0598328ab1f1b 100644
--- a/tipc/bearer.c
+++ b/tipc/bearer.c
@@ -438,8 +438,8 @@ static int cmd_bearer_enable(struct nlmsghdr *nlh, const struct cmd *cmd,
 	if (err)
 		return err;
 
-	opt = get_opt(opts, "media");
-	if (strcmp(opt->val, "udp") == 0) {
+	if ((opt = get_opt(opts, "media")) &&
+	    strcmp(opt->val, "udp") == 0) {
 		err = nl_add_udp_enable_opts(nlh, opts, cmdl);
 		if (err)
 			return err;
-- 
2.13.1

RE: [iproute PATCH v2 4/5] tc/tc_filter: Make sure filter name is not empty

[David Laight]

From: Phil Sutter
> Sent: 17 August 2017 18:10
> The later check for 'k[0] != 0' requires a non-empty filter name,
> otherwise NULL pointer dereference in 'q' might happen.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  tc/tc_filter.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tc/tc_filter.c b/tc/tc_filter.c
> index b13fb9185d4fd..a799edb35886d 100644
> --- a/tc/tc_filter.c
> +++ b/tc/tc_filter.c
> @@ -412,6 +412,9 @@ static int tc_filter_get(int cmd, unsigned int flags, int argc, char **argv)
>  			usage();
>  			return 0;
>  		} else {
> +			if (!strlen(*argv))
> +				invarg("invalid filter name", *argv);

That is nearly as bad as:
	p[strlen(p)] = 0;

> +
>  			strncpy(k, *argv, sizeof(k)-1);
> 
>  			q = get_filter_kind(k);
> --
> 2.13.1

Re: [iproute PATCH v2 4/5] tc/tc_filter: Make sure filter name is not empty

[Phil Sutter]

On Fri, Aug 18, 2017 at 09:30:35AM +0000, David Laight wrote:
> From: Phil Sutter
> > Sent: 17 August 2017 18:10
> > The later check for 'k[0] != 0' requires a non-empty filter name,
> > otherwise NULL pointer dereference in 'q' might happen.
> > 
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  tc/tc_filter.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tc/tc_filter.c b/tc/tc_filter.c
> > index b13fb9185d4fd..a799edb35886d 100644
> > --- a/tc/tc_filter.c
> > +++ b/tc/tc_filter.c
> > @@ -412,6 +412,9 @@ static int tc_filter_get(int cmd, unsigned int flags, int argc, char **argv)
> >  			usage();
> >  			return 0;
> >  		} else {
> > +			if (!strlen(*argv))
> > +				invarg("invalid filter name", *argv);
> 
> That is nearly as bad as:
> 	p[strlen(p)] = 0;

Hey, it's not impossible! I could call tc like so:

| # tc filter get protocol ip prio 1 ""

What's funny about it is that the first call to matches() in
tc_filter_get() will catch the empty last parameter:

| # tc filter get prio 1 protocol ip ""
| Command line is not complete. Try option "help"
| # ./tc/tc filter get prio 1 protocol ip "" ""
| Error: duplicate "priority": "" is the second value.

Cheers, Phil

Re: [iproute PATCH v2 5/5] tipc/bearer: Prevent NULL pointer dereference

[Stephen Hemminger]

On Thu, 17 Aug 2017 19:09:31 +0200
Phil Sutter <phil@nwl.cc> wrote:

> -	opt = get_opt(opts, "media");
> -	if (strcmp(opt->val, "udp") == 0) {
> +	if ((opt = get_opt(opts, "media")) &&
> +	    strcmp(opt->val, "udp") == 0) {

Please don't merge assignment and comparison unless necessary for other reasons.

	opt = get_opt(opts, "media");
	if (opt && strcmp(opt->val, "udp") == 0) {

Re: [iproute PATCH v2 3/5] tc/q_netem: Don't dereference possibly NULL pointer

[Stephen Hemminger]

On Thu, 17 Aug 2017 19:09:29 +0200
Phil Sutter <phil@nwl.cc> wrote:

> @@ -546,6 +546,8 @@ static int netem_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
>  	if (opt == NULL)
>  		return 0;
>  
> +	len = RTA_PAYLOAD(opt) - sizeof(qopt);
> +
>  	if (len < 0) {

Dont add blank line between computation and conditional.
Having them together reads better.

Re: [iproute PATCH v2 2/5] nstat: Fix for potential NULL pointer dereference

[Stephen Hemminger]

On Thu, 17 Aug 2017 19:09:28 +0200
Phil Sutter <phil@nwl.cc> wrote:

> If the string at 'p' contains neither space not newline, 'p' will become
> NULL. Make sure this isn't the case before dereferencing it.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Why not fix the parsing code instead. Other places here call abort().

RE: [iproute PATCH v2 4/5] tc/tc_filter: Make sure filter name is not empty

[David Laight]

From: Phil Sutter
> Sent: 18 August 2017 12:16
> On Fri, Aug 18, 2017 at 09:30:35AM +0000, David Laight wrote:
> > From: Phil Sutter
> > > Sent: 17 August 2017 18:10
> > > The later check for 'k[0] != 0' requires a non-empty filter name,
> > > otherwise NULL pointer dereference in 'q' might happen.
> > >
> > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > ---
> > >  tc/tc_filter.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/tc/tc_filter.c b/tc/tc_filter.c
> > > index b13fb9185d4fd..a799edb35886d 100644
> > > --- a/tc/tc_filter.c
> > > +++ b/tc/tc_filter.c
> > > @@ -412,6 +412,9 @@ static int tc_filter_get(int cmd, unsigned int flags, int argc, char **argv)
> > >  			usage();
> > >  			return 0;
> > >  		} else {
> > > +			if (!strlen(*argv))
> > > +				invarg("invalid filter name", *argv);
> >
> > That is nearly as bad as:
> > 	p[strlen(p)] = 0;
> 
> Hey, it's not impossible! I could call tc like so:
> 
> | # tc filter get protocol ip prio 1 ""

You missed the point. Just check **argv there is no need to
determine the length just to check it is non-zero.

	David

Re: [iproute PATCH v2 4/5] tc/tc_filter: Make sure filter name is not empty

[Phil Sutter]

On Fri, Aug 18, 2017 at 04:34:44PM +0000, David Laight wrote:
> From: Phil Sutter
> > Sent: 18 August 2017 12:16
> > On Fri, Aug 18, 2017 at 09:30:35AM +0000, David Laight wrote:
> > > From: Phil Sutter
> > > > Sent: 17 August 2017 18:10
> > > > The later check for 'k[0] != 0' requires a non-empty filter name,
> > > > otherwise NULL pointer dereference in 'q' might happen.
> > > >
> > > > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > > > ---
> > > >  tc/tc_filter.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/tc/tc_filter.c b/tc/tc_filter.c
> > > > index b13fb9185d4fd..a799edb35886d 100644
> > > > --- a/tc/tc_filter.c
> > > > +++ b/tc/tc_filter.c
> > > > @@ -412,6 +412,9 @@ static int tc_filter_get(int cmd, unsigned int flags, int argc, char **argv)
> > > >  			usage();
> > > >  			return 0;
> > > >  		} else {
> > > > +			if (!strlen(*argv))
> > > > +				invarg("invalid filter name", *argv);
> > >
> > > That is nearly as bad as:
> > > 	p[strlen(p)] = 0;
> > 
> > Hey, it's not impossible! I could call tc like so:
> > 
> > | # tc filter get protocol ip prio 1 ""
> 
> You missed the point. Just check **argv there is no need to
> determine the length just to check it is non-zero.

Oh, hehe. Thanks for the short-cut!

Thanks, PHil