Re: [PATCH] udp: Fix port and address checks for DNS forwarder

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

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

On Mon, Oct 10, 2022 at 10:21:09AM +0200, Stefano Brivio wrote:
> First off, as we swap endianness for source ports in
> udp_fill_data_v{4,6}(), we want host endianness, not network
> endianness. It doesn't actually matter if we use htons() or ntohs()
> here, but the current version is confusing.
> 
> In the IPv4 path, when we remap DNS answers, we already swapped the
> endianness as needed for the source port: don't swap it again,
> otherwise we'll not map DNS answers for IPv4.
> 
> In the IPv6 path, when we remap DNS answers, we want to check that
> they came from our upstream DNS server, not the one configured via
> --dns-forward (which doesn't even need to exist for this
> functionality to work).

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

..although you appear to have forgotten your S-o-b on this one.

> ---
>  udp.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/udp.c b/udp.c
> index cac9c65..4b201d3 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -678,7 +678,7 @@ static void udp_sock_fill_data_v4(const struct ctx *c, int n,
>  	b->iph.tot_len = htons(ip_len);
>  
>  	src = ntohl(b->s_in.sin_addr.s_addr);
> -	src_port = htons(b->s_in.sin_port);
> +	src_port = ntohs(b->s_in.sin_port);
>  
>  	if (src >> IN_CLASSA_NSHIFT == IN_LOOPBACKNET ||
>  	    src == INADDR_ANY || src == ntohl(c->ip4.addr_seen)) {
> @@ -693,7 +693,7 @@ static void udp_sock_fill_data_v4(const struct ctx *c, int n,
>  
>  		bitmap_set(udp_act[V4][UDP_ACT_TAP], src_port);
>  	} else if (c->ip4.dns_fwd &&
> -		   src == ntohl(c->ip4.dns[0]) && ntohs(src_port) == 53) {
> +		   src == htonl(c->ip4.dns[0]) && src_port == 53) {
>  		b->iph.saddr = c->ip4.dns_fwd;
>  	} else {
>  		b->iph.saddr = b->s_in.sin_addr.s_addr;
> @@ -795,7 +795,7 @@ static void udp_sock_fill_data_v6(const struct ctx *c, int n,
>  
>  		bitmap_set(udp_act[V6][UDP_ACT_TAP], src_port);
>  	} else if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns_fwd) &&
> -		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.dns_fwd) && src_port == 53) {
> +		   IN6_ARE_ADDR_EQUAL(src, &c->ip6.dns[0]) && src_port == 53) {
>  		b->ip6h.daddr = c->ip6.addr_seen;
>  		b->ip6h.saddr = c->ip6.dns_fwd;
>  	} else {

-- 
David Gibson			| 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 --]