[iproute PATCH v4 0/4] Covscan: Fix potential NULL pointer dereferences

[iproute PATCH v4 0/4] Covscan: Fix potential NULL pointer dereferences

[Phil Sutter]

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

Changes since v3:
- Dropped upstream rejected patch 2.

Phil Sutter (4):
  ifstat, nstat: Check fdopen() return value
  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   | 16 +++++++++++-----
 tc/q_netem.c   |  3 ++-
 tc/tc_filter.c |  3 +++
 tipc/bearer.c  |  2 +-
 5 files changed, 28 insertions(+), 12 deletions(-)

-- 
2.13.1

[iproute PATCH v4 1/4] 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 v4 2/4] 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 v4 3/4] 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 v4 4/4] 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 v4 0/4] Covscan: Fix potential NULL pointer dereferences

[Stephen Hemminger]

On Thu, 24 Aug 2017 11:46:30 +0200
Phil Sutter <phil@nwl.cc> wrote:

> This series collects patches from v1 which eliminate possible cases of
> NULL pointer dereferences.
> 
> Changes since v3:
> - Dropped upstream rejected patch 2.
> 
> Phil Sutter (4):
>   ifstat, nstat: Check fdopen() return value
>   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   | 16 +++++++++++-----
>  tc/q_netem.c   |  3 ++-
>  tc/tc_filter.c |  3 +++
>  tipc/bearer.c  |  2 +-
>  5 files changed, 28 insertions(+), 12 deletions(-)
> 

Applied. Thanks