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

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

[Mathias Lark]

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.

Signed-off-by: Mathias Lark <mathias.lark@gmail.com>
---
 include/uapi/linux/icmp.h               | 5 +++++
 net/ipv4/icmp.c                         | 8 +++-----
 net/netfilter/nf_conntrack_proto_icmp.c | 4 +---
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/include/uapi/linux/icmp.h b/include/uapi/linux/icmp.h
index 163c0998aec9..ad736a24f0c8 100644
--- a/include/uapi/linux/icmp.h
+++ b/include/uapi/linux/icmp.h
@@ -159,4 +159,9 @@ struct icmp_ext_echo_iio {
 		} addr;
 	} ident;
 };
+
+static inline bool icmp_is_valid_type(__u8 type)
+{
+	return type <= NR_ICMP_TYPES || type == ICMP_EXT_ECHO || type == ICMP_EXT_ECHOREPLY;
+}
 #endif /* _UAPI_LINUX_ICMP_H */
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;
 
 	/* 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;
 	}
-- 
2.36.1

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

[Jakub Kicinski]

On Thu, 14 Jul 2022 17:13:58 +0200 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.

Would be good to note in the commit message why we can't bump
NR_ICMP_TYPES to include all the types. What are the types between 
18 and 42?

> diff --git a/include/uapi/linux/icmp.h b/include/uapi/linux/icmp.h
> index 163c0998aec9..ad736a24f0c8 100644
> --- a/include/uapi/linux/icmp.h
> +++ b/include/uapi/linux/icmp.h
> @@ -159,4 +159,9 @@ struct icmp_ext_echo_iio {
>  		} addr;
>  	} ident;
>  };
> +
> +static inline bool icmp_is_valid_type(__u8 type)
> +{
> +	return type <= NR_ICMP_TYPES || type == ICMP_EXT_ECHO || type == ICMP_EXT_ECHOREPLY;
> +}

This doesn't look related to uAPI, include/linux/icmp.h or
include/net/icmp.h seems like a better place for it.

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

[kernel test robot]

Hi Mathias,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathias-Lark/improve-handling-of-ICMP_EXT_ECHO-icmp-type/20220714-231818
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git b126047f43f11f61f1dd64802979765d71795dae
config: x86_64-randconfig-a016 (https://download.01.org/0day-ci/archive/20220716/202207160519.M8OOdyfX-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 5e61b9c556267086ef9b743a0b57df302eef831b)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/d238a024060b9cf5095b5027301f5921c9140c4e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mathias-Lark/improve-handling-of-ICMP_EXT_ECHO-icmp-type/20220714-231818
        git checkout d238a024060b9cf5095b5027301f5921c9140c4e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from <built-in>:1:
>> ./usr/include/linux/icmp.h:163:19: error: unknown type name 'bool'
   static __inline__ bool icmp_is_valid_type(__u8 type)
                     ^
   1 error generated.

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

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

[kernel test robot]

Hi Mathias,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathias-Lark/improve-handling-of-ICMP_EXT_ECHO-icmp-type/20220714-231818
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git b126047f43f11f61f1dd64802979765d71795dae
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20220717/202207171049.oU9toH2Z-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/d238a024060b9cf5095b5027301f5921c9140c4e
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Mathias-Lark/improve-handling-of-ICMP_EXT_ECHO-icmp-type/20220714-231818
        git checkout d238a024060b9cf5095b5027301f5921c9140c4e
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from <command-line>:
>> ./usr/include/linux/icmp.h:163:19: error: unknown type name 'bool'
     163 | static __inline__ bool icmp_is_valid_type(__u8 type)
         |                   ^~~~

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp