From 1a563a0cbd4926d0dfe9065a4fcd8771c5b292cc Mon Sep 17 00:00:00 2001 From: Stefano Brivio Date: Tue, 19 Oct 2021 17:28:18 +0200 Subject: [PATCH] passt: Address gcc 11 warnings A mix of unchecked return values, a missing permission mask for open(2) with O_CREAT, and some false positives from -Wstringop-overflow and -Wmaybe-uninitialized. Reported-by: Martin Hauke Signed-off-by: Stefano Brivio --- Makefile | 14 ++++++++++++++ passt.c | 13 +++++++++---- pasta.c | 15 ++++++++++----- pcap.c | 35 +++++++++++++++++++++++++---------- siphash.c | 3 +++ tcp.c | 36 +++++++++++++++++++++++++----------- udp.c | 8 +++++--- 7 files changed, 91 insertions(+), 33 deletions(-) diff --git a/Makefile b/Makefile index c2077f2..6d11e22 100644 --- a/Makefile +++ b/Makefile @@ -15,6 +15,20 @@ CFLAGS += -DPAGE_SIZE=$(shell getconf PAGE_SIZE) CFLAGS += -DNETNS_RUN_DIR=\"/run/netns\" CFLAGS += -DPASST_AUDIT_ARCH=AUDIT_ARCH_$(shell uname -m | tr [a-z] [A-Z]) +# On gcc 11.2, with -O2 and -flto, tcp_hash() and siphash_20b(), if inlined, +# seem to be hitting something similar to: +# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78993 +# from the pointer arithmetic used from the tcp_tap_handler() path to get the +# remote connection address. +ifeq ($(shell $(CC) -dumpversion),11) +ifneq (,$(filter -flto%,$(CFLAGS))) +ifneq (,$(filter -O2,$(CFLAGS))) + CFLAGS += -DTCP_HASH_NOINLINE + CFLAGS += -DSIPHASH_20B_NOINLINE +endif +endif +endif + prefix ?= /usr/local all: passt pasta passt4netns qrap diff --git a/passt.c b/passt.c index a5f9bf8..846ba7f 100644 --- a/passt.c +++ b/passt.c @@ -257,13 +257,16 @@ static void pid_file(struct ctx *c) { if (!*c->pid_file) return; - pid_fd = open(c->pid_file, O_CREAT | O_WRONLY); + pid_fd = open(c->pid_file, O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR); if (pid_fd < 0) return; n = snprintf(pid_buf, sizeof(pid_buf), "%i\n", getpid()); - write(pid_fd, pid_buf, n); + if (write(pid_fd, pid_buf, n) < 0) { + perror("PID file write"); + exit(EXIT_FAILURE); + } close(pid_fd); } @@ -365,8 +368,10 @@ int main(int argc, char **argv) else __setlogmask(LOG_UPTO(LOG_INFO)); - if (isatty(fileno(stdout)) && !c.foreground) - daemon(0, 0); + if (isatty(fileno(stdout)) && !c.foreground && daemon(0, 0)) { + perror("daemon"); + exit(EXIT_FAILURE); + } pid_file(&c); diff --git a/pasta.c b/pasta.c index fe0bf0e..395f459 100644 --- a/pasta.c +++ b/pasta.c @@ -184,7 +184,8 @@ void pasta_start_ns(struct ctx *c) snprintf(proc_path, PATH_MAX, "/proc/%i/ns/net", pasta_child_pid); - readlink(proc_path, pasta_child_ns, PATH_MAX); + if (readlink(proc_path, pasta_child_ns, PATH_MAX) < 0) + warn("Cannot read link to ns, won't clean up on exit"); return; } @@ -198,20 +199,24 @@ void pasta_start_ns(struct ctx *c) snprintf(buf, BUFSIZ, "%u %u %u", 0, euid, 1); fd = open("/proc/self/uid_map", O_WRONLY); - write(fd, buf, strlen(buf)); + if (write(fd, buf, strlen(buf)) < 0) + warn("Cannot set uid_map in namespace"); close(fd); fd = open("/proc/self/setgroups", O_WRONLY); - write(fd, "deny", sizeof("deny")); + if (write(fd, "deny", sizeof("deny"))) + warn("Cannot write to setgroups in namespace"); close(fd); fd = open("/proc/self/gid_map", O_WRONLY); - write(fd, buf, strlen(buf)); + if (write(fd, buf, strlen(buf)) < 0) + warn("Cannot set gid_map in namespace"); close(fd); } fd = open("/proc/sys/net/ipv4/ping_group_range", O_WRONLY); - write(fd, "0 0", strlen("0 0")); + if (write(fd, "0 0", strlen("0 0")) < 0) + warn("Cannot set ping_group_range, ICMP requests might fail"); close(fd); shell = getenv("SHELL") ? getenv("SHELL") : "/bin/sh"; diff --git a/pcap.c b/pcap.c index 9cf585f..8ff1636 100644 --- a/pcap.c +++ b/pcap.c @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -87,8 +88,8 @@ void pcap(char *pkt, size_t len) h.tv_usec = tv.tv_usec; h.caplen = h.len = len; - write(pcap_fd, &h, sizeof(h)); - write(pcap_fd, pkt, len); + if (write(pcap_fd, &h, sizeof(h)) < 0 || write(pcap_fd, pkt, len) < 0) + debug("Cannot log packet, length %u", len); } /** @@ -98,6 +99,7 @@ void pcap(char *pkt, size_t len) void pcapm(struct msghdr *mh) { struct pcap_pkthdr h; + struct iovec *iov; struct timeval tv; unsigned int i; @@ -109,13 +111,19 @@ void pcapm(struct msghdr *mh) h.tv_usec = tv.tv_usec; for (i = 0; i < mh->msg_iovlen; i++) { - struct iovec *iov = &mh->msg_iov[i]; + iov = &mh->msg_iov[i]; h.caplen = h.len = iov->iov_len - 4; - write(pcap_fd, &h, sizeof(h)); - write(pcap_fd, (char *)iov->iov_base + 4, iov->iov_len - 4); + if (write(pcap_fd, &h, sizeof(h)) < 0) + goto fail; + if (write(pcap_fd, (char *)iov->iov_base + 4, iov->iov_len - 4)) + goto fail; } + + return; +fail: + debug("Cannot log packet, length %u", iov->iov_len - 4); } /** @@ -125,6 +133,7 @@ void pcapm(struct msghdr *mh) void pcapmm(struct mmsghdr *mmh, unsigned int vlen) { struct pcap_pkthdr h; + struct iovec *iov; struct timeval tv; unsigned int i, j; @@ -139,15 +148,20 @@ void pcapmm(struct mmsghdr *mmh, unsigned int vlen) struct msghdr *mh = &mmh[i].msg_hdr; for (j = 0; j < mh->msg_iovlen; j++) { - struct iovec *iov = &mh->msg_iov[j]; + iov = &mh->msg_iov[j]; h.caplen = h.len = iov->iov_len - 4; - write(pcap_fd, &h, sizeof(h)); - write(pcap_fd, (char *)iov->iov_base + 4, - iov->iov_len - 4); + if (write(pcap_fd, &h, sizeof(h)) < 0) + goto fail; + if (write(pcap_fd, (char *)iov->iov_base + 4, + iov->iov_len - 4) < 0) + goto fail; } } + return; +fail: + debug("Cannot log packet, length %u", iov->iov_len - 4); } /** @@ -194,5 +208,6 @@ void pcap_init(struct ctx *c, int index) info("Saving packet capture at %s", c->pcap); - write(pcap_fd, &pcap_hdr, sizeof(pcap_hdr)); + if (write(pcap_fd, &pcap_hdr, sizeof(pcap_hdr)) < 0) + warn("Cannot write PCAP header: %s", strerror(errno)); } diff --git a/siphash.c b/siphash.c index b7a7861..88f8bd2 100644 --- a/siphash.c +++ b/siphash.c @@ -146,6 +146,9 @@ uint32_t siphash_12b(const uint8_t *in, const uint64_t *k) * * Return: the 64-bit hash output */ +#if SIPHASH_20B_NOINLINE +__attribute__((__noinline__)) /* See comment in Makefile */ +#endif uint64_t siphash_20b(const uint8_t *in, const uint64_t *k) { uint32_t *in32 = (uint32_t *)in; diff --git a/tcp.c b/tcp.c index 0093ec4..99674ef 100644 --- a/tcp.c +++ b/tcp.c @@ -311,6 +311,7 @@ #include #include #include +#include #include #include #include @@ -1103,6 +1104,9 @@ static int tcp_hash_match(struct tcp_tap_conn *conn, int af, void *addr, * * Return: hash value, already modulo size of the hash table */ +#if TCP_HASH_NOINLINE +__attribute__((__noinline__)) /* See comment in Makefile */ +#endif static unsigned int tcp_hash(struct ctx *c, int af, void *addr, in_port_t tap_port, in_port_t sock_port) { @@ -1322,8 +1326,9 @@ static void tcp_l2_flags_buf_flush(struct ctx *c) for (i = 0; i < mh.msg_iovlen; i++) { struct iovec *iov = &mh.msg_iov[i]; - write(c->fd_tap, (char *)iov->iov_base + 4, - iov->iov_len - 4); + if (write(c->fd_tap, (char *)iov->iov_base + 4, + iov->iov_len - 4) < 0) + debug("tap write: %s", strerror(errno)); } } tcp6_l2_flags_buf_used = 0; @@ -1338,8 +1343,9 @@ static void tcp_l2_flags_buf_flush(struct ctx *c) for (i = 0; i < mh.msg_iovlen; i++) { struct iovec *iov = &mh.msg_iov[i]; - write(c->fd_tap, (char *)iov->iov_base + 4, - iov->iov_len - 4); + if (write(c->fd_tap, (char *)iov->iov_base + 4, + iov->iov_len - 4) < 0) + debug("tap write: %s", strerror(errno)); } } tcp4_l2_flags_buf_used = 0; @@ -1392,8 +1398,9 @@ static void tcp_l2_buf_flush(struct ctx *c) for (i = 0; i < mh.msg_iovlen; i++) { struct iovec *iov = &mh.msg_iov[i]; - write(c->fd_tap, (char *)iov->iov_base + 4, - iov->iov_len - 4); + if (write(c->fd_tap, (char *)iov->iov_base + 4, + iov->iov_len - 4) < 0) + debug("tap write: %s", strerror(errno)); } } tcp6_l2_buf_used = tcp6_l2_buf_bytes = 0; @@ -1413,8 +1420,9 @@ v4: for (i = 0; i < mh.msg_iovlen; i++) { struct iovec *iov = &mh.msg_iov[i]; - write(c->fd_tap, (char *)iov->iov_base + 4, - iov->iov_len - 4); + if (write(c->fd_tap, (char *)iov->iov_base + 4, + iov->iov_len - 4) < 0) + debug("tap write: %s", strerror(errno)); } } tcp4_l2_buf_used = tcp4_l2_buf_bytes = 0; @@ -1628,14 +1636,16 @@ static int tcp_send_to_tap(struct ctx *c, struct tcp_tap_conn *conn, int flags, iov = tcp4_l2_flags_iov_tap + tcp4_l2_flags_buf_used; p = b4 = tcp4_l2_flags_buf + tcp4_l2_flags_buf_used++; th = &b4->th; + + /* gcc 11.2 would complain on data = (char *)(th + 1); */ + data = b4->opts; } else { iov = tcp6_l2_flags_iov_tap + tcp6_l2_flags_buf_used; p = b6 = tcp6_l2_flags_buf + tcp6_l2_flags_buf_used++; th = &b6->th; + data = b6->opts; } - data = (char *)(th + 1); - if (flags & SYN) { uint16_t mss; @@ -3538,7 +3548,11 @@ int tcp_sock_init(struct ctx *c, struct timespec *now) in_port_t port; int i; - getrandom(&c->tcp.hash_secret, sizeof(c->tcp.hash_secret), GRND_RANDOM); + if (getrandom(&c->tcp.hash_secret, sizeof(c->tcp.hash_secret), + GRND_RANDOM) < 0) { + perror("TCP initial sequence getrandom"); + exit(EXIT_FAILURE); + } for (port = 0; port < USHRT_MAX; port++) { if (!bitmap_isset(c->tcp.port_to_tap, port)) diff --git a/udp.c b/udp.c index ceb10a6..5c0e955 100644 --- a/udp.c +++ b/udp.c @@ -536,8 +536,8 @@ static void udp_sock_handler_splice(struct ctx *c, union epoll_ref ref, uint32_t events, struct timespec *now) { struct msghdr *mh = &udp_splice_mmh_recv[0].msg_hdr; + in_port_t src, dst = ref.udp.port, send_dst = 0; struct sockaddr_storage *sa_s = mh->msg_name; - in_port_t src, dst = ref.udp.port, send_dst; int s, v6 = ref.udp.v6, n, i; if (!(events & EPOLLIN)) @@ -721,7 +721,8 @@ void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, if (c->mode == MODE_PASTA) { ip_len += sizeof(struct ethhdr); - write(c->fd_tap, &b->eh, ip_len); + if (write(c->fd_tap, &b->eh, ip_len) < 0) + debug("tap write: %s", strerror(errno)); pcap((char *)&b->eh, ip_len); continue; } @@ -791,7 +792,8 @@ void udp_sock_handler(struct ctx *c, union epoll_ref ref, uint32_t events, if (c->mode == MODE_PASTA) { ip_len += sizeof(struct ethhdr); - write(c->fd_tap, &b->eh, ip_len); + if (write(c->fd_tap, &b->eh, ip_len) < 0) + debug("tap write: %s", strerror(errno)); pcap((char *)&b->eh, ip_len); continue; }