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 <david(a)gibson.dropbear.id.au>
---
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