Re: [PATCH v2] net-next: improve handling of ICMP_EXT_ECHO icmp type

[Eric Dumazet <eric.dumazet@gmail.com>]

On 7/20/22 10:28, Mathias Lark wrote:
> Introduce a helper for icmp type checking - icmp_is_valid_type.
>
> There is a number of code paths handling ICMP packets. To check
> icmp type validity, some of those code paths perform the check
> `type <= NR_ICMP_TYPES`. Since the introduction of ICMP_EXT_ECHO
> and ICMP_EXT_ECHOREPLY (RFC 8335), this check is no longer correct.
>
> To fix this inconsistency and avoid further problems with future
> ICMP types, the patch inserts the icmp_is_valid type helper
> wherever it is required. The helper checks if the type is less than
> NR_ICMP_TYPES or is equal to ICMP_EXT_ECHO/REPLY.


It is not clear what is the nature of the inconsistency,

and if this patch needs to be backported to versions

after 5.13 (commit d329ea5bd884)

What happens if we do not backport this patch (if it is ever applied after

being reviewed)


>
> NR_ICMP_TYPES could theoretically be increased to ICMP_EXT_ECHOREPLY
> (43), but that would not make sense as types 19-41 are not used.
>
> Signed-off-by: Mathias Lark <mathias.lark@gmail.com>
> ---
>   include/linux/icmp.h                    | 4 ++++
>   net/ipv4/icmp.c                         | 8 +++-----
>   net/netfilter/nf_conntrack_proto_icmp.c | 4 +---
>   3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/icmp.h b/include/linux/icmp.h
> index 0af4d210ee31..e979c80696b0 100644
> --- a/include/linux/icmp.h
> +++ b/include/linux/icmp.h
> @@ -36,6 +36,11 @@ static inline bool icmp_is_err(int type)
>   	return false;
>   }
>   
> +static inline bool icmp_is_valid_type(int type)
> +{
> +	return type <= NR_ICMP_TYPES || type == ICMP_EXT_ECHO || type == ICMP_EXT_ECHOREPLY;
> +}
> +
>   void ip_icmp_error_rfc4884(const struct sk_buff *skb,
>   			   struct sock_ee_data_rfc4884 *out,
>   			   int thlen, int off);
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index 236debd9fded..686f3133370f 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -273,7 +273,7 @@ EXPORT_SYMBOL(icmp_global_allow);
>   
>   static bool icmpv4_mask_allow(struct net *net, int type, int code)
>   {
> -	if (type > NR_ICMP_TYPES)
> +	if (!icmp_is_valid_type(type))
>   		return true;
>   

And later this function will trigger an overflow after your patch is 
applied,

because (1 << 42) is undefined, and sysctl_icmp_ratemask is 32 bit anyway...

if (!((1 << type) & net->ipv4.sysctl_icmp_ratemask))



>   	/* Don't limit PMTU discovery. */
> @@ -661,7 +661,7 @@ void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
>   			 *	Assume any unknown ICMP type is an error. This
>   			 *	isn't specified by the RFC, but think about it..
>   			 */
> -			if (*itp > NR_ICMP_TYPES ||
> +			if (!icmp_is_valid_type(*itp) ||
>   			    icmp_pointers[*itp].error)
>   				goto out;
>   		}
> @@ -1225,12 +1225,10 @@ int icmp_rcv(struct sk_buff *skb)
>   	}
>   
>   	/*
> -	 *	18 is the highest 'known' ICMP type. Anything else is a mystery
> -	 *
>   	 *	RFC 1122: 3.2.2  Unknown ICMP messages types MUST be silently
>   	 *		  discarded.
>   	 */
> -	if (icmph->type > NR_ICMP_TYPES) {
> +	if (!icmp_is_valid_type(icmph->type)) {
>   		reason = SKB_DROP_REASON_UNHANDLED_PROTO;
>   		goto error;
>   	}
> diff --git a/net/netfilter/nf_conntrack_proto_icmp.c b/net/netfilter/nf_conntrack_proto_icmp.c
> index b38b7164acd5..ba4462c393be 100644
> --- a/net/netfilter/nf_conntrack_proto_icmp.c
> +++ b/net/netfilter/nf_conntrack_proto_icmp.c
> @@ -225,12 +225,10 @@ int nf_conntrack_icmpv4_error(struct nf_conn *tmpl,
>   	}
>   
>   	/*
> -	 *	18 is the highest 'known' ICMP type. Anything else is a mystery
> -	 *
>   	 *	RFC 1122: 3.2.2  Unknown ICMP messages types MUST be silently
>   	 *		  discarded.
>   	 */
> -	if (icmph->type > NR_ICMP_TYPES) {
> +	if (!icmp_is_valid_type(icmph->type)) {
>   		icmp_error_log(skb, state, "invalid icmp type");
>   		return -NF_ACCEPT;
>   	}