From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 928385A0275 for ; Mon, 29 Jan 2024 05:36:07 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1706502959; bh=6aQESKFVQB0qiUqfHK6SvajeVTXqUeePifuzpmV8Q14=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=QQUxU2unV01Ed7vVLi2mx1BD7sxrkSjg0afbPRv/gC5UM94uDvUsFsXgiVA8oBCKX 8v3aStiJZunqkyBGxf5XAqrlqFwA9gq8Hhwv9lbt9q/nTD7uCNbhFHUogRfUzlH4NB xEzbxhsJBH0wACslxOfkTllmlF9yuiur2xmV9BKzSv/g4nG0w4ZXjIDkWXjKOkirwf 7+tpZL/RplFwlwqjfQTpEYj90riotjraC9Zr88mw8Zma5w5eDlJWte9FB688M1PMEq yEIQmp0eQNkn4lXrdZolrHZCnf9dne+GGdv71Ya2rh0qA3sVHf1s2/pmCgM3Y+nem3 e1b1/qvJS+13Q== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4TNb8b6xxsz4xCg; Mon, 29 Jan 2024 15:35:59 +1100 (AEDT) From: David Gibson To: Stefano Brivio , passt-dev@passt.top Subject: [PATCH 15/16] tcp, tcp_splice: Better construction of IPv4 or IPv6 sockaddrs Date: Mon, 29 Jan 2024 15:35:56 +1100 Message-ID: <20240129043557.823451-16-david@gibson.dropbear.id.au> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20240129043557.823451-1-david@gibson.dropbear.id.au> References: <20240129043557.823451-1-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: OYMMDHNUUPJPT33B2ULD2FOXE2M624QQ X-Message-ID-Hash: OYMMDHNUUPJPT33B2ULD2FOXE2M624QQ X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: David Gibson X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: In both tcp_conn_from_tap() and tcp_splice_connect() we need to construct a socket address for connect() which could be either IPv4 or IPv6. At the moment we initialise both a sockaddr_in and a sockaddr_in6 as locals, then set a pointer to one or the other. This is a little bit ugly. More importantly, though, in the case of tcp_conn_from_tap() initialising the sockaddr_in6 when we're actually passed an IPv4 address will access memory beyond the implied (struct in_addr) we're passed as daddr. In practice that will be a pointer into a packet buffer, so there will be enough valid memory to get 16 bytes of (garbage) IPv6 address that are then ignored. However, it's not a good look to access beyond what the parameters seem to imply is passed. We can clean up these cases using sockaddr_inany and a new helper sockaddr_inany_init(). Signed-off-by: David Gibson --- inany.h | 33 +++++++++++++++++++++++++++++++++ tcp.c | 37 +++++++++++-------------------------- tcp_splice.c | 27 ++++++++------------------- 3 files changed, 52 insertions(+), 45 deletions(-) diff --git a/inany.h b/inany.h index 474e09d0..063545b7 100644 --- a/inany.h +++ b/inany.h @@ -138,4 +138,37 @@ static inline void inany_siphash_feed(struct siphash_state *state, const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size); +/** sockaddr_inany_init - Construct a sockaddr_inany + * @sa: Pointer to sockaddr to fill in + * @sl: Relevant length of @sa after initialisation + * @af: Address family, AF_INET or AF_INET6 + * @addr: Address (either in_addr or in6_addr) + * @port: Port (host byte order) + * @scope: Scope ID for AF_INET6 (ignored for AF_INET) + */ +static inline void sockaddr_inany_init(union sockaddr_inany *sa, socklen_t *sl, + sa_family_t af, const void *addr, + in_port_t port, uint32_t scope) +{ + sa->sa_family = af; + switch (af) { + case AF_INET: + sa->sa4.sin_addr = *(const struct in_addr *)addr; + sa->sa4.sin_port = htons(port); + *sl = sizeof(sa->sa4); + break; + + case AF_INET6: + sa->sa6.sin6_addr = *(const struct in6_addr *)addr; + sa->sa6.sin6_port = htons(port); + sa->sa6.sin6_scope_id = scope; + sa->sa6.sin6_flowinfo = 0; + *sl = sizeof(sa->sa6); + break; + + default: + ASSERT(0); + } +} + #endif /* INANY_H */ diff --git a/tcp.c b/tcp.c index a52a1f84..6c9edbe1 100644 --- a/tcp.c +++ b/tcp.c @@ -1930,18 +1930,9 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, const struct tcphdr *th, const char *opts, size_t optlen, const struct timespec *now) { - struct sockaddr_in addr4 = { - .sin_family = AF_INET, - .sin_port = th->dest, - .sin_addr = *(struct in_addr *)daddr, - }; - struct sockaddr_in6 addr6 = { - .sin6_family = AF_INET6, - .sin6_port = th->dest, - .sin6_addr = *(struct in6_addr *)daddr, - }; - const struct sockaddr *sa; + const void *host_daddr = daddr; struct tcp_tap_conn *conn; + union sockaddr_inany sa; union flow *flow; socklen_t sl; int s, mss; @@ -1956,12 +1947,12 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, if (!c->no_map_gw) { if (af == AF_INET && IN4_ARE_ADDR_EQUAL(daddr, &c->ip4.gw)) - addr4.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + host_daddr = &in4addr_loopback; if (af == AF_INET6 && IN6_ARE_ADDR_EQUAL(daddr, &c->ip6.gw)) - addr6.sin6_addr = in6addr_loopback; + host_daddr = &in6addr_loopback; } - if (af == AF_INET6 && IN6_IS_ADDR_LINKLOCAL(&addr6.sin6_addr)) { + if (af == AF_INET6 && IN6_IS_ADDR_LINKLOCAL(host_daddr)) { struct sockaddr_in6 addr6_ll = { .sin6_family = AF_INET6, .sin6_addr = c->ip6.addr_ll, @@ -1994,13 +1985,7 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, inany_from_af(&conn->faddr, af, daddr); - if (af == AF_INET) { - sa = (struct sockaddr *)&addr4; - sl = sizeof(addr4); - } else { - sa = (struct sockaddr *)&addr6; - sl = sizeof(addr6); - } + sockaddr_inany_init(&sa, &sl, af, host_daddr, ntohs(th->dest), 0); conn->fport = ntohs(th->dest); conn->eport = ntohs(th->source); @@ -2014,19 +1999,19 @@ static void tcp_conn_from_tap(struct ctx *c, sa_family_t af, tcp_hash_insert(c, conn); - if (!bind(s, sa, sl)) { + if (!bind(s, &sa.sa, sl)) { tcp_rst(c, conn); /* Nobody is listening then */ return; } if (errno != EADDRNOTAVAIL && errno != EACCES) conn_flag(c, conn, LOCAL); - if ((af == AF_INET && !IN4_IS_ADDR_LOOPBACK(&addr4.sin_addr)) || - (af == AF_INET6 && !IN6_IS_ADDR_LOOPBACK(&addr6.sin6_addr) && - !IN6_IS_ADDR_LINKLOCAL(&addr6.sin6_addr))) + if ((af == AF_INET && !IN4_IS_ADDR_LOOPBACK(&sa.sa4.sin_addr)) || + (af == AF_INET6 && !IN6_IS_ADDR_LOOPBACK(&sa.sa6.sin6_addr) && + !IN6_IS_ADDR_LINKLOCAL(&sa.sa6.sin6_addr))) tcp_bind_outbound(c, s, af); - if (connect(s, sa, sl)) { + if (connect(s, &sa.sa, sl)) { if (errno != EINPROGRESS) { tcp_rst(c, conn); return; diff --git a/tcp_splice.c b/tcp_splice.c index 3a2c0781..20f56ac3 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -327,17 +327,7 @@ static int tcp_splice_connect_finish(const struct ctx *c, static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn, int sock_conn, in_port_t port) { - struct sockaddr_in6 addr6 = { - .sin6_family = AF_INET6, - .sin6_port = htons(port), - .sin6_addr = IN6ADDR_LOOPBACK_INIT, - }; - struct sockaddr_in addr4 = { - .sin_family = AF_INET, - .sin_port = htons(port), - .sin_addr = IN4ADDR_LOOPBACK_INIT, - }; - const struct sockaddr *sa; + union sockaddr_inany sa; socklen_t sl; conn->s[1] = sock_conn; @@ -348,15 +338,14 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn, conn->s[1]); } - if (CONN_V6(conn)) { - sa = (struct sockaddr *)&addr6; - sl = sizeof(addr6); - } else { - sa = (struct sockaddr *)&addr4; - sl = sizeof(addr4); - } + if (CONN_V6(conn)) + sockaddr_inany_init(&sa, &sl, + AF_INET6, &in6addr_loopback, port, 0); + else + sockaddr_inany_init(&sa, &sl, + AF_INET, &in4addr_loopback, port, 0); - if (connect(conn->s[1], sa, sl)) { + if (connect(conn->s[1], &sa.sa, sl)) { if (errno != EINPROGRESS) return -errno; conn_event(c, conn, SPLICE_CONNECT); -- 2.43.0