[iproute PATCH v2 0/7] Covscan: Don't access garbage

This series collects patches from v1 which resolve situations where
garbage might be read, either due to missing initialization of
variables or accessing data which went out of scope.

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

Phil Sutter (7):
  ipaddress: Make buffer for filter.flushb static
  ipaddress: Avoid accessing uninitialized variable lcl
  iplink_can: Prevent overstepping array bounds
  ipmaddr: Avoid accessing uninitialized data
  ss: Use C99 initializer in netlink_show_one()
  netem/maketable: Check return value of fstat()
  tc/q_multiq: Don't pass garbage in TCA_OPTIONS

 ip/ipaddress.c    |  4 ++--
 ip/iplink_can.c   |  4 ++--
 ip/ipmaddr.c      |  2 +-
 misc/ss.c         | 13 +++++++------
 netem/maketable.c |  4 ++--
 tc/q_multiq.c     |  2 +-
 6 files changed, 15 insertions(+), 14 deletions(-)

-- 
2.13.1

Otherwise info.st_size may contain garbage.

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

diff --git a/netem/maketable.c b/netem/maketable.c
index 6aff927be7040..ad660e7d457f0 100644
--- a/netem/maketable.c
+++ b/netem/maketable.c

@@ -24,8 +24,8 @@ readdoubles(FILE *fp, int *number)
 	int limit;
 	int n=0, i;
 
-	fstat(fileno(fp), &info);
-	if (info.st_size > 0) {
+	if (!fstat(fileno(fp), &info) &&
+	    info.st_size > 0) {
 		limit = 2*info.st_size/sizeof(double);	/* @@ approximate */
 	} else {
 		limit = 10000;

-- 
2.13.1

This has the additional benefit of initializing st.ino to zero which is
used later in is_sctp_assoc() function.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 misc/ss.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/misc/ss.c b/misc/ss.c
index b2a7f069e294c..d767b1103ea81 100644
--- a/misc/ss.c
+++ b/misc/ss.c

@@ -3469,17 +3469,18 @@ static int netlink_show_one(struct filter *f,
 				int rq, int wq,
 				unsigned long long sk, unsigned long long cb)
 {
-	struct sockstat st;
+	struct sockstat st = {
+		.state		= SS_CLOSE,
+		.rq		= rq,
+		.wq		= wq,
+		.local.family	= AF_NETLINK,
+		.remote.family	= AF_NETLINK,
+	};
 
 	SPRINT_BUF(prot_buf) = {};
 	const char *prot_name;
 	char procname[64] = {};
 
-	st.state = SS_CLOSE;
-	st.rq	 = rq;
-	st.wq	 = wq;
-	st.local.family = st.remote.family = AF_NETLINK;
-
 	if (f->f) {
 		st.rport = -1;
 		st.lport = pid;

-- 
2.13.1

multiq_parse_opt() doesn't change 'opt' at all. So at least make sure
it doesn't fill TCA_OPTIONS attribute with garbage from stack.

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

diff --git a/tc/q_multiq.c b/tc/q_multiq.c
index 7823931494563..9c09c9a7748f6 100644
--- a/tc/q_multiq.c
+++ b/tc/q_multiq.c

@@ -43,7 +43,7 @@ static void explain(void)
 static int multiq_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 			    struct nlmsghdr *n)
 {
-	struct tc_multiq_qopt opt;
+	struct tc_multiq_qopt opt = {};
 
 	if (argc) {
 		if (strcmp(*argv, "help") == 0) {

-- 
2.13.1

The buffer is accessed outside of the function defining it, so make it
static.

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

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 4d37c5e045071..3c9decb51b412 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c

@@ -1488,7 +1488,7 @@ static void ipaddr_filter(struct nlmsg_chain *linfo, struct nlmsg_chain *ainfo)
 static int ipaddr_flush(void)
 {
 	int round = 0;
-	char flushb[4096-512];
+	static char flushb[4096-512];
 
 	filter.flushb = flushb;
 	filter.flushp = 0;

-- 
2.13.1

If no address was given, ipaddr_modify() accesses uninitialized data
when assigning to req.ifa.ifa_prefixlen.

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

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 3c9decb51b412..9307c9416dde3 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c

@@ -1888,7 +1888,7 @@ static int ipaddr_modify(int cmd, int flags, int argc, char **argv)
 	char  *lcl_arg = NULL;
 	char  *valid_lftp = NULL;
 	char  *preferred_lftp = NULL;
-	inet_prefix lcl;
+	inet_prefix lcl = {};
 	inet_prefix peer;
 	int local_len = 0;
 	int peer_len = 0;

-- 
2.13.1

Looks like this can only happen if /proc/net/igmp is malformed, but
better be sure.

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

diff --git a/ip/ipmaddr.c b/ip/ipmaddr.c
index 4f726fdd976f1..85a69e779563d 100644
--- a/ip/ipmaddr.c
+++ b/ip/ipmaddr.c

@@ -136,7 +136,7 @@ static void read_igmp(struct ma_info **result_p)
 
 	while (fgets(buf, sizeof(buf), fp)) {
 		struct ma_info *ma;
-		size_t len;
+		size_t len = 0;
 
 		if (buf[0] != '\t') {
 			sscanf(buf, "%d%s", &m.index, m.name);

-- 
2.13.1

can_state_names array contains at most CAN_STATE_MAX fields, so allowing
an index to it to be equal to that number is wrong. While here, also
make sure the array is indeed that big so nothing bad happens if
CAN_STATE_MAX ever increases.

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

diff --git a/ip/iplink_can.c b/ip/iplink_can.c
index 5df56b2bbbb3b..2954010fefa22 100644
--- a/ip/iplink_can.c
+++ b/ip/iplink_can.c

@@ -251,7 +251,7 @@ static int can_parse_opt(struct link_util *lu, int argc, char **argv,
 	return 0;
 }
 
-static const char *can_state_names[] = {
+static const char *can_state_names[CAN_STATE_MAX] = {
 	[CAN_STATE_ERROR_ACTIVE] = "ERROR-ACTIVE",
 	[CAN_STATE_ERROR_WARNING] = "ERROR-WARNING",
 	[CAN_STATE_ERROR_PASSIVE] = "ERROR-PASSIVE",

@@ -275,7 +275,7 @@ static void can_print_opt(struct link_util *lu, FILE *f, struct rtattr *tb[])
 	if (tb[IFLA_CAN_STATE]) {
 		uint32_t state = rta_getattr_u32(tb[IFLA_CAN_STATE]);
 
-		fprintf(f, "state %s ", state <= CAN_STATE_MAX ?
+		fprintf(f, "state %s ", state < CAN_STATE_MAX ?
 			can_state_names[state] : "UNKNOWN");
 	}
 

-- 
2.13.1

On Thu, Aug 17, 2017 at 07:09:25PM +0200, Phil Sutter wrote:

The buffer is accessed outside of the function defining it, so make it
static.

Signed-off-by: Phil Sutter <phil@nwl.cc>

Self-NACK: Access to flushb should be sane since all accessors are
called from ipaddr_flush(). Looking at the covscan output again, it
merely complained about filter.flusb still pointing to the local buffer
when ipaddr_flush() returns.

Cheers, Phil

On Fri, 18 Aug 2017 13:57:03 +0200
Phil Sutter [off-list ref] wrote:

On Thu, Aug 17, 2017 at 07:09:25PM +0200, Phil Sutter wrote:

quoted

The buffer is accessed outside of the function defining it, so make it
static.

Signed-off-by: Phil Sutter <phil@nwl.cc>  

Self-NACK: Access to flushb should be sane since all accessors are
called from ipaddr_flush(). Looking at the covscan output again, it
merely complained about filter.flusb still pointing to the local buffer
when ipaddr_flush() returns.

Cheers, Phil

Ok, dropping this  sub-series