[PATCH 0/2] Fix handling of ICMP flows with 0 id
Well, that's the impetus, but it's done as a more general changing of where we sanity check flow endpoints. Link: https://bugs.passt.top/show_bug.cgi?id=105 David Gibson (2): udp: Improve detail of UDP endpoint sanity checking flow: Remove over-zealous sanity checks in flow_sidx_hash() flow.c | 7 +------ udp_flow.c | 32 ++++++++++++++++++++++++-------- 2 files changed, 25 insertions(+), 14 deletions(-) -- 2.47.0
In udp_flow_new() we reject a flow if the endpoint isn't unicast, or it has
a zero endpoint port. Those conditions aren't strictly illegal, but we
can't safely handle them at present:
* Multicast UDP endpoints are certainly possible, but our current flow
tracking only makes sense for simple unicast flows - we'll need
different handling if we want to handle multicast flows in future
* It's not entirely clear if port 0 is RFC-ishly correct, but for socket
interfaces port 0 sometimes has a special meaning such as "pick the port
for me, kernel". That makes flows on port 0 unsafe to forward in the
usual way.
For the same reason we also can't safely handle port 0 as our port. In
principle that's also true for our address, however in the case of flows
initiated from a socket, we may not know our address since the socket
could be bound to 0.0.0.0 or ::, so we can only verify that our address
is unicast for flows initiated from the tap side.
Refine the current check in udp_flow_new() to slightly more detailed checks
in udp_flow_from_sock() and udp_flow_from_tap() to make what is and isn't
handled clearer. This makes this checking more similar to what we do for
TCP connections.
Signed-off-by: David Gibson
In flow_sidx_hash() we verify that the flow we're hashing doesn't have an
unspecified endpoint address, or zero for either port. The hash table only
works if we're looking for exact matches of address and port, and this is
attempting to catch any cases where we might have left address or port
unpopulated or filled with a wildcard.
This doesn't really work though, because there are cases where unspecified
addresses or zero ports are correct:
* We already use unspecified addresses for our address in cases where we
don't know the specific local address for that side, and exclude the
obvious extra check on side->oaddr for that reason.
* Zero port numbers aren't strictly forbidden over the wire. We forbid
them for TCP & UDP because they can't safely be handled on the socket
side. However for ICMP a zero id, which goes in the port field is
valid.
* Possible future flow types (for example, for multicast protocols) might
legitimately have an unspecified address.
Although it makes them easier to miss, these sorts of sanity checks really
have to be done at the protocol / flow type layer, and we already do so.
Remove the checks in flow_sidx_hash() other than checking that the pif
is specified.
Link: https://bugs.passt.top/show_bug.cgi?id=105
Signed-off-by: David Gibson
On Thu, 5 Dec 2024 15:26:00 +1100
David Gibson
Well, that's the impetus, but it's done as a more general changing of where we sanity check flow endpoints.
Link: https://bugs.passt.top/show_bug.cgi?id=105
David Gibson (2): udp: Improve detail of UDP endpoint sanity checking flow: Remove over-zealous sanity checks in flow_sidx_hash()
Applied. -- Stefano
participants (2)
-
David Gibson
-
Stefano Brivio