From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202504 header.b=BEiT7wpW; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id ED1D15A0272 for ; Thu, 17 Apr 2025 03:56:02 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202504; t=1744854945; bh=jZkXCn7zQOhdgXXpmrrVEI+/WtleW2qB4I4n+wCWMP8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=BEiT7wpWAizmiaQVvJh+oImFE7oDSn40DBCcXaLkpCpsWHHDav5ubdAvW51pWMnHy iuGo8FkwQ9om3A+PmsBc1RN/zMxHQHrBJVRpbe11ApLeRAhmjo8n2US36EQicQdXLz n/Sw8T4vWQdjqFMk7Jav0LnVAAlh9qVXuUs/L/5fZ3Bs1BKOzhCCkpE4gH6q6CFrAN TLaAq0tjcgxbKl/zWJgSxBEKTBGjNNicJ55QanYC9QwX5fJRYcQ+Iuzj7DoQakEvCp HrbSYebKcZZaLuiVumeiIGGkE2Nhp+qBF31yZiohBfdrOZVQZ0HB1HeM/MZWCvDtoh uAksjE75+zvyg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4ZdLZn6ChXz4xND; Thu, 17 Apr 2025 11:55:45 +1000 (AEST) Date: Thu, 17 Apr 2025 11:33:12 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 3/4] udp: Rework offender address handling in udp_sock_recverr() Message-ID: References: <20250416090707.393497-1-david@gibson.dropbear.id.au> <20250416090707.393497-4-david@gibson.dropbear.id.au> <20250416162736.0380a5c4@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="uHtL84ZFGhK3kX5f" Content-Disposition: inline In-Reply-To: <20250416162736.0380a5c4@elisabeth> Message-ID-Hash: SCUC5HQ5OS6M2JY3YKM7WMOULB6QRN3K X-Message-ID-Hash: SCUC5HQ5OS6M2JY3YKM7WMOULB6QRN3K X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Jon Maloy , passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --uHtL84ZFGhK3kX5f Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: >=20 > > Make a number of changes to udp_sock_recverr() to improve the robustness > > of how we handle addresses. > >=20 > > * 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 > >=20 > > Signed-off-by: David Gibson > > --- > > udp.c | 67 ++++++++++++++++++++++++++++++++++++++++------------------- > > 1 file changed, 46 insertions(+), 21 deletions(-) > >=20 > > 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))) > > =20 > > +#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 f= rame > > * @UDP_IOV_TAP tap specific header > > @@ -516,12 +522,8 @@ static int udp_pktinfo(struct msghdr *msg, union i= nany_addr *dst) > > static int udp_sock_recverr(const struct ctx *c, int s, flow_sidx_t si= dx, > > 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 =3D NULL; > > + char buf[PKTINFO_SPACE + RECVERR_SPACE]; > > + const struct sock_extended_err *ee; > > char data[ICMP6_MAX_DLEN]; > > struct cmsghdr *hdr; > > struct iovec iov =3D { > > @@ -538,7 +540,12 @@ static int udp_sock_recverr(const struct ctx *c, i= nt s, flow_sidx_t sidx, > > .msg_controllen =3D 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; > > =20 > > @@ -569,10 +576,10 @@ static int udp_sock_recverr(const struct ctx *c, = int s, flow_sidx_t sidx, > > return -1; > > } > > =20 > > - eh =3D (const struct errhdr *)CMSG_DATA(hdr); > > + ee =3D (const struct sock_extended_err *)CMSG_DATA(hdr); > > =20 > > 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)); > > =20 > > 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 =3D pif_at_sidx(sidx); =09 >=20 > Two stray trailing tabs here. Oops, fixed. > > } > > =20 > > - tosidx =3D flow_sidx_opposite(sidx); > > - toside =3D flowside_at_sidx(tosidx); > > + toside =3D flowside_at_sidx(flow_sidx_opposite(sidx)); > > + topif =3D pif_at_sidx(flow_sidx_opposite(sidx)); > > dlen =3D rc; > > =20 > > - 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 =3D=3D IPPROTO_IP) { > > + if (inany_from_sockaddr(&offender, &offender_port, > > + SO_EE_OFFENDER(ee)) < 0) > > + goto fail; > > + > > + if (pif !=3D PIF_HOST || topif !=3D PIF_TAP) > > + /* XXX Can we support any other cases? */ > > + goto fail; > > + > > + if (hdr->cmsg_level =3D=3D IPPROTO_IP && > > + (o4 =3D inany_v4(&offender)) && inany_v4(&toside->eaddr)) { > > dlen =3D 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 =3D=3D 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 =3D=3D IPPROTO_IPV6 && !inany_v4(&toside->eaddr))= { > > + udp_send_tap_icmp6(c, ee, toside, &offender.a6, data, dlen, > > + sidx.flowi); > > + return 1; > > } > > =20 > > +fail: > > + flow_dbg(flow_at_sidx(sidx), >=20 > 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. >=20 > 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(...). >=20 > 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; > > } >=20 --=20 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 --uHtL84ZFGhK3kX5f Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmgAWlcACgkQzQJF27ox 2Gdghw/6A9VZrWUc1oNy8GB4l90TIc0dDwn8sqi+QRLhZXeioC1x4G4F1ZpQn9cK ux4k8kphVPGFNev+hz5zun60vWOmROTuY2fdb2bdHZG8ODh71+IQSvvCParGYI2T Z9YMYmNI0wFDetQ89g7dg1TGIvFLig5nFnO8u+npBH0KEdzNRoW1Bl1SK9j88GsJ S6qVS+x2Ogf243M7sjwExJpWc+vmoPMIhH4k9Q795kpe85jIbYtVedaNmpdUTR7A TgrGQVvgLHrH4IMvJegCEEWMK6qQocrJ1dZMMSAGi0Jt1+Uv2PGxT6aebrEAnc8v /HLIYlTjpo8VhZmfei2V66qSCGXUnlnr1H39FldVapnlx5Kph/k+DCOnxMfqu+3x 0f2MVdaWmtLWF4vGiTRfy+0L/wpiIG3O4oBuej+BNm4cp0T9ZoheKShwJIGciR7p cqKE93mb2m3S2qCRxQSYvgWfr7nrUoW/IY7VQ6FesD9pM7hChHm5buw7xBbDZYyp 9cB6CzdGmiWsQvGeK2ywErGPOCOLoa+zNuzyXM7oS9IkRtw+xeYfQaGQOs3mUNLH RLXzM1sc/MQiDs5uw3SZY0noz1IZQc9OIuaAx2TZXLuCcC8E3kmxHnZuRGz6/qpl JqMfPdVf85N//TMU/txGAsX+kchrFMiPcs+Ralrhv/aRd04LVhM= =vg4z -----END PGP SIGNATURE----- --uHtL84ZFGhK3kX5f--