Re: [PATCH 3/4] udp: Rework offender address handling in udp_sock_recverr()

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

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

On Wed, Apr 16, 2025 at 04:27:36PM +0200, Stefano Brivio wrote:
> On Wed, 16 Apr 2025 19:07:06 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Make a number of changes to udp_sock_recverr() to improve the robustness
> > of how we handle addresses.
> > 
> >  * Get the "offender" address (source of the ICMP packet) using the
> >    SO_EE_OFFENDER() macro, reducing assumptions about structure layout.
> >  * Parse the offender sockaddr using inany_from_sockaddr()
> >  * Check explicitly that the source and destination pifs are what we
> >    expect.  Previously we checked something that was probably equivalent
> >    in practice, but isn't strictly speaking what we require for the rest
> >    of the code.
> >  * Verify that for an ICMPv4 error we also have an IPv4 source/offender
> >    and destination/endpoint address
> >  * Verify that for an ICMPv6 error we have an IPv6 endpoint
> >  * Improve debug reporting of any failures
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  udp.c | 67 ++++++++++++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 46 insertions(+), 21 deletions(-)
> > 
> > diff --git a/udp.c b/udp.c
> > index 57769d06..4352520e 100644
> > --- a/udp.c
> > +++ b/udp.c
> > @@ -159,6 +159,12 @@ udp_meta[UDP_MAX_FRAMES];
> >  	MAX(CMSG_SPACE(sizeof(struct in_pktinfo)),	\
> >  	    CMSG_SPACE(sizeof(struct in6_pktinfo)))
> >  
> > +#define RECVERR_SPACE							\
> > +	MAX(CMSG_SPACE(sizeof(struct sock_extended_err) +		\
> > +		       sizeof(struct sockaddr_in)),			\
> > +	    CMSG_SPACE(sizeof(struct sock_extended_err) +		\
> > +		       sizeof(struct sockaddr_in6)))
> > +
> >  /**
> >   * enum udp_iov_idx - Indices for the buffers making up a single UDP frame
> >   * @UDP_IOV_TAP         tap specific header
> > @@ -516,12 +522,8 @@ static int udp_pktinfo(struct msghdr *msg, union inany_addr *dst)
> >  static int udp_sock_recverr(const struct ctx *c, int s, flow_sidx_t sidx,
> >  			    uint8_t pif, in_port_t port)
> >  {
> > -	struct errhdr {
> > -		struct sock_extended_err ee;
> > -		union sockaddr_inany saddr;
> > -	};
> > -	char buf[PKTINFO_SPACE + CMSG_SPACE(sizeof(struct errhdr))];
> > -	const struct errhdr *eh = NULL;
> > +	char buf[PKTINFO_SPACE + RECVERR_SPACE];
> > +	const struct sock_extended_err *ee;
> >  	char data[ICMP6_MAX_DLEN];
> >  	struct cmsghdr *hdr;
> >  	struct iovec iov = {
> > @@ -538,7 +540,12 @@ static int udp_sock_recverr(const struct ctx *c, int s, flow_sidx_t sidx,
> >  		.msg_controllen = sizeof(buf),
> >  	};
> >  	const struct flowside *toside;
> > -	flow_sidx_t tosidx;
> > +	char astr[INANY_ADDRSTRLEN];
> > +	char sastr[SOCKADDR_STRLEN];
> > +	union inany_addr offender;
> > +	const struct in_addr *o4;
> > +	in_port_t offender_port;
> > +	uint8_t topif;
> >  	size_t dlen;
> >  	ssize_t rc;
> >  
> > @@ -569,10 +576,10 @@ static int udp_sock_recverr(const struct ctx *c, int s, flow_sidx_t sidx,
> >  		return -1;
> >  	}
> >  
> > -	eh = (const struct errhdr *)CMSG_DATA(hdr);
> > +	ee = (const struct sock_extended_err *)CMSG_DATA(hdr);
> >  
> >  	debug("%s error on UDP socket %i: %s",
> > -	      str_ee_origin(&eh->ee), s, strerror_(eh->ee.ee_errno));
> > +	      str_ee_origin(ee), s, strerror_(ee->ee_errno));
> >  
> >  	if (!flow_sidx_valid(sidx)) {
> >  		/* No hint from the socket, determine flow from addresses */
> > @@ -588,25 +595,43 @@ static int udp_sock_recverr(const struct ctx *c, int s, flow_sidx_t sidx,
> >  			debug("Ignoring UDP error without flow");
> >  			return 1;
> >  		}
> > +	} else {
> > +		pif = pif_at_sidx(sidx);		
> 
> Two stray trailing tabs here.

Oops, fixed.

> >  	}
> >  
> > -	tosidx = flow_sidx_opposite(sidx);
> > -	toside = flowside_at_sidx(tosidx);
> > +	toside = flowside_at_sidx(flow_sidx_opposite(sidx));
> > +	topif = pif_at_sidx(flow_sidx_opposite(sidx));
> >  	dlen = rc;
> >  
> > -	if (pif_is_socket(pif_at_sidx(tosidx))) {
> > -		/* XXX Is there any way to propagate ICMPs from socket to
> > -		 * socket? */
> > -	} else if (hdr->cmsg_level == IPPROTO_IP) {
> > +	if (inany_from_sockaddr(&offender, &offender_port,
> > +				SO_EE_OFFENDER(ee)) < 0)
> > +		goto fail;
> > +
> > +	if (pif != PIF_HOST || topif != PIF_TAP)
> > +		/* XXX Can we support any other cases? */
> > +		goto fail;
> > +
> > +	if (hdr->cmsg_level == IPPROTO_IP &&
> > +	    (o4 = inany_v4(&offender)) && inany_v4(&toside->eaddr)) {
> >  		dlen = MIN(dlen, ICMP4_MAX_DLEN);
> > -		udp_send_tap_icmp4(c, &eh->ee, toside,
> > -				   eh->saddr.sa4.sin_addr, data, dlen);
> > -	} else if (hdr->cmsg_level == IPPROTO_IPV6) {
> > -		udp_send_tap_icmp6(c, &eh->ee, toside,
> > -				   &eh->saddr.sa6.sin6_addr, data,
> > -				   dlen, sidx.flowi);
> > +		udp_send_tap_icmp4(c, ee, toside, *o4, data, dlen);
> > +		return 1;
> > +	}
> > +
> > +	if (hdr->cmsg_level == IPPROTO_IPV6 && !inany_v4(&toside->eaddr)) {
> > +		udp_send_tap_icmp6(c, ee, toside, &offender.a6, data, dlen,
> > +				   sidx.flowi);
> > +		return 1;
> >  	}
> >  
> > +fail:
> > +	flow_dbg(flow_at_sidx(sidx),
> 
> Coverity Scan seems to hallucinate here and says that flow_at_sidx()
> could return NULL, with its return value later dereferenced by
> flow_log(), even if you're explicitly checking flow_sidx_valid() in all
> the paths reaching to this point.
> 
> Calling this conditionally only if flow_sidx_valid() doesn't mask the
> false positive either (I guess that's the part that goes wrong
> somehow), we really need to check if (flow_at_sidx(sidx)) flow_dbg(...).
> 
> Would it be possible to add the useless check just for my own sanity?

Sure.  I was already borderline on whether it was clearer to introduce
an explicit uflow variable, so I've done that now, and asserted it's
non-NULL.  I've checked that removes the coverity whinge, at least
running locally.

> > +		 "Can't propagate %s error from %s %s to %s %s",
> > +		 str_ee_origin(ee),
> > +		 pif_name(pif),
> > +		 sockaddr_ntop(SO_EE_OFFENDER(ee), sastr, sizeof(sastr)),
> > +		 pif_name(topif),
> > +		 inany_ntop(&toside->eaddr, astr, sizeof(astr)));
> >  	return 1;
> >  }
> 

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