From d044ea3e784d1a4f0a61f306b86ce95c9a26b0b5 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 21 Aug 2017 11:26:59 +0200 Subject: [PATCH 01/10] ipaddress: Avoid accessing uninitialized variable lcl If no address was given, ipaddr_modify() accesses uninitialized data when assigning to req.ifa.ifa_prefixlen. Signed-off-by: Phil Sutter --- ip/ipaddress.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ip/ipaddress.c b/ip/ipaddress.c index 4d37c5e0..c9312f06 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; From 258b7c0fa70c2d6b5f9776cc35c38c80b4ee5752 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 21 Aug 2017 11:27:00 +0200 Subject: [PATCH 02/10] iplink_can: Prevent overstepping array bounds 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 --- 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 5df56b2b..2954010f 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"); } From b48a1161f5f9b6a0cda399a224bbbf72eba4a5c6 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 21 Aug 2017 11:27:01 +0200 Subject: [PATCH 03/10] ipmaddr: Avoid accessing uninitialized data Looks like this can only happen if /proc/net/igmp is malformed, but better be sure. Signed-off-by: Phil Sutter --- ip/ipmaddr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ip/ipmaddr.c b/ip/ipmaddr.c index 4f726fdd..85a69e77 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); From 301826beb3baa902e2057d81912d1586459f605f Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 21 Aug 2017 11:27:02 +0200 Subject: [PATCH 04/10] ss: Use C99 initializer in netlink_show_one() 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 --- misc/ss.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/misc/ss.c b/misc/ss.c index bd68ecdc..34c6da54 100644 --- a/misc/ss.c +++ b/misc/ss.c @@ -3479,17 +3479,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; From d304b05c12b3a0247b627ebc8e4477520bb4b969 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 21 Aug 2017 11:27:03 +0200 Subject: [PATCH 05/10] netem/maketable: Check return value of fstat() Otherwise info.st_size may contain garbage. Signed-off-by: Phil Sutter --- netem/maketable.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/netem/maketable.c b/netem/maketable.c index 6aff927b..ad660e7d 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; From 82ed9ffa2bb86eea653f68a0ade945b7708818c9 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 21 Aug 2017 11:27:04 +0200 Subject: [PATCH 06/10] tc/q_multiq: Don't pass garbage in TCA_OPTIONS 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 --- 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 78239314..9c09c9a7 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) { From 7c66d89828a6ee4c5a4e3f48ef4a4cb07b50013d Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 21 Aug 2017 18:36:50 +0200 Subject: [PATCH 07/10] iproute: Check mark value input Signed-off-by: Phil Sutter --- ip/iproute.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ip/iproute.c b/ip/iproute.c index 89caac12..83fd70c3 100644 --- a/ip/iproute.c +++ b/ip/iproute.c @@ -1495,7 +1495,8 @@ static int iproute_list_flush_or_save(int argc, char **argv, int action) id = *argv; } else if (strcmp(*argv, "mark") == 0) { NEXT_ARG(); - get_unsigned(&mark, *argv, 0); + if (get_unsigned(&mark, *argv, 0)) + invarg("invalid mark value", *argv); filter.markmask = -1; } else if (strcmp(*argv, "via") == 0) { int family; @@ -1712,7 +1713,8 @@ static int iproute_get(int argc, char **argv) idev = *argv; } else if (matches(*argv, "mark") == 0) { NEXT_ARG(); - get_unsigned(&mark, *argv, 0); + if (get_unsigned(&mark, *argv, 0)) + invarg("invalid mark value", *argv); } else if (matches(*argv, "oif") == 0 || strcmp(*argv, "dev") == 0) { NEXT_ARG(); From 84b6a3f4b5720aaf673c2eaad2cf60f786de077b Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 21 Aug 2017 18:36:51 +0200 Subject: [PATCH 08/10] iplink_vrf: Complain if main table is not found Signed-off-by: Phil Sutter Acked-by: David Ahern --- ip/iplink_vrf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ip/iplink_vrf.c b/ip/iplink_vrf.c index 917630e8..2b85a3a5 100644 --- a/ip/iplink_vrf.c +++ b/ip/iplink_vrf.c @@ -131,7 +131,9 @@ __u32 ipvrf_get_table(const char *name) &answer.n, sizeof(answer)) < 0) { /* special case "default" vrf to be the main table */ if (errno == ENODEV && !strcmp(name, "default")) - rtnl_rttable_a2n(&tb_id, "main"); + if (rtnl_rttable_a2n(&tb_id, "main")) + fprintf(stderr, + "BUG: RTTable \"main\" not found.\n"); return tb_id; } From 6e33f7b0f6e04dd46bea24c3ab28d61e54625dd7 Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 21 Aug 2017 18:36:52 +0200 Subject: [PATCH 09/10] devlink: Check return code of strslashrsplit() This function shouldn't fail because all callers of __dl_argv_handle_port() make sure the passed string contains enough slashes already, but better make sure if this changes in future the function won't access uninitialized data. Signed-off-by: Phil Sutter --- devlink/devlink.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/devlink/devlink.c b/devlink/devlink.c index 5b325ce6..8f11f865 100644 --- a/devlink/devlink.c +++ b/devlink/devlink.c @@ -526,18 +526,26 @@ static int __dl_argv_handle_port(char *str, char **p_bus_name, char **p_dev_name, uint32_t *p_port_index) { - char *handlestr = handlestr; - char *portstr = portstr; + char *handlestr; + char *portstr; int err; - strslashrsplit(str, &handlestr, &portstr); + err = strslashrsplit(str, &handlestr, &portstr); + if (err) { + pr_err("Port identification \"%s\" is invalid\n", str); + return err; + } err = strtouint32_t(portstr, p_port_index); if (err) { pr_err("Port index \"%s\" is not a number or not within range\n", portstr); return err; } - strslashrsplit(handlestr, p_bus_name, p_dev_name); + err = strslashrsplit(handlestr, p_bus_name, p_dev_name); + if (err) { + pr_err("Port identification \"%s\" is invalid\n", str); + return err; + } return 0; } From c3724e4bc3a6c40dc846f0c3b02934d711bf81fb Mon Sep 17 00:00:00 2001 From: Phil Sutter Date: Mon, 21 Aug 2017 16:46:51 +0200 Subject: [PATCH 10/10] lib/bpf: Don't leak fp in bpf_find_mntpt() If fopen() succeeded but len != PATH_MAX, the function leaks the open FILE pointer. Fix this by checking len value before calling fopen(). Signed-off-by: Phil Sutter Acked-by: Daniel Borkmann --- lib/bpf.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/bpf.c b/lib/bpf.c index 4f52ad4a..1dcb261d 100644 --- a/lib/bpf.c +++ b/lib/bpf.c @@ -537,8 +537,11 @@ static const char *bpf_find_mntpt(const char *fstype, unsigned long magic, } } + if (len != PATH_MAX) + return NULL; + fp = fopen("/proc/mounts", "r"); - if (fp == NULL || len != PATH_MAX) + if (fp == NULL) return NULL; while (fscanf(fp, "%*s %" textify(PATH_MAX) "s %99s %*s %*d %*d\n",