improve handling of ICMP_EXT_ECHO icmp type

PATCH Superseded Mathias Lark 3 participants 4 messages Jul 14, 2022 – Jul 17, 2022

Subsystems (4)

Networking Network drivers netfilter IPv4/IPv6

lore

Share & tools

lore

Mathias Lark

author

2022-07-14 15:16 UTC #0 lore raw

PATCH 1/1 improve handling of ICMP_EXT_ECHO icmp type

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 <[email protected]>

---
 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(-)

include/uapi/linux/icmp.h +5 -0

index 163c0998aec9..ad736a24f0c8 100644
@@ -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 */

net/ipv4/icmp.c +3 -5

index 236debd9fded..686f3133370f 100644
@@ -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;
 	}

net/netfilter/nf_conntrack_proto_icmp.c +1 -3

index b38b7164acd5..ba4462c393be 100644
@@ -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;
 	}

Signature · 1 line

2.36.1

Jakub Kicinski

maintainer

2022-07-15 04:51 UTC #1 lore raw

Quoted 10 lines · 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?

include/uapi/linux/icmp.h struct icmp_ext_echo_iio { · line 166

Quoted 13 lines

include/uapi/linux/icmp.h +5 -0

index 163c0998aec9..ad736a24f0c8 100644
@@ -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.

kernel test robot

build bot

2022-07-15 21:35 UTC #2 lore raw

In reply to Mathias Lark

Thank you for the patch! Yet something to improve:

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/[email protected]/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 <[email protected]>

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.

Signature · 2 lines

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

kernel test robot

build bot

2022-07-17 03:07 UTC #3 lore raw

In reply to Mathias Lark

Thank you for the patch! Yet something to improve:

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/[email protected]/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 <[email protected]>

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)
         |                   ^~~~

Signature · 2 lines

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