Re: [PATCH 3/3] fwd: Rework inconsistencies in translation of inbound flows

[Stefano Brivio <sbrivio@redhat.com>]

I applied up to 2/3, I just have one doubt here, and a nit:

On Mon, 12 Aug 2024 19:53:55 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> We usually avoid NAT, but in a few cases we need to apply address
> translations.  The current logic for this on inbound flows has some
> inconsistencies:
> 
>  * For IPv4 (but not IPv6) we translated unspecified source addresses

...I know we already talked about this, but 0.0.0.0/8 is not just
unspecified, it also means "this host on this network" (RFC 6890,
2.2.2), and that's the reason for this apparent inconsistency (::
doesn't). By the way, somebody was reminded of this just recently:

  https://www.oligo.security/blog/0-0-0-0-day-exploiting-localhost-apis-from-the-browser
  https://lwn.net/Articles/984838/

Now, if I recall correctly, the kernel translates that anyway to a
loopback address before we see it. But still it's a legitimate address
to use, so I would keep handling it as a loopback address.

> While an unspecified source probably doesn't make sense, it makes no less
> sense in the guest context than it does for the host, it's possible there
> could be protocols / flow types where this works.  At the moment, this
> should never happen since the protocols will drop the flow before getting
> here.  So, don't alter unspecified addresses for either IP version.
> 
>  * For IPv6 (but not IPv4) we translated the guest's assigned address
> 
> We always translated the guest's observed address, but for the case where
> the assigned address different we should be consistent.  Translate the
> guest assigned address for IPv4 as well.

Hm, right, this looks like an oversight in e58828f34067 ("tcp: Fixes
for closing states, spliced connections, out-of-order packets, etc.").

> The overall point is that we need to translate here if and only if the host
> address is one that's not accessible to the guest.  Create some helper
> functions to determine this, which will have additional uses later.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  fwd.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 77 insertions(+), 11 deletions(-)
> 
> diff --git a/fwd.c b/fwd.c
> index dea36f6c..24700998 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -170,6 +170,73 @@ static bool is_dns_flow(uint8_t proto, const struct flowside *ini)
>  		((ini->fport == 53) || (ini->fport == 853));
>  }
>  
> +/**
> + * fwd_guest_accessible4() - Is IPv4 address guest accessible
> + * @c:		Execution context
> + * @addr:	Host visible IPv4 address
> + *
> + * Return: true if @addr on the host is accessible to the guest without
> + *         translation, false otherwise
> + */
> +static bool fwd_guest_accessible4(const struct ctx *c,
> +				    const struct in_addr *addr)
> +{
> +	if (IN4_IS_ADDR_LOOPBACK(addr))
> +		return false;
> +
> +	if (IN4_ARE_ADDR_EQUAL(addr, &c->ip4.addr))
> +		return false;
> +
> +	if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_seen) &&
> +	    IN4_ARE_ADDR_EQUAL(addr, &c->ip4.addr_seen))
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * fwd_guest_accessible6() - Is IPv6 address guest accessible
> + * @c:		Execution context
> + * @addr:	Host visible IPv6 address
> + *
> + * Return: true if @addr on the host is accessible to the guest without
> + *         translation, false otherwise
> + */
> +static bool fwd_guest_accessible6(const struct ctx *c,
> +				  const struct in6_addr *addr)
> +{
> +	if (IN6_IS_ADDR_LOOPBACK(addr))
> +		return false;
> +
> +	if (IN6_ARE_ADDR_EQUAL(addr, &c->ip6.addr))
> +		return false;
> +
> +	if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_seen) &&
> +	    IN6_ARE_ADDR_EQUAL(addr, &c->ip6.addr_seen))
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
> + * fwd_guest_accessible() - Is IPv[46] address guest accessible
> + * @c:		Execution context
> + * @addr:	Host visible IPv[46] address
> + *
> + * Return: true if @addr on the host is accessible to the guest without
> + *         translation, false otherwise
> + */
> +static bool fwd_guest_accessible(const struct ctx *c,
> +				 const union inany_addr *addr)
> +{
> +	const struct in_addr *a4 = inany_v4(addr);
> +
> +	if (a4)
> +		return fwd_guest_accessible4(c, a4);
> +
> +	return fwd_guest_accessible6(c, &addr->a6);
> +}
> +
>  /**
>   * fwd_nat_from_tap() - Determine to forward a flow from the tap interface
>   * @c:		Execution context
> @@ -307,21 +374,20 @@ uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto,
>  		return PIF_SPLICE;
>  	}
>  
> -	tgt->faddr = ini->eaddr;
> -	tgt->fport = ini->eport;
> -
> -	if (inany_is_loopback4(&tgt->faddr) ||
> -	    inany_is_unspecified4(&tgt->faddr) ||
> -	    inany_equals4(&tgt->faddr, &c->ip4.addr_seen)) {
> -		tgt->faddr = inany_from_v4(c->ip4.gw);
> -	} else if (inany_is_loopback6(&tgt->faddr) ||
> -		   inany_equals6(&tgt->faddr, &c->ip6.addr_seen) ||
> -		   inany_equals6(&tgt->faddr, &c->ip6.addr)) {
> -		if (IN6_IS_ADDR_LINKLOCAL(&c->ip6.gw))
> +	/* These host-visible addresses aren't visible to the guest, so need

I guess you wrote this before moving the checks to their own function:
it took me a bit to figure out that "These host-visible addresses" are
not really mentioned here. Maybe s/These/Some/?

> +	 * translation
> +	 */
> +	if (!fwd_guest_accessible(c, &ini->eaddr)) {
> +		if (inany_v4(&ini->eaddr))
> +			tgt->faddr = inany_from_v4(c->ip4.gw);
> +		else if (IN6_IS_ADDR_LINKLOCAL(&c->ip6.gw))
>  			tgt->faddr.a6 = c->ip6.gw;
>  		else
>  			tgt->faddr.a6 = c->ip6.addr_ll;
> +	} else {
> +		tgt->faddr = ini->eaddr;
>  	}
> +	tgt->fport = ini->eport;
>  
>  	if (inany_v4(&tgt->faddr)) {
>  		tgt->eaddr = inany_from_v4(c->ip4.addr_seen);

-- 
Stefano