Re: [PATCH v2 04/12] icmp: Don't attempt to handle "wrong direction" ping socket traffic

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 21 Dec 2023 17:53:19 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Linux ICMP "ping" sockets are very specific in what they do.  They let
> userspace send ping requests (ICMP_ECHO or ICMP6_ECHO_REQUEST), and receive
> matching replies (ICMP_ECHOREPLY or ICMP6_ECHO_REPLY).  They don't let you
> intercept or handle incoming ping requests.

Right... I don't know exactly what I was trying to do with this, back
then. By the way this is the main reason why it took me a while to
review this series... did I really write all those checks without a
purpose? :) Well, it looks like it.

Anyway, if you look at ping_err() in net/ipv4/ping.c, you'll see that
among the messages which can be sent back as error (they're received on
the socket causing the error -- same as ICMP messages you get on a UDP
socket for port unreachable), ICMP_ECHO is allowed (see
ping_supported()).

I think I just used that ping_supported() function to find out which
messages we could get on the socket. But we're not going to get those
anyway.

By the way, ICMP{,V6}_EXT_ECHO{,_REQUEST} support (RFC 8335, PROBE
messages) was added a while ago (kernel commit 08baf54f01f5 "net: add
support for sending RFC 8335 PROBE messages")... we should add that at
some point, it's kind of trivial.

> In the case of passt/pasta that means we can process echo requests from tap
> and forward them to a ping socket, then take the replies from the ping
> socket and forward them to tap.  We can't do the reverse: take echo
> requests from the host and somehow forward them to the guest. There's
> really no way for something outside to initiate a ping to a passt/pasta
> connected guest and if there was we'd need an entirely different mechanism
> to handle it.
> 
> However, we have some logic to deal with packets going in that reverse
> direction.  Remove it, since it can't ever be used that way.  While we're
> there use defines for the ICMPv6 types, instead of open coded type values.

I guess this last sentence only applied to a previous version of your
patch. It doesn't matter so much, but it would be nice to drop if you
re-spin.

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  icmp.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/icmp.c b/icmp.c
> index c745b7b..1f41440 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -93,8 +93,7 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
>  		icmp_id_map[V4][id].seq = seq;
>  	}
>  
> -	debug("ICMP: echo %s to tap, ID: %i, seq: %i",
> -	      (ih->type == ICMP_ECHO) ? "request" : "reply", id, seq);
> +	debug("ICMP: echo reply to tap, ID: %i, seq: %i", id, seq);
>  
>  	tap_icmp4_send(c, sr.sin_addr, tap_ip4_daddr(c), buf, n);
>  }
> @@ -138,8 +137,7 @@ void icmpv6_sock_handler(const struct ctx *c, union epoll_ref ref)
>  		icmp_id_map[V6][id].seq = seq;
>  	}
>  
> -	debug("ICMPv6: echo %s to tap, ID: %i, seq: %i",
> -	      (ih->icmp6_type == 128) ? "request" : "reply", id, seq);
> +	debug("ICMPv6: echo reply to tap, ID: %i, seq: %i", id, seq);
>  
>  	tap_icmp6_send(c, &sr.sin6_addr,
>  		       tap_ip6_daddr(c, &sr.sin6_addr), buf, n);
> @@ -178,7 +176,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
>  		if (!ih)
>  			return 1;
>  
> -		if (ih->type != ICMP_ECHO && ih->type != ICMP_ECHOREPLY)
> +		if (ih->type != ICMP_ECHO)
>  			return 1;
>  
>  		iref.id = id = ntohs(ih->un.echo.id);
> @@ -205,8 +203,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
>  			   (struct sockaddr *)&sa, sizeof(sa)) < 0) {
>  			debug("ICMP: failed to relay request to socket");
>  		} else {
> -			debug("ICMP: echo %s to socket, ID: %i, seq: %i",
> -			      (ih->type == ICMP_ECHO) ? "request" : "reply",
> +			debug("ICMP: echo request to socket, ID: %i, seq: %i",
>  			      id, ntohs(ih->un.echo.sequence));
>  		}
>  	} else if (af == AF_INET6) {
> @@ -222,7 +219,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
>  		if (!ih)
>  			return 1;
>  
> -		if (ih->icmp6_type != 128 && ih->icmp6_type != 129)
> +		if (ih->icmp6_type != ICMPV6_ECHO_REQUEST)
>  			return 1;
>  
>  		iref.id = id = ntohs(ih->icmp6_identifier);
> @@ -249,8 +246,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
>  			   (struct sockaddr *)&sa, sizeof(sa)) < 1) {
>  			debug("ICMPv6: failed to relay request to socket");
>  		} else {
> -			debug("ICMPv6: echo %s to socket, ID: %i, seq: %i",
> -			      (ih->icmp6_type == 128) ? "request" : "reply",
> +			debug("ICMPv6: echo request to socket, ID: %i, seq: %i",
>  			      id, ntohs(ih->icmp6_sequence));
>  		}
>  	}