[iproute PATCH v3 0/6] Covscan: Don't access garbage

[iproute PATCH v3 0/6] Covscan: Don't access garbage

[Phil Sutter]

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.

Changes since v2:
- Rebased onto current master branch.
- Dropped first patch since it is not a real issue.

Phil Sutter (6):
  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    |  2 +-
 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, 14 insertions(+), 13 deletions(-)

-- 
2.13.1

[iproute PATCH v3 1/6] ipaddress: Avoid accessing uninitialized variable lcl

[Phil Sutter]

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 4d37c5e045071..c9312f06dbd4d 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

[iproute PATCH v3 2/6] iplink_can: Prevent overstepping array bounds

[Phil Sutter]

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

[iproute PATCH v3 3/6] ipmaddr: Avoid accessing uninitialized data

[Phil Sutter]

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

[iproute PATCH v3 4/6] ss: Use C99 initializer in netlink_show_one()

[Phil Sutter]

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 10360e5a04ff8..63d12871dd826 100644
--- a/misc/ss.c
+++ b/misc/ss.c
@@ -3480,17 +3480,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

[iproute PATCH v3 5/6] netem/maketable: Check return value of fstat()

[Phil Sutter]

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

[iproute PATCH v3 6/6] tc/q_multiq: Don't pass garbage in TCA_OPTIONS

[Phil Sutter]

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

Re: [iproute PATCH v3 0/6] Covscan: Don't access garbage

[Stephen Hemminger]

On Mon, 21 Aug 2017 11:26:58 +0200
Phil Sutter <phil@nwl.cc> wrote:

> 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.
> 
> Changes since v2:
> - Rebased onto current master branch.
> - Dropped first patch since it is not a real issue.
> 
> Phil Sutter (6):
>   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    |  2 +-
>  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, 14 insertions(+), 13 deletions(-)
> 

These look fine. Applied.