From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none 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=D8jg3A8s; 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 E96BE5A026F for ; Wed, 05 Mar 2025 09:59:17 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1741165156; 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=FRg282qnkkQQ7JC2PBQLQveoc9d41h1h+pUaaAZlrBw=; b=D8jg3A8sI66mNry26CVDQQqm8Jr8xrzm5rDdzleycGoGSfwqk2avvGn+2tU6+bP/wh5KhY h3pdan57Xjt+fuDZVjdTvNqSrL2Kc7jUSbNALPvmXS/dYvt02nQh7LxAUAqazwwSsPucMX 5EO7Sa6bh9g5Tgdsrq6CslUQOLsUlWE= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-503-re-QZxO3NNGtYlBZHnDDEg-1; Wed, 05 Mar 2025 03:59:15 -0500 X-MC-Unique: re-QZxO3NNGtYlBZHnDDEg-1 X-Mimecast-MFC-AGG-ID: re-QZxO3NNGtYlBZHnDDEg_1741165154 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-43bcddbe698so5782025e9.3 for ; Wed, 05 Mar 2025 00:59:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1741165154; x=1741769954; 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=FRg282qnkkQQ7JC2PBQLQveoc9d41h1h+pUaaAZlrBw=; b=JqPP+YTj5ENnjdfaM0vkZERpv2HAkMyWbgDDaU/e9se/YzVdDFlX5NBg9b+UFGUsn/ rnEx7TeGzpwmrNrEQTdcUPIriM15AINb5jacIsRK1lf/Vz3T4Gd8BF2f4RxCobyu0ZDY B6uHvzJrDs+KUkzTpmTmJ1+Xx+3w6Nl+gCGUd+KVQMrT1rSTfq6DdYhCPDNg4RTA9nE6 rKbUx9F7qQwDY8NR+i+dkqsror9Iu/tEotxFrrt6gMW5qfxHEmCWgGZ8Iuo6rkEu5zFd 2MAZ8JeBkrfCt2K9jd5v/dj0HiQpjrzixzLE2eRy0mMt4YBPDEjg6qNdY9RlaQSDbTvM u5yQ== X-Gm-Message-State: AOJu0YyyeKK9VFFgigPphaMkbwh9pZKapEq+aPSNn2FzTRqLeATrI3ee /ckCyXn/Too9NvQMIW0lFFWu63uN3hoXRmaUXcvXZD+GmD+5mjUHOfccIDVBqCqXHBkNIJuBVrC Ce8x0615/dnyyqRQYba11T2+2m5GaTdKDqbznOX11AsMWSkrToQ== X-Gm-Gg: ASbGncv0OPqaqzENnosHZ/Y+MO2EwGbprscs3h2A5lr9IlJg3mDFjO7gom6XBzN/3Wf EZpOA3SDmHw39f6sKgRyw0yNZe8m1RwTZTkYWi06IPfX6FnjYngkXTEuTdW/LxMMYPMMjEJq51u u9hIQSs0oD8LrXAdl0+i3OD/5GTu6sjd7/bAaLgAat60SjlOgjSXVQ7P6kyGNPDV4BR+P3cbPrp nismkZWswUhWB74JYFVGzarrd0/l9LgzOVJVynY9fECd3Niq5OuuLdsBkKfu5/0cRyW2y4bcZWn OmSXh83T3SolnVMzEfNmINY/i6D7PIesoDZvldGF6EBF X-Received: by 2002:a05:600c:4750:b0:439:9e8b:228e with SMTP id 5b1f17b1804b1-43bd29c42c8mr12527885e9.20.1741165154078; Wed, 05 Mar 2025 00:59:14 -0800 (PST) X-Google-Smtp-Source: AGHT+IHfDIezjKDGeDzXKCCxV+3ZaWym9wPtyBFTkgKeE0qq+B/27O8yPjRqVfdQXTBpzQIih+bXMA== X-Received: by 2002:a05:600c:4750:b0:439:9e8b:228e with SMTP id 5b1f17b1804b1-43bd29c42c8mr12527675e9.20.1741165153646; Wed, 05 Mar 2025 00:59:13 -0800 (PST) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-43bd426c841sm11179385e9.8.2025.03.05.00.59.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Mar 2025 00:59:13 -0800 (PST) Date: Wed, 5 Mar 2025 09:59:11 +0100 From: Stefano Brivio To: Jon Maloy Subject: Re: [PATCH v9 0/4] Reconstruct incoming ICMP headers for failed UDP connect and forward back Message-ID: <20250305095911.355cc499@elisabeth> In-Reply-To: References: <20250304012915.1517536-1-jmaloy@redhat.com> <20250304130520.40dfaa55@elisabeth> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: AgrabO6-pei6NLOJm4zaAu17DV80Cv1Wow7CrQBmjk8_1741165154 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: J3NMKUP4FTX7P7TWIJDPZNUBDEXDTEJI X-Message-ID-Hash: J3NMKUP4FTX7P7TWIJDPZNUBDEXDTEJI 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: passt-dev@passt.top, David Gibson 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: [Dropping Laurent from Cc:, I'm not sure why he's Cc'ed here, and fixing David's email] On Tue, 4 Mar 2025 17:44:29 -0500 Jon Maloy wrote: > On 2025-03-04 07:05, Stefano Brivio wrote: > > On Mon, 3 Mar 2025 20:29:11 -0500 > > Jon Maloy wrote: > > > >> v2: - Added patch breaking out udp header creation from function > >> tap_udp4_send(). > >> - Updated the ICMP creation by using the new function. > >> - Added logics to find correct flow, depending on origin. > >> - All done after feedback from David Gibson. > >> v3: - More changes after feedback from David Gibson. > >> v4: - Even more changes after feedback from D. Gibson > >> v5: - Added corresponding patches for IPv6 > >> v6: - Fixed some small nits after comments from D. Gibson. > >> v7: - Added handling of all rejected ICMP messages > >> - Returning correct user data amount if IPv6 as per RFC 4884. > >> v8: - Added MTU to ICMPv4 ICMP_FRAG_NEEDED messages. > >> - Added ASSERT() validation to message creation functions. > >> v9: - Using real source address of ICMP to complement destination > >> address for originial UDP message when needed. > >> > >> Jon Maloy (4): > >> tap: break out building of udp header from tap_udp4_send function > >> udp: create and send ICMPv4 to local peer when applicable > >> tap: break out building of udp header from tap_udp6_send function > >> udp: create and send ICMPv6 to local peer when applicable > > > > I was about to apply those, then I realised that Coverity Scan isn't > > happy about a few things, listed below. I didn't check if those are > > false positives (I can have a look later or within a couple of days > > unless you get to it first). > > > > 1. > > --- > > /home/sbrivio/passt/udp.c:448:2: > > Type: Out-of-bounds access (ARRAY_VS_SINGLETON) > > > > /home/sbrivio/passt/udp.c:440:2: > > 1. path: Condition "!(dlen <= 8)", taking false branch. > > /home/sbrivio/passt/udp.c:444:2: > > 2. path: Condition "ee->ee_type == 3", taking true branch. > > /home/sbrivio/passt/udp.c:444:2: > > 3. path: Condition "ee->ee_code == 4", taking true branch. > > /home/sbrivio/passt/udp.c:448:2: > > 4. address_of: Taking address with "&msg.ip4h" yields a singleton pointer. > > /home/sbrivio/passt/udp.c:448:2: > > 5. callee_ptr_arith: Passing "&msg.ip4h" to function "tap_push_ip4h" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. > > /home/sbrivio/passt/tap.c:162:2: > > 5.1. ptr_arith: Performing pointer arithmetic on "ip4h" in expression "ip4h + 1". > > --- > > [...] > > > > 3. > > --- > > /home/sbrivio/passt/udp.c:449:2: > > Type: Out-of-bounds access (ARRAY_VS_SINGLETON) > > > > /home/sbrivio/passt/udp.c:440:2: > > 1. path: Condition "!(dlen <= 8)", taking false branch. > > /home/sbrivio/passt/udp.c:444:2: > > 2. path: Condition "ee->ee_type == 3", taking true branch. > > /home/sbrivio/passt/udp.c:444:2: > > 3. path: Condition "ee->ee_code == 4", taking true branch. > > /home/sbrivio/passt/udp.c:449:2: > > 4. address_of: Taking address with "&msg.uh" yields a singleton pointer. > > /home/sbrivio/passt/udp.c:449:2: > > 5. callee_ptr_arith: Passing "&msg.uh" to function "tap_push_uh4" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. > > /home/sbrivio/passt/tap.c:190:2: > > 5.1. ptr_arith: Performing pointer arithmetic on "uh" in expression "uh + 1". > > --- > [...] > I installed coverity and tried it, of course with the same result. > > These are clearly false positives, and the first one is already in the > upstream code, not added by me. Not really, that one: /home/sbrivio/passt/udp.c:448:2: 5. callee_ptr_arith: Passing "&msg.ip4h" to function "tap_push_ip4h" which uses it as an array. This might corrupt or misinterpret adjacent memory locations. /home/sbrivio/passt/tap.c:162:2: 5.1. ptr_arith: Performing pointer arithmetic on "ip4h" in expression "ip4h + 1". comes from patch 2/4 in this series: + /* Reconstruct the original headers as returned in the ICMP message */ + tap_push_ip4h(&msg.ip4h, eaddr, oaddr, l4len, IPPROTO_UDP); + tap_push_uh4(&msg.uh, eaddr, eport, oaddr, oport, in, dlen); > I can probably get rid of them with some > pointer gymnastics, but is it really worth it? You didn't mention why they're false positives, so I checked. First off, I noticed that you forgot to update function comments: you don't say what tap_push_uh4() and tap_push_uh6() return now. Hint: look at the function comments for tap_push_ip4h() and tap_push_ip6h(). By the way, in both comments: * @in: UDP payload contents (not including UDP header) needs two tabs to be aligned with the other arguments. Back to the warning: the fact is that "ip4h + 1", "ipv6h + 1", and "uh + 1" are seen, per se, as array usage. It's kind of arbitrary what "uses it as an array" means: one could even say that simply taking what's after an element (header) qualifies as array usage because you have one element and something after it in an ordered set. So yes, the check is overzealous and not helpful here, but strictly speaking I wouldn't call it a false positive. I think it's worth it because as part of my maintainer duties I routinely use that checker, and keeping warnings to a minimum decreases the noise I have to go through, regardless of the fact that warnings make no sense as it's the case here. Static checkers are otherwise very valuable. Besides, the workaround is perhaps a bit annoying but not significantly less readable, and it took me approximately 30 seconds: -- diff --git a/tap.c b/tap.c index 7e4bc00..48cca00 100644 --- a/tap.c +++ b/tap.c @@ -159,7 +159,8 @@ void *tap_push_ip4h(struct iphdr *ip4h, struct in_addr src, ip4h->saddr = src.s_addr; ip4h->daddr = dst.s_addr; ip4h->check = csum_ip4_header(l3len, proto, src, dst); - return ip4h + 1; + + return (char *)ip4h + sizeof(*ip4h); } /** @@ -187,7 +188,8 @@ void *tap_push_uh4(struct udphdr *uh, struct in_addr src, in_port_t sport, uh->dest = htons(dport); uh->len = htons(l4len); csum_udp4(uh, src, dst, &payload); - return uh + 1; + + return (char *)uh + sizeof(*uh); } /** @@ -262,7 +264,8 @@ void *tap_push_ip6h(struct ipv6hdr *ip6h, ip6h->flow_lbl[0] = (flow >> 16) & 0xf; ip6h->flow_lbl[1] = (flow >> 8) & 0xff; ip6h->flow_lbl[2] = (flow >> 0) & 0xff; - return ip6h + 1; + + return (char *)ip6h + sizeof(*ip6h); } /** @@ -292,7 +295,8 @@ void *tap_push_uh6(struct udphdr *uh, uh->dest = htons(dport); uh->len = htons(l4len); csum_udp6(uh, src, dst, &payload); - return uh + 1; + + return (char *)uh + sizeof(*uh); } /** -- -- Stefano