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

[Jon Maloy <jmaloy@redhat.com>]

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. I can probably get rid of them with some 
pointer gymnastics, but is it really worth it?

BTW, I discovered a bug in patch #2 which I just fixed. I will post a 
v10 of my patches shortly.

///jon