On Wed, Apr 16, 2025 at 04:27:36PM +0200, Stefano Brivio wrote: > On Wed, 16 Apr 2025 19:07:06 +1000 > David Gibson 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 > > --- > > 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