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

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

[Phil Sutter]

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

Changes since v2:
- Rebased onto current master branch.
- Adjusted patches according to feedback.

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   |  3 ++-
 tc/tc_filter.c |  3 +++
 tipc/bearer.c  |  2 +-
 5 files changed, 30 insertions(+), 12 deletions(-)

-- 
2.13.1

[iproute PATCH v3 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 v3 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>
---
Changes since v2:
- Call abort() if 'p' becomes NULL.
---
 misc/nstat.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/misc/nstat.c b/misc/nstat.c
index a4dd405d43a93..56e9367e99736 100644
--- a/misc/nstat.c
+++ b/misc/nstat.c
@@ -217,6 +217,8 @@ static void load_ugly_table(FILE *fp)
 			n->next = db;
 			db = n;
 			p = next;
+			if (!p)
+				abort();
 		}
 		n = db;
 		if (fgets(buf, sizeof(buf), fp) == NULL)
-- 
2.13.1

[iproute PATCH v3 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>
---
Changes since v2:
- Dropped empty line between assignment and check.
---
 tc/q_netem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tc/q_netem.c b/tc/q_netem.c
index 0975ae111de97..5a9e747411e85 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,7 @@ 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 v3 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>
---
Changes since v2:
- Instead of calling strlen(), just make sure **argv is not 0.
---
 tc/tc_filter.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tc/tc_filter.c b/tc/tc_filter.c
index b13fb9185d4fd..cf290ae8e252c 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 (!**argv)
+				invarg("invalid filter name", *argv);
+
 			strncpy(k, *argv, sizeof(k)-1);
 
 			q = get_filter_kind(k);
-- 
2.13.1

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

[Phil Sutter]

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
Changes since v2:
- Keep assignment and check in separate statements.
---
 tipc/bearer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[David Laight]

From: Phil Sutter
> Sent: 21 August 2017 11:03
> To: Stephen Hemminger
> Cc: netdev@vger.kernel.org
> Subject: [iproute PATCH v3 4/5] tc/tc_filter: Make sure filter name is not empty
> 
> 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>
> ---
> Changes since v2:
> - Instead of calling strlen(), just make sure **argv is not 0.
> ---
>  tc/tc_filter.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tc/tc_filter.c b/tc/tc_filter.c
> index b13fb9185d4fd..cf290ae8e252c 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 (!**argv)
> +				invarg("invalid filter name", *argv);
> +

The error message won't make sense...

	David

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

[Phil Sutter]

On Mon, Aug 21, 2017 at 02:53:46PM +0000, David Laight wrote:
> From: Phil Sutter
> > Sent: 21 August 2017 11:03
> > To: Stephen Hemminger
> > Cc: netdev@vger.kernel.org
> > Subject: [iproute PATCH v3 4/5] tc/tc_filter: Make sure filter name is not empty
> > 
> > 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>
> > ---
> > Changes since v2:
> > - Instead of calling strlen(), just make sure **argv is not 0.
> > ---
> >  tc/tc_filter.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tc/tc_filter.c b/tc/tc_filter.c
> > index b13fb9185d4fd..cf290ae8e252c 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 (!**argv)
> > +				invarg("invalid filter name", *argv);
> > +
> 
> The error message won't make sense...

Hmm. It would print:

| Error: argument "" is wrong: invalid filter name

Since this parameter is not introduced by a previous one, I don't think
the error message could be made more precise. Do you have a better
suggestion?

Thanks, Phil

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

[Stephen Hemminger]

On Mon, 21 Aug 2017 12:03:05 +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>
> ---
> Changes since v2:
> - Call abort() if 'p' becomes NULL.
> ---
>  misc/nstat.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/misc/nstat.c b/misc/nstat.c
> index a4dd405d43a93..56e9367e99736 100644
> --- a/misc/nstat.c
> +++ b/misc/nstat.c
> @@ -217,6 +217,8 @@ static void load_ugly_table(FILE *fp)
>  			n->next = db;
>  			db = n;
>  			p = next;
> +			if (!p)
> +				abort();
>  		}
>  		n = db;
>  		if (fgets(buf, sizeof(buf), fp) == NULL)

This doesn't do anything better than just dereferencing NULL.
In either case program crashes with no useful information to user.
Not applying this.