[PATCH v2 0/5] Assorted small fixes for UDP

[PATCH v2 0/5] Assorted small fixes for UDP

[David Gibson]

Partly in looking at flow table support, partly for other reasons,
I've spotted a number of small fixes that can be made to the UDP code
(in addition to the large fixes I'm still working on).

These are ready to go now, so here they are.  They'll go before the
flow related addressing cleanups.

Changes since v1:
 * Removed patch to unconditionally remove ACT flags, which was Just
   Plain Wrong.
 * Removed patch to sort out incorrect handling of port flags.  While
   there's a real bug there, it interacts with at least two more bugs
   that I hadn't spotted, complicating the picture.  I'll fix those
   some other day.

David Gibson (5):
  udp: Don't attempt to translate a 0.0.0.0 source address
  udp: Set pif in epoll reference for ephemeral host sockets
  udp: Small streamline to udp_update_hdr4()
  udp: Fix incorrect usage of IPv6 state in IPv4 path
  udp: Remove unnecessary test for unspecified addr_out

 udp.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

-- 
2.43.2

[PATCH v2 1/5] udp: Don't attempt to translate a 0.0.0.0 source address

[David Gibson]

If an incoming packet has a source address of 0.0.0.0 we translate that to
the gateway address.  This doesn't really make sense, because we have no
way to do a reverse translation for reply packets.

Certain UDP protocols do use an unspecified source address in some
circumstances (e.g. DHCP).  These generally either require no reply, a
multicast reply, or provide a suitable reply address by other means.

In none of those cases does translating it in passt/pasta make sense.  The
best we can really do here is just leave it as is.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/udp.c b/udp.c
index b19e76db..3d44f81e 100644
--- a/udp.c
+++ b/udp.c
@@ -599,7 +599,6 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 	    src_port == 53) {
 		b->iph.saddr = c->ip4.dns_match.s_addr;
 	} else if (IN4_IS_ADDR_LOOPBACK(&b->s_in.sin_addr) ||
-		   IN4_IS_ADDR_UNSPECIFIED(&b->s_in.sin_addr)||
 		   IN4_ARE_ADDR_EQUAL(&b->s_in.sin_addr, &c->ip4.addr_seen)) {
 		b->iph.saddr = c->ip4.gw.s_addr;
 		udp_tap_map[V4][src_port].ts = now->tv_sec;
-- 
@@ -599,7 +599,6 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 	    src_port == 53) {
 		b->iph.saddr = c->ip4.dns_match.s_addr;
 	} else if (IN4_IS_ADDR_LOOPBACK(&b->s_in.sin_addr) ||
-		   IN4_IS_ADDR_UNSPECIFIED(&b->s_in.sin_addr)||
 		   IN4_ARE_ADDR_EQUAL(&b->s_in.sin_addr, &c->ip4.addr_seen)) {
 		b->iph.saddr = c->ip4.gw.s_addr;
 		udp_tap_map[V4][src_port].ts = now->tv_sec;
-- 
2.43.2

[PATCH v2 2/5] udp: Set pif in epoll reference for ephemeral host sockets

[David Gibson]

The udp_epoll_ref contains a field for the pif to which the socket belongs.
We fill this in for permanent sockets created with udp_sock_init() and for
spliced sockets, however, we omit it for ephemeral sockets created for
tap originated flows.

This is a bug, although we currently get away with it, because we don't
consult that field for such flows.  Correctly fill it in.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/udp.c b/udp.c
index 3d44f81e..d992acd0 100644
--- a/udp.c
+++ b/udp.c
@@ -868,7 +868,10 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 		      src, dst, udp_tap_map[V4][src].sock);
 		if ((s = udp_tap_map[V4][src].sock) < 0) {
 			struct in_addr bind_addr = IN4ADDR_ANY_INIT;
-			union udp_epoll_ref uref = { .port = src };
+			union udp_epoll_ref uref = {
+				.port = src,
+				.pif = PIF_HOST,
+			};
 			const char *bind_if = NULL;
 
 			if (!IN6_IS_ADDR_LOOPBACK(&s_in.sin_addr))
@@ -916,7 +919,11 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 		}
 
 		if ((s = udp_tap_map[V6][src].sock) < 0) {
-			union udp_epoll_ref uref = { .v6 = 1, .port = src };
+			union udp_epoll_ref uref = {
+				.v6 = 1,
+				.port = src,
+				.pif = PIF_HOST,
+			};
 			const char *bind_if = NULL;
 
 			if (!IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr))
-- 
@@ -868,7 +868,10 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 		      src, dst, udp_tap_map[V4][src].sock);
 		if ((s = udp_tap_map[V4][src].sock) < 0) {
 			struct in_addr bind_addr = IN4ADDR_ANY_INIT;
-			union udp_epoll_ref uref = { .port = src };
+			union udp_epoll_ref uref = {
+				.port = src,
+				.pif = PIF_HOST,
+			};
 			const char *bind_if = NULL;
 
 			if (!IN6_IS_ADDR_LOOPBACK(&s_in.sin_addr))
@@ -916,7 +919,11 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 		}
 
 		if ((s = udp_tap_map[V6][src].sock) < 0) {
-			union udp_epoll_ref uref = { .v6 = 1, .port = src };
+			union udp_epoll_ref uref = {
+				.v6 = 1,
+				.port = src,
+				.pif = PIF_HOST,
+			};
 			const char *bind_if = NULL;
 
 			if (!IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr))
-- 
2.43.2

[PATCH v2 3/5] udp: Small streamline to udp_update_hdr4()

[David Gibson]

Streamline the logic here slightly, by introducing a 'src' temporary for
brevity.  We also transform the logic for setting/clearing PORT_LOOPBACK.
This makes udp_update_hdr4() more closely match the corresponding logic
from udp_update_udp6().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/udp.c b/udp.c
index d992acd0..561ba604 100644
--- a/udp.c
+++ b/udp.c
@@ -584,6 +584,7 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 			      const struct timespec *now)
 {
 	struct udp4_l2_buf_t *b = &udp4_l2_buf[n];
+	struct in_addr *src;
 	in_port_t src_port;
 	size_t ip_len;
 
@@ -592,26 +593,26 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 	b->iph.tot_len = htons(ip_len);
 	b->iph.daddr = c->ip4.addr_seen.s_addr;
 
+	src = &b->s_in.sin_addr;
 	src_port = ntohs(b->s_in.sin_port);
 
 	if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match) &&
-	    IN4_ARE_ADDR_EQUAL(&b->s_in.sin_addr, &c->ip4.dns_host) &&
-	    src_port == 53) {
+	    IN4_ARE_ADDR_EQUAL(src, &c->ip4.dns_host) && src_port == 53) {
 		b->iph.saddr = c->ip4.dns_match.s_addr;
-	} else if (IN4_IS_ADDR_LOOPBACK(&b->s_in.sin_addr) ||
-		   IN4_ARE_ADDR_EQUAL(&b->s_in.sin_addr, &c->ip4.addr_seen)) {
+	} else if (IN4_IS_ADDR_LOOPBACK(src) ||
+		   IN4_ARE_ADDR_EQUAL(src, &c->ip4.addr_seen)) {
 		b->iph.saddr = c->ip4.gw.s_addr;
 		udp_tap_map[V4][src_port].ts = now->tv_sec;
 		udp_tap_map[V4][src_port].flags |= PORT_LOCAL;
 
-		if (IN4_ARE_ADDR_EQUAL(&b->s_in.sin_addr.s_addr, &c->ip4.addr_seen))
-			udp_tap_map[V4][src_port].flags &= ~PORT_LOOPBACK;
-		else
+		if (IN4_IS_ADDR_LOOPBACK(src))
 			udp_tap_map[V4][src_port].flags |= PORT_LOOPBACK;
+		else
+			udp_tap_map[V4][src_port].flags &= ~PORT_LOOPBACK;
 
 		bitmap_set(udp_act[V4][UDP_ACT_TAP], src_port);
 	} else {
-		b->iph.saddr = b->s_in.sin_addr.s_addr;
+		b->iph.saddr = src->s_addr;
 	}
 
 	udp_update_check4(b);
-- 
@@ -584,6 +584,7 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 			      const struct timespec *now)
 {
 	struct udp4_l2_buf_t *b = &udp4_l2_buf[n];
+	struct in_addr *src;
 	in_port_t src_port;
 	size_t ip_len;
 
@@ -592,26 +593,26 @@ static size_t udp_update_hdr4(const struct ctx *c, int n, in_port_t dstport,
 	b->iph.tot_len = htons(ip_len);
 	b->iph.daddr = c->ip4.addr_seen.s_addr;
 
+	src = &b->s_in.sin_addr;
 	src_port = ntohs(b->s_in.sin_port);
 
 	if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match) &&
-	    IN4_ARE_ADDR_EQUAL(&b->s_in.sin_addr, &c->ip4.dns_host) &&
-	    src_port == 53) {
+	    IN4_ARE_ADDR_EQUAL(src, &c->ip4.dns_host) && src_port == 53) {
 		b->iph.saddr = c->ip4.dns_match.s_addr;
-	} else if (IN4_IS_ADDR_LOOPBACK(&b->s_in.sin_addr) ||
-		   IN4_ARE_ADDR_EQUAL(&b->s_in.sin_addr, &c->ip4.addr_seen)) {
+	} else if (IN4_IS_ADDR_LOOPBACK(src) ||
+		   IN4_ARE_ADDR_EQUAL(src, &c->ip4.addr_seen)) {
 		b->iph.saddr = c->ip4.gw.s_addr;
 		udp_tap_map[V4][src_port].ts = now->tv_sec;
 		udp_tap_map[V4][src_port].flags |= PORT_LOCAL;
 
-		if (IN4_ARE_ADDR_EQUAL(&b->s_in.sin_addr.s_addr, &c->ip4.addr_seen))
-			udp_tap_map[V4][src_port].flags &= ~PORT_LOOPBACK;
-		else
+		if (IN4_IS_ADDR_LOOPBACK(src))
 			udp_tap_map[V4][src_port].flags |= PORT_LOOPBACK;
+		else
+			udp_tap_map[V4][src_port].flags &= ~PORT_LOOPBACK;
 
 		bitmap_set(udp_act[V4][UDP_ACT_TAP], src_port);
 	} else {
-		b->iph.saddr = b->s_in.sin_addr.s_addr;
+		b->iph.saddr = src->s_addr;
 	}
 
 	udp_update_check4(b);
-- 
2.43.2

[PATCH v2 4/5] udp: Fix incorrect usage of IPv6 state in IPv4 path

[David Gibson]

When forwarding IPv4 packets in udp_tap_handler(), we incorrectly use an
IPv6 address test on our IPv4 address (which could cause an out of bounds
access), and possibly set our bind interface to the IPv6 interface based on
it.  Adjust to correctly look at the IPv4 address and IPv4 interface.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/udp.c b/udp.c
index 561ba604..d55a6827 100644
--- a/udp.c
+++ b/udp.c
@@ -875,8 +875,8 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 			};
 			const char *bind_if = NULL;
 
-			if (!IN6_IS_ADDR_LOOPBACK(&s_in.sin_addr))
-				bind_if = c->ip6.ifname_out;
+			if (!IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr))
+				bind_if = c->ip4.ifname_out;
 
 			if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out) &&
 			    !IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr))
-- 
@@ -875,8 +875,8 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 			};
 			const char *bind_if = NULL;
 
-			if (!IN6_IS_ADDR_LOOPBACK(&s_in.sin_addr))
-				bind_if = c->ip6.ifname_out;
+			if (!IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr))
+				bind_if = c->ip4.ifname_out;
 
 			if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out) &&
 			    !IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr))
-- 
2.43.2

[PATCH v2 5/5] udp: Remove unnecessary test for unspecified addr_out

[David Gibson]

If the configured output address is unspecified, we don't set the bind
address to it when creating a new socket in udp_tap_handler().  That sounds
sensible, but what we're leaving the bind address as is, exactly, the
unspecified address, so this test makes no difference.  Remove it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 udp.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/udp.c b/udp.c
index d55a6827..5b7778eb 100644
--- a/udp.c
+++ b/udp.c
@@ -878,8 +878,7 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 			if (!IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr))
 				bind_if = c->ip4.ifname_out;
 
-			if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out) &&
-			    !IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr))
+			if (!IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr))
 				bind_addr = c->ip4.addr_out;
 
 			s = sock_l4(c, AF_INET, IPPROTO_UDP, &bind_addr,
@@ -930,8 +929,7 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 			if (!IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr))
 				bind_if = c->ip6.ifname_out;
 
-			if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_out) &&
-			    !IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr) &&
+			if (!IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr) &&
 			    !IN6_IS_ADDR_LINKLOCAL(&s_in6.sin6_addr))
 				bind_addr = &c->ip6.addr_out;
 
-- 
@@ -878,8 +878,7 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 			if (!IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr))
 				bind_if = c->ip4.ifname_out;
 
-			if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_out) &&
-			    !IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr))
+			if (!IN4_IS_ADDR_LOOPBACK(&s_in.sin_addr))
 				bind_addr = c->ip4.addr_out;
 
 			s = sock_l4(c, AF_INET, IPPROTO_UDP, &bind_addr,
@@ -930,8 +929,7 @@ int udp_tap_handler(struct ctx *c, uint8_t pif,
 			if (!IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr))
 				bind_if = c->ip6.ifname_out;
 
-			if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_out) &&
-			    !IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr) &&
+			if (!IN6_IS_ADDR_LOOPBACK(&s_in6.sin6_addr) &&
 			    !IN6_IS_ADDR_LINKLOCAL(&s_in6.sin6_addr))
 				bind_addr = &c->ip6.addr_out;
 
-- 
2.43.2

Re: [PATCH v2 0/5] Assorted small fixes for UDP

[Stefano Brivio]

On Wed, 28 Feb 2024 16:39:24 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Partly in looking at flow table support, partly for other reasons,
> I've spotted a number of small fixes that can be made to the UDP code
> (in addition to the large fixes I'm still working on).
> 
> These are ready to go now, so here they are.  They'll go before the
> flow related addressing cleanups.
> 
> Changes since v1:
>  * Removed patch to unconditionally remove ACT flags, which was Just
>    Plain Wrong.
>  * Removed patch to sort out incorrect handling of port flags.  While
>    there's a real bug there, it interacts with at least two more bugs
>    that I hadn't spotted, complicating the picture.  I'll fix those
>    some other day.
> 
> David Gibson (5):
>   udp: Don't attempt to translate a 0.0.0.0 source address
>   udp: Set pif in epoll reference for ephemeral host sockets
>   udp: Small streamline to udp_update_hdr4()
>   udp: Fix incorrect usage of IPv6 state in IPv4 path
>   udp: Remove unnecessary test for unspecified addr_out

Applied.

-- 
Stefano