netdev - [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

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-next>] [day] [month] [year] [list]

Message-ID: <20080725152059.GA4656@gerrit.erg.abdn.ac.uk>
Date:	Fri, 25 Jul 2008 16:20:59 +0100
From:	Gerrit Renker <gerrit@....abdn.ac.uk>
To:	netdev@...r.kernel.org
Subject: [RFC] sctp/tcp: Question -- ICMPv4 length check (not) redundant?

I wonder if the two pieces of code as marked by the patch below still
serve a purpose. 

>From grepping through the net/ code it seems they are both redundant.

This is on the basis that

 * the v6 counterparts of these handlers ({tcp,sctp}_v6_err) do not have
   this payload-length check;

 * icmp_unreach() in net/ipv4/icmp.c already has this test:

	/* Checkin full IP header plus 8 bytes of protocol to
	 * avoid additional coding at protocol handlers.
	 */
        if (!pskb_may_pull(skb, iph->ihl * 4 + 8))
                goto out;
 
 * a similar test is implemented by icmpv6_notify() in net/ipv6/icmp.c;

 * this type of test seems to be absent in all of the following handlers:
   - tcp_v6_err()		- sctp_v6_err() 
   - __udp4_lib_err()		- __udp6_lib_err()
   - ah4_err()			- ah6_err()
   - esp4_err()			- esp6_err()
   - xfrm_tunnel_err()		- xfrm6_tunnel_err()
   - tunnel4_err()		- tunnel6_err()		- tunnel64_err()
   - ipip_err()			- ipip6_err()
   - ip4ip6_err()		- ip6ip6_err() 
   - ipcomp4_err()		- ipcomp6_err()

---------------------------------------------------------------------------
 The following two are the exceptions in question.
 The question is whether there is a reason for the code marked below.
---------------------------------------------------------------------------

--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -344,11 +344,6 @@ void tcp_v4_err(struct sk_buff *skb, u32 info)
 	int err;
 	struct net *net = dev_net(skb->dev);
 
-	if (skb->len < (iph->ihl << 2) + 8) {
-		ICMP_INC_STATS_BH(net, ICMP_MIB_INERRORS);
-		return;
-	}
-
 	sk = inet_lookup(net, &tcp_hashinfo, iph->daddr, th->dest,
 			iph->saddr, th->source, inet_iif(skb));
 	if (!sk) {
--- a/net/sctp/input.c
+++ b/net/sctp/input.c
@@ -534,11 +534,6 @@ void sctp_v4_err(struct sk_buff *skb, __u32 info)
 	sk_buff_data_t saveip, savesctp;
 	int err;
 
-	if (skb->len < ihlen + 8) {
-		ICMP_INC_STATS_BH(&init_net, ICMP_MIB_INERRORS);
-		return;
-	}
-
 	/* Fix up skb to look at the embedded net header. */
 	saveip = skb->network_header;
 	savesctp = skb->transport_header;

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html