net-next: improve handling of ICMP_EXT_ECHO icmp type
Subsystems (4)
PATCH v2 1/1 net-next: 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. The helper checks if the type is less than NR_ICMP_TYPES or is equal to ICMP_EXT_ECHO/REPLY.
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.
--- 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(-)
include/linux/icmp.h +5 -0
@@ -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);
net/ipv4/icmp.c +3 -5
@@ -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
@@ -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
Quoted 11 lines · 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 ofICMP_EXT_ECHOandICMP_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_validtype helper wherever it is required. The helper checks if the type is less thanNR_ICMP_TYPESor 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)
Quoted 32 lines
NR_ICMP_TYPEScould theoretically be increased toICMP_EXT_ECHOREPLY(43), but that would not make sense as types 19-41 are not used.Signed-off-by Mathias Lark <[email protected]>--- 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(-)include/linux/icmp.h +5
-0@@ -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);net/ipv4/icmp.c +0
-0
net/ipv4/icmp.c EXPORT_SYMBOL(icmp_global_allow); · line 278
Quoted 7 lines
net/ipv4/icmp.c +1
-1@@ -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))
Quoted 42 lines
net/ipv4/icmp.c +2
-4/* 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@@ -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 ; }