Re: [PATCH v9 0/4] Reconstruct incoming ICMP headers for failed UDP connect and forward back

[Stefano Brivio <sbrivio@redhat.com>]

[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@redhat.com> wrote:

> On 2025-03-04 07:05, Stefano Brivio wrote:
> > On Mon,  3 Mar 2025 20:29:11 -0500
> > Jon Maloy <jmaloy@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 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);
 }
 
 /**
--

-- 
@@ -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