From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=VY3gd0CX; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id 7AFEB5A0272 for ; Wed, 16 Apr 2025 16:27:50 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1744813669; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=KTRjTZqWlbrztsfQPErXea07+e0QZ2TE3nPbfR1rDhE=; b=VY3gd0CX2OfWydiU2JJjkyZrlwyJOCLR3zQbmMeE12PN4hxG4kR2c6AdXuG2noR3WonFDU 01LTfPGcT5GISZGyCIUg0/kLdBIaOio2MbWgeNtcg3OdXyKGUZe7Nsse0N/SZKUzgO6LLH aW7cIvYHD1zZ8cAwYNY9pDmnOS6Cd7s= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-505-kyvtHhwdMQaBq_d8ayfiuQ-1; Wed, 16 Apr 2025 10:27:48 -0400 X-MC-Unique: kyvtHhwdMQaBq_d8ayfiuQ-1 X-Mimecast-MFC-AGG-ID: kyvtHhwdMQaBq_d8ayfiuQ_1744813666 Received: by mail-wm1-f71.google.com with SMTP id 5b1f17b1804b1-43d3b211d0eso4575435e9.1 for ; Wed, 16 Apr 2025 07:27:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1744813666; x=1745418466; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=KTRjTZqWlbrztsfQPErXea07+e0QZ2TE3nPbfR1rDhE=; b=tGRzkJ39TKoKtCIhLKnj4v+qLc+9byd6Cl3DiDIq68ypcePl4koxuc3fH3hoZBWLso rq3HkIkurdfAKS3LY3jaxZhaio5OaIPA4AmrRxW0P1gvOgTrHs1l2OVHZ2OzmJUBJvz+ pyinU+IG6vEA8Ytn6K1sgTSSSnHkYFfixG1mvwut58T82v3xgtK8PD0fQPkUUMhXlD6G 40SA46NcToC0CUEnaIDv2Slr+xo2kBzz6ovtYkztYKfrQepdTzFGsLhisl3Ge9AJOwET lSMUkOoHwAN2o6QItaf7zVf84mG75qBKcLjakk4puhyG/jHLPagkab28SgJIz9OcEdYo fyBg== X-Forwarded-Encrypted: i=1; AJvYcCXX3RHB0sNCgQKhc4sDpuzzWCH17fVXs8VdZPwzIwSo7qut5PgY+95pOL2dDsTKry+kIbcUZ2ZtpRQ=@passt.top X-Gm-Message-State: AOJu0Yw2L2pjOIrDaK1yytH0967Yg98t3PHHlPNw7oHYD2QO4dbLtqzX xfJwqSLj2klpUsxiSPm9VAnDukZXMNvo+Ju9CJIuewA+AUD1dHA4vtwJLlHz/4Nu9AGCeaMgFP3 pBFYkST6Vf4Pvys+sY/H4kLWyEmmo35OzZ4szubYdXAGPtata8A== X-Gm-Gg: ASbGncv3GNCliqpG8bH8FR9ByCPYOIG3VycbRO/OFGnBNYJwC+rZOA/6+49+xz96iWK yKNkBVwSG1O+dEntXvGvqpU53wA2BtUK+cSWwpzG5ret+X/ebK7/6+pV7wuK0Lhf87+bdXuWy9f DoKRM/E8bcpQJYEv4Yjf34Cl6zfTP1wd8cVci+WqniscejhlIwZsdOAEewPsrxZXJ2sdhxhobPH 1X5m31Qsr9r54pASKhl1V1EJ7hW7lNh/QHgOHDJtoBKDCeCfELiLBg23q/p+dBS403HcsNcQKv0 q5pkJ29rMTPzKeSi5SyeBLvswk1sxfVNofWzVSSM X-Received: by 2002:a05:600c:214e:b0:43c:ed33:a500 with SMTP id 5b1f17b1804b1-4405d7b9ademr19633865e9.10.1744813665827; Wed, 16 Apr 2025 07:27:45 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFF5Vh1J+2gbR9cmdjhy7JpT2xjBtoiIxLLSsOmIlxdbyp3wSxEYXesk4Gk+7aBYiZwpR0ctQ== X-Received: by 2002:a05:600c:214e:b0:43c:ed33:a500 with SMTP id 5b1f17b1804b1-4405d7b9ademr19633595e9.10.1744813665413; Wed, 16 Apr 2025 07:27:45 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-4405b4c83c6sm23344155e9.1.2025.04.16.07.27.45 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Apr 2025 07:27:45 -0700 (PDT) Date: Wed, 16 Apr 2025 16:27:36 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 3/4] udp: Rework offender address handling in udp_sock_recverr() Message-ID: <20250416162736.0380a5c4@elisabeth> In-Reply-To: <20250416090707.393497-4-david@gibson.dropbear.id.au> References: <20250416090707.393497-1-david@gibson.dropbear.id.au> <20250416090707.393497-4-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: v98z1kRVc-SEWsoJvJ7oPND2Fs3xGhve9cQyglekeLY_1744813666 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: N24MPVOWWD3KGMMZ7J4E52ESWIOPH6KI X-Message-ID-Hash: N24MPVOWWD3KGMMZ7J4E52ESWIOPH6KI X-MailFrom: sbrivio@redhat.com 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: 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. > } > > - 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? > + "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; > } -- Stefano