[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 <jmaloy(a)redhat.com> wrote:On 2025-03-04 07:05, Stefano Brivio wrote: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);On Mon, 3 Mar 2025 20:29:11 -0500 Jon Maloy <jmaloy(a)redhat.com> 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 applicableI 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.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