Re: [PATCH v5 12/15] tcp, udp: Bind outbound listening sockets by interface instead of address

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5168 bytes --]

Sorry, I realised the warning I add here isn't correct in a couple of
ways.  I'll respin shortly, but patches 1..11 should still be fine to
review.

On Tue, Dec 02, 2025 at 03:02:12PM +1100, David Gibson wrote:
> Currently, outbound forwards (-T, -U) are handled by sockets bound to the
> loopback address.  Typically we create two sockets, one for 127.0.0.1 and
> one for ::1.
> 
> This has some disadvantages:
>  * The guest can't connect via 127.0.0.0/8 addresses other than 127.0.0.1
>  * We can't use dual-stack sockets, we have to have separate sockets for
>    IPv4 and IPv6.
> 
> The restriction exists for a reason though.  If the guest has any
> interfaces other than pasta (e.g. a VPN tunnel) external hosts could reach
> the host via the forwards.  Especially combined with -T auto / -U auto this
> would make it very easy to make a mistake with nasty security implications.
> 
> We can achieve this a different way, however.  Don't bind to a specific
> address, but _do_ use SO_BINDTODEVICE to restrict the sockets to the "lo"
> interface.  We fall back to the old behaviour for older kernels where
> SO_BINDTODEVICE is not available unprivileged.
> 
> Note that although traffic to a local but non-loopback address is passed
> over the 'lo' interface (as seen by netfilter and dumpcap), it doesn't
> count as attached to that interface for the purposes of SO_BINDTODEVICE
> (information from the routing table overrides the "physical" interface).
> So, this change doesn't help for bug 100.
> 
> It's also not a complete fix for bug 113, it does however:
>  * Get us a step closer to fixing bug 113
>  * Slightly simplify the code
>  * Make things a bit easier to allow more flexible binding on the guest in
>    in future
> 
> Link: https://bugs.passt.top/show_bug.cgi?id=113
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c |  6 ++++++
>  pif.c  |  6 ------
>  tcp.c  |  5 +++++
>  udp.c  | 30 +++++++++++++++++++++++-------
>  4 files changed, 34 insertions(+), 13 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 6bd9717b..02a4b65a 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -235,6 +235,12 @@ static void conf_ports(const struct ctx *c, char optname, const char *optarg,
>  		if (c->mode != MODE_PASTA)
>  			die("'auto' port forwarding is only allowed for pasta");
>  
> +		if ((optname == 'T' || optname == 'U') && c->no_bindtodevice) {
> +			warn(
> +"'-%c auto' enabled without unprivileged SO_BINDTODEVICE", optname);
> +			warn(
> +"Forwarding from addresses other than 127.0.0.1 will not work");
> +		}
>  		fwd->mode = FWD_AUTO;
>  		return;
>  	}
> diff --git a/pif.c b/pif.c
> index 85904f35..db447b4f 100644
> --- a/pif.c
> +++ b/pif.c
> @@ -81,12 +81,6 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t pif,
>  
>  	ASSERT(pif_is_socket(pif));
>  
> -	if (pif == PIF_SPLICE) {
> -		/* Sanity checks */
> -		ASSERT(!ifname);
> -		ASSERT(addr && inany_is_loopback(addr));
> -	}
> -
>  	if (!addr) {
>  		ref.fd = sock_l4_dualstack(c, type, port, ifname);
>  	} else {
> diff --git a/tcp.c b/tcp.c
> index 2abb8be4..aacc5b20 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -2627,6 +2627,11 @@ static void tcp_ns_sock_init(const struct ctx *c, in_port_t port)
>  {
>  	ASSERT(!c->no_tcp);
>  
> +	if (!c->no_bindtodevice) {
> +		tcp_sock_init(c, PIF_SPLICE, NULL, "lo", port);
> +		return;
> +	}
> +
>  	if (c->ifi4)
>  		tcp_sock_init_one(c, PIF_SPLICE, &inany_loopback4, NULL, port);
>  	if (c->ifi6)
> diff --git a/udp.c b/udp.c
> index 3d097fbb..4b625b78 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -1182,6 +1182,26 @@ static void udp_splice_iov_init(void)
>  	}
>  }
>  
> +/**
> + * udp_ns_sock_init() - Init socket to listen for spliced outbound connections
> + * @c:		Execution context
> + * @port:	Port, host order
> + */
> +static void udp_ns_sock_init(const struct ctx *c, in_port_t port)
> +{
> +	ASSERT(!c->no_udp);
> +
> +	if (!c->no_bindtodevice) {
> +		udp_sock_init(c, PIF_SPLICE, NULL, "lo", port);
> +		return;
> +	}
> +
> +	if (c->ifi4)
> +		udp_sock_init(c, PIF_SPLICE, &inany_loopback4, NULL, port);
> +	if (c->ifi6)
> +		udp_sock_init(c, PIF_SPLICE, &inany_loopback6, NULL, port);
> +}
> +
>  /**
>   * udp_port_rebind() - Rebind ports to match forward maps
>   * @c:		Execution context
> @@ -1213,14 +1233,10 @@ static void udp_port_rebind(struct ctx *c, bool outbound)
>  
>  		if ((c->ifi4 && socks[V4][port] == -1) ||
>  		    (c->ifi6 && socks[V6][port] == -1)) {
> -			if (outbound) {
> -				udp_sock_init(c, PIF_SPLICE,
> -					      &inany_loopback4, NULL, port);
> -				udp_sock_init(c, PIF_SPLICE,
> -					      &inany_loopback6, NULL, port);
> -			} else {
> +			if (outbound)
> +				udp_ns_sock_init(c, port);
> +			else
>  				udp_sock_init(c, PIF_HOST, NULL, NULL, port);
> -			}
>  		}
>  	}
>  }
> -- 
> 2.52.0
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]