netdev - Re: [PATCH net-next] icmp: support rfc 4884

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <cb763bc5-b361-891a-94e9-be2385ddcbe0@gmail.com>
Date:   Mon, 29 Jun 2020 14:14:55 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        netdev@...r.kernel.org
Cc:     davem@...emloft.net, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next] icmp: support rfc 4884



On 6/29/20 9:57 AM, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@...gle.com>
> 
> ICMP messages may include an extension structure after the original
> datagram. RFC 4884 standardized this behavior.
> 
> It introduces an explicit original datagram length field in the ICMP
> header to delineate the original datagram from the extension struct.
> 
> Return this field when reading an ICMP error from the error queue.

RFC mentions a 'length' field of 8 bits, your patch chose to export the whole
second word of icmp header.

Why is this field mapped to a prior one (icmp_hdr(skb)->un.gateway) ?

Should we add an element in the union to make this a little bit more explicit/readable ?

diff --git a/include/uapi/linux/icmp.h b/include/uapi/linux/icmp.h
index 5589eeb791ca580bb182e1dc38c05eab1c75adb9..427ed5a6765316a4c1e2fa06f3b6618447c01564 100644
--- a/include/uapi/linux/icmp.h
+++ b/include/uapi/linux/icmp.h
@@ -76,6 +76,7 @@ struct icmphdr {
                __be16  sequence;
        } echo;
        __be32  gateway;
+       __be32  second_word; /* RFC 4884 4.[123] : <unused:8>,<length:8>,<mtu:16> */
        struct {
                __be16  __unused;
                __be16  mtu;



> 
> ICMPv6 by default already returns the entire 32-bit part of the header
> that includes this field by default. For consistency, do the exact
> same for ICMP. So far it only returns mtu on ICMP_FRAG_NEEDED and gw
> on ICMP_PARAMETERPROB.
> 
> For backwards compatibility, make this optional, set by setsockopt
> SOL_IP/IP_RECVERR_RFC4884. For API documentation and feature test, see
> https://github.com/wdebruij/kerneltools/blob/master/tests/recv_icmp.c
> 
> Alternative implementation to reading from the skb in ip_icmp_error
> is to pass the field from icmp_unreach, again analogous to ICMPv6. But
> this would require changes to every $proto_err() callback, which for
> ICMP_FRAG_NEEDED pass the u32 info arg to a pmtu update function.
> 
> Signed-off-by: Willem de Bruijn <willemb@...gle.com>
> ---
>  include/net/inet_sock.h |  1 +
>  include/uapi/linux/in.h |  1 +
>  net/ipv4/ip_sockglue.c  | 12 ++++++++++++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
> index a7ce00af6c44..a3702d1d4875 100644
> --- a/include/net/inet_sock.h
> +++ b/include/net/inet_sock.h
> @@ -225,6 +225,7 @@ struct inet_sock {
>  				mc_all:1,
>  				nodefrag:1;
>  	__u8			bind_address_no_port:1,
> +				recverr_rfc4884:1,
>  				defer_connect:1; /* Indicates that fastopen_connect is set
>  						  * and cookie exists so we defer connect
>  						  * until first data frame is written
> diff --git a/include/uapi/linux/in.h b/include/uapi/linux/in.h
> index 8533bf07450f..3d0d8231dc19 100644
> --- a/include/uapi/linux/in.h
> +++ b/include/uapi/linux/in.h
> @@ -123,6 +123,7 @@ struct in_addr {
>  #define IP_CHECKSUM	23
>  #define IP_BIND_ADDRESS_NO_PORT	24
>  #define IP_RECVFRAGSIZE	25
> +#define IP_RECVERR_RFC4884	26
>  
>  /* IP_MTU_DISCOVER values */
>  #define IP_PMTUDISC_DONT		0	/* Never send DF frames */
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 84ec3703c909..525140e3947c 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -398,6 +398,9 @@ void ip_icmp_error(struct sock *sk, struct sk_buff *skb, int err,
>  	if (!skb)
>  		return;
>  
> +	if (inet_sk(sk)->recverr_rfc4884)
> +		info = ntohl(icmp_hdr(skb)->un.gateway);

ntohl(icmp_hdr(skb)->un.second_word);

> +
>  	serr = SKB_EXT_ERR(skb);
>  	serr->ee.ee_errno = err;
>  	serr->ee.ee_origin = SO_EE_ORIGIN_ICMP;
> @@ -755,6 +758,7 @@ static int do_ip_setsockopt(struct sock *sk, int level,
>  	case IP_RECVORIGDSTADDR:
>  	case IP_CHECKSUM:
>  	case IP_RECVFRAGSIZE:
> +	case IP_RECVERR_RFC4884:
>  		if (optlen >= sizeof(int)) {
>  			if (get_user(val, (int __user *) optval))
>  				return -EFAULT;
> @@ -914,6 +918,11 @@ static int do_ip_setsockopt(struct sock *sk, int level,
>  		if (!val)
>  			skb_queue_purge(&sk->sk_error_queue);
>  		break;
> +	case IP_RECVERR_RFC4884:
> +		if (val != 0 && val != 1)
> +			goto e_inval;
> +		inet->recverr_rfc4884 = val;
> +		break;
>  	case IP_MULTICAST_TTL:
>  		if (sk->sk_type == SOCK_STREAM)
>  			goto e_inval;
> @@ -1588,6 +1597,9 @@ static int do_ip_getsockopt(struct sock *sk, int level, int optname,
>  	case IP_RECVERR:
>  		val = inet->recverr;
>  		break;
> +	case IP_RECVERR_RFC4884:
> +		val = inet->recverr_rfc4884;
> +		break;
>  	case IP_MULTICAST_TTL:
>  		val = inet->mc_ttl;
>  		break;
>