[lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Ingo Molnar]

running an "isic" stresstest on and against a testbox [which, amongst 
other things, generates random incoming and outgoing packets] on 
2.6.17-rc3 (and 2.6.17-rc3-mm1) over gigabit results in a reproducible 
lockup, after 5-10 minutes of runtime:

BUG: soft lockup detected on CPU#0!
 [<c0104e7f>] show_trace+0xd/0xf
 [<c0104e96>] dump_stack+0x15/0x17
 [<c015ad02>] softlockup_tick+0xc5/0xd9
 [<c0134c02>] run_local_timers+0x22/0x24
 [<c0134fb7>] update_process_times+0x40/0x65
 [<c011aa56>] smp_apic_timer_interrupt+0x58/0x60
 [<c010492b>] apic_timer_interrupt+0x27/0x2c
 [<c0f00df9>] sctp_new+0x8b/0x235
 [<c0ef9666>] ip_conntrack_in+0x175/0x4ca
 [<c0eb6dd7>] nf_iterate+0x31/0x94
 [<c0eb6e83>] nf_hook_slow+0x49/0xda
 [<c0ec2f55>] ip_rcv+0x24c/0x567
 [<c0e7dec4>] netif_receive_skb+0x34b/0x397
 [<c07870cb>] rtl8139_poll+0x3d8/0x5db
 [<c0e7c7ad>] net_rx_action+0x9b/0x1ba
 [<c0131955>] __do_softirq+0x6e/0xec
 [<c0106187>] do_softirq+0x59/0xcd
 =======================
 [<c0131427>] local_bh_enable+0x111/0x15d
 [<c0e7d8fb>] dev_queue_xmit+0x218/0x222
 [<c0ec7e2a>] ip_output+0x20b/0x249
 [<c0ec5711>] ip_push_pending_frames+0x331/0x3fe
 [<c0ede6e3>] raw_sendmsg+0x5cf/0x678
 [<c0ee62f8>] inet_sendmsg+0x39/0x46
 [<c0e7406f>] sock_sendmsg+0xf2/0x10d
 [<c0e741e5>] sys_sendmsg+0x15b/0x1c9
 [<c0e748b5>] sys_socketcall+0x16f/0x18a
 [<c1048c1b>] syscall_call+0x7/0xb

this is with FRAME_POINTERS enabled, so it's an exact stacktrace.

the lockup is at:

(gdb) list *0xc0f00df9
0xc0f00df9 is in sctp_new 
(net/ipv4/netfilter/ip_conntrack_proto_sctp.c:444).
439
440             sh = skb_header_pointer(skb, iph->ihl * 4, sizeof(_sctph), &_sctph);
441             if (sh == NULL)
442                     return 0;
443
444             if (do_basic_checks(conntrack, skb, map) != 0)
445                     return 0;
446
447             /* If an OOTB packet has any of these chunks discard (Sec 8.4) */
448             if ((test_bit (SCTP_CID_ABORT, (void *)map))

most likely somewhere within do_basic_checks(). [whose stack entry is 
obscured by the irq entry, so it's not in the stackdump.] I have SCTP 
turned on:

 CONFIG_NETFILTER_XT_MATCH_SCTP=y
 CONFIG_IP_NF_CT_PROTO_SCTP=y
 # SCTP Configuration (EXPERIMENTAL)
 CONFIG_IP_SCTP=y

but otherwise it's a stock Fedora install.

the full log and configs can be found at:

	http://redhat.com/~mingo/misc/crash3.log
	http://redhat.com/~mingo/misc/config3

this is a v2.6.17 showstopper i guess?

	Ingo

Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> running an "isic" stresstest on and against a testbox [which, amongst 
> other things, generates random incoming and outgoing packets] on 
> 2.6.17-rc3 (and 2.6.17-rc3-mm1) over gigabit results in a reproducible 
> lockup, after 5-10 minutes of runtime:
> 
> BUG: soft lockup detected on CPU#0!
>  [<c0104e7f>] show_trace+0xd/0xf
>  [<c0104e96>] dump_stack+0x15/0x17
>  [<c015ad02>] softlockup_tick+0xc5/0xd9
>  [<c0134c02>] run_local_timers+0x22/0x24
>  [<c0134fb7>] update_process_times+0x40/0x65
>  [<c011aa56>] smp_apic_timer_interrupt+0x58/0x60
>  [<c010492b>] apic_timer_interrupt+0x27/0x2c
>  [<c0f00df9>] sctp_new+0x8b/0x235
>  [<c0ef9666>] ip_conntrack_in+0x175/0x4ca
>  [<c0eb6dd7>] nf_iterate+0x31/0x94
>  [<c0eb6e83>] nf_hook_slow+0x49/0xda
>  [<c0ec2f55>] ip_rcv+0x24c/0x567
>  [<c0e7dec4>] netif_receive_skb+0x34b/0x397
>  [<c07870cb>] rtl8139_poll+0x3d8/0x5db
>  [<c0e7c7ad>] net_rx_action+0x9b/0x1ba
>  [<c0131955>] __do_softirq+0x6e/0xec
>  [<c0106187>] do_softirq+0x59/0xcd

thinking about it, what prevents the SCTP chunk's len field from being 
zero, and thus causing an infinite loop in for_each_sctp_chunk()? The 
patch below should fix that.

	Ingo

----
From: Ingo Molnar <mingo@elte.hu>

fix infinite loop in the SCTP-netfilter code: check SCTP chunk size to 
guarantee progress of for_each_sctp_chunk(). (all other uses of 
for_each_sctp_chunk() are preceded by do_basic_checks(), so this fix 
should be complete.)

Signed-off-by: Ingo Molnar <mingo@elte.hu>

Index: linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
===================================================================
--- linux.orig/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
+++ linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
@@ -227,6 +227,15 @@ static int do_basic_checks(struct ip_con
 	flag = 0;
 
 	for_each_sctp_chunk (skb, sch, _sch, offset, count) {
+		unsigned int len = (htons(sch->length) + 3) & ~3;
+
+		/*
+		 * Dont get into a loop with zero-sized or negative
+		 * length values:
+		 */
+		if (!len || len >= skb->len)
+			goto fail;
+
 		DEBUGP("Chunk Num: %d  Type: %d\n", count, sch->type);
 
 		if (sch->type == SCTP_CID_INIT 
@@ -241,6 +250,7 @@ static int do_basic_checks(struct ip_con
 			|| sch->type == SCTP_CID_COOKIE_ECHO
 			|| flag)
 		     && count !=0 ) {
+fail:
 			DEBUGP("Basic checks failed\n");
 			return 1;
 		}

Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > running an "isic" stresstest on and against a testbox [which, amongst 
> > other things, generates random incoming and outgoing packets] on 
> > 2.6.17-rc3 (and 2.6.17-rc3-mm1) over gigabit results in a reproducible 
> > lockup, after 5-10 minutes of runtime:

btw., just in case someone would like to build ISIC on a modern distro, 
the minimal fixes are below. The commandline i'm typically using for 
testing is:

  isic -s firstbox.com -d secondbox.com -m 50000 >/dev/null 2>/dev/null

seems useful.

	Ingo

--- icmpsic.c	2006-04-26 16:14:32.000000000 +0200
+++ icmpsic.c.orig	2006-04-26 16:14:33.000000000 +0200
@@ -265,7 +265,7 @@ main(int argc, char **argv)
 
 		payload = (short int *)((u_char *) icmp + 4);
 		for(cx = 0; cx <= (payload_s >> 1); cx+=1)
-				payload[cx] = rand() & 0xffff;
+				(u_short) payload[cx] = rand() & 0xffff;
 
 
 		if ( rand() <= (RAND_MAX * ICMPCksm) )
--- isic.c	2006-04-26 16:14:32.000000000 +0200
+++ isic.c.orig	2006-04-26 16:14:33.000000000 +0200
@@ -229,8 +229,8 @@ main(int argc, char **argv)
 		
 		payload = (short int *)(buf + IP_H);
 		for(cx = 0; cx <= (payload_s >> 1); cx+=1)
-				payload[cx] = rand() & 0xffff;
-		payload[payload_s] = rand() & 0xffff;
+				(u_int16_t) payload[cx] = rand() & 0xffff;
+		(u_int16_t) payload[payload_s] = rand() & 0xffff;
 		
 		if ( printout ) {
 			printf("%s ->",
--- tcpsic.c	2006-04-26 16:14:32.000000000 +0200
+++ tcpsic.c.orig	2006-04-26 16:14:32.000000000 +0200
@@ -317,7 +317,7 @@ main(int argc, char **argv)
 
 		payload = (short int *)((u_char *) tcp + 20);
 		for(cx = 0; cx <= (payload_s >> 1); cx+=1)
-				payload[cx] = rand() & 0xffff;
+				(u_int16_t) payload[cx] = rand() & 0xffff;
 
 		if ( rand() <= (RAND_MAX * TCPCksm) )
 			libnet_do_checksum(l, (u_int8_t *)buf, IPPROTO_TCP, (tcp->th_off << 2)
--- udpsic.c	2006-04-26 16:14:32.000000000 +0200
+++ udpsic.c.orig	2006-04-26 16:14:32.000000000 +0200
@@ -292,7 +292,7 @@ main(int argc, char **argv)
 
 		payload = (short int *)((u_char *) udp + UDP_H);
 		for(cx = 0; cx <= (payload_s >> 1); cx+=1)
-				payload[cx] = rand() & 0xffff;
+				(u_int16_t) payload[cx] = rand() & 0xffff;
 
 		if ( printout ) {
 			printf("%s,%i ->",

Re: [netfilter-core] Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Patrick McHardy]

Ingo Molnar wrote:
> thinking about it, what prevents the SCTP chunk's len field from being 
> zero, and thus causing an infinite loop in for_each_sctp_chunk()? The 
> patch below should fix that.
> 
> 	Ingo
> 
> ----
> From: Ingo Molnar <mingo@elte.hu>
> 
> fix infinite loop in the SCTP-netfilter code: check SCTP chunk size to 
> guarantee progress of for_each_sctp_chunk(). (all other uses of 
> for_each_sctp_chunk() are preceded by do_basic_checks(), so this fix 
> should be complete.)
> 
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> 
> Index: linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
> ===================================================================
> --- linux.orig/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
> +++ linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
> @@ -227,6 +227,15 @@ static int do_basic_checks(struct ip_con
>  	flag = 0;
>  
>  	for_each_sctp_chunk (skb, sch, _sch, offset, count) {
> +		unsigned int len = (htons(sch->length) + 3) & ~3;
> +
> +		/*
> +		 * Dont get into a loop with zero-sized or negative
> +		 * length values:
> +		 */
> +		if (!len || len >= skb->len)
> +			goto fail;
> +

I just came up with a similar fix :) I think I'm going to take
my own patch though because its IMO slightly nicer. Thanks anyway.

Re: [netfilter-core] Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Ingo Molnar]

* Patrick McHardy <kaber@trash.net> wrote:

> Ingo Molnar wrote:
> > thinking about it, what prevents the SCTP chunk's len field from being 
> > zero, and thus causing an infinite loop in for_each_sctp_chunk()? The 
> > patch below should fix that.
> > 
> > 	Ingo
> > 
> > ----
> > From: Ingo Molnar <mingo@elte.hu>
> > 
> > fix infinite loop in the SCTP-netfilter code: check SCTP chunk size to 
> > guarantee progress of for_each_sctp_chunk(). (all other uses of 
> > for_each_sctp_chunk() are preceded by do_basic_checks(), so this fix 
> > should be complete.)
> > 
> > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> > 
> > Index: linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
> > ===================================================================
> > --- linux.orig/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
> > +++ linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
> > @@ -227,6 +227,15 @@ static int do_basic_checks(struct ip_con
> >  	flag = 0;
> >  
> >  	for_each_sctp_chunk (skb, sch, _sch, offset, count) {
> > +		unsigned int len = (htons(sch->length) + 3) & ~3;
> > +
> > +		/*
> > +		 * Dont get into a loop with zero-sized or negative
> > +		 * length values:
> > +		 */
> > +		if (!len || len >= skb->len)
> > +			goto fail;
> > +
> 
> I just came up with a similar fix :) I think I'm going to take my own 
> patch though because its IMO slightly nicer. Thanks anyway.

could you send your patch so that i can start using it instead of mine?

	Ingo

Re: [netfilter-core] Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 322 bytes --]

Ingo Molnar wrote:
>>
>>I just came up with a similar fix :) I think I'm going to take my own 
>>patch though because its IMO slightly nicer. Thanks anyway.
> 
> 
> could you send your patch so that i can start using it instead of mine?

I did a couple of minutes ago. Here it is again in case my last mail
won't show up.

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2421 bytes --]

[NETFILTER]: Fix endless loop in SCTP conntrack

When a chunk length is zero, for_each_sctp_chunk() doesn't make any forward
progress and loops forever. A chunk length of 0 is invalid, so just abort
in that case.

Reported by Ingo Molnar <mingo@elte.hu>.

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 32491b3d62bc8c3ff2400deebd46972ebc7332af
tree 7249133ec32c18f4e6f989560e8d86b5e2e2cf0c
parent 462f3ddd384045c731b3268a1b9c91c834a5a68a
author Patrick McHardy <kaber@trash.net> Tue, 02 May 2006 15:44:30 +0200
committer Patrick McHardy <kaber@trash.net> Tue, 02 May 2006 15:44:30 +0200

 net/ipv4/netfilter/ip_conntrack_proto_sctp.c |    4 ++--
 net/netfilter/nf_conntrack_proto_sctp.c      |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/netfilter/ip_conntrack_proto_sctp.c b/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
index 5259abd..ebd4ecf 100644
--- a/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
+++ b/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
@@ -209,8 +209,8 @@ static int sctp_print_conntrack(struct s
 #define for_each_sctp_chunk(skb, sch, _sch, offset, count)		\
 for (offset = skb->nh.iph->ihl * 4 + sizeof(sctp_sctphdr_t), count = 0;	\
 	offset < skb->len &&						\
-	(sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch));	\
-	offset += (htons(sch->length) + 3) & ~3, count++)
+	(sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch)) &&  \
+	sch->length; offset += (htons(sch->length) + 3) & ~3, count++)
 
 /* Some validity checks to make sure the chunks are fine */
 static int do_basic_checks(struct ip_conntrack *conntrack,
diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index 9cccc32..2e34436 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -213,8 +213,8 @@ static int sctp_print_conntrack(struct s
 #define for_each_sctp_chunk(skb, sch, _sch, offset, dataoff, count)	\
 for (offset = dataoff + sizeof(sctp_sctphdr_t), count = 0;		\
 	offset < skb->len &&						\
-	(sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch));	\
-	offset += (htons(sch->length) + 3) & ~3, count++)
+	(sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch)) &&	\
+	sch->length; offset += (htons(sch->length) + 3) & ~3, count++)
 
 /* Some validity checks to make sure the chunks are fine */
 static int do_basic_checks(struct nf_conn *conntrack,

Re: [netfilter-core] Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Ingo Molnar]

* Patrick McHardy <kaber@trash.net> wrote:

> I did a couple of minutes ago. Here it is again in case my last mail 
> won't show up.

> -	(sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch));	\
> -	offset += (htons(sch->length) + 3) & ~3, count++)
> +	(sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch)) &&  \
> +	sch->length; offset += (htons(sch->length) + 3) & ~3, count++)

but this makes do_basic_checks() not fail, and the clearly bogus packet 
is passed further down. The reason i have put it inside the loop is to 
be able to return 1 for the early checks. How about the fix below? It 
should be cleaner and it will also return 1 if the initial offset is 
oversized.

	Ingo

----
From: Ingo Molnar <mingo@elte.hu>

fix infinite loop in the SCTP-netfilter code: check SCTP chunk size to 
guarantee progress of for_each_sctp_chunk(). (all other uses of 
for_each_sctp_chunk() are preceded by do_basic_checks(), so this fix 
should be complete.)

Signed-off-by: Ingo Molnar <mingo@elte.hu>

---
 net/ipv4/netfilter/ip_conntrack_proto_sctp.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

Index: linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
===================================================================
--- linux.orig/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
+++ linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
@@ -224,6 +224,13 @@ static int do_basic_checks(struct ip_con
 	DEBUGP(__FUNCTION__);
 	DEBUGP("\n");
 
+	/*
+	 * Dont trust the initial offset:
+	 */
+	offset = skb->nh.iph->ihl * 4 + sizeof(sctp_sctphdr_t);
+	if (offset >= skb->len)
+		return 1;
+
 	flag = 0;
 
 	for_each_sctp_chunk (skb, sch, _sch, offset, count) {
@@ -235,12 +242,15 @@ static int do_basic_checks(struct ip_con
 			flag = 1;
 		}
 
-		/* Cookie Ack/Echo chunks not the first OR 
-		   Init / Init Ack / Shutdown compl chunks not the only chunks */
+		/*
+		 * Cookie Ack/Echo chunks not the first OR 
+		 * Init / Init Ack / Shutdown compl chunks not the only chunks
+		 * OR zero-length.
+		 */
 		if ((sch->type == SCTP_CID_COOKIE_ACK 
 			|| sch->type == SCTP_CID_COOKIE_ECHO
 			|| flag)
-		     && count !=0 ) {
+		     && count !=0 || !sched->length) {
 			DEBUGP("Basic checks failed\n");
 			return 1;
 		}

Re: [netfilter-core] Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> but this makes do_basic_checks() not fail, and the clearly bogus 
> packet is passed further down. The reason i have put it inside the 
> loop is to be able to return 1 for the early checks. How about the fix 
> below? It should be cleaner and it will also return 1 if the initial 
> offset is oversized.

that should be:

----
From: Ingo Molnar <mingo@elte.hu>

fix infinite loop in the SCTP-netfilter code: check SCTP chunk size to 
guarantee progress of for_each_sctp_chunk(). (all other uses of 
for_each_sctp_chunk() are preceded by do_basic_checks(), so this fix 
should be complete.)

Signed-off-by: Ingo Molnar <mingo@elte.hu>

---
 net/ipv4/netfilter/ip_conntrack_proto_sctp.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)

Index: linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
===================================================================
--- linux.orig/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
+++ linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
@@ -224,6 +224,13 @@ static int do_basic_checks(struct ip_con
 	DEBUGP(__FUNCTION__);
 	DEBUGP("\n");
 
+	/*
+	 * Dont trust the initial offset:
+	 */
+	offset = skb->nh.iph->ihl * 4 + sizeof(sctp_sctphdr_t);
+	if (offset >= skb->len)
+		return 1;
+
 	flag = 0;
 
 	for_each_sctp_chunk (skb, sch, _sch, offset, count) {
@@ -235,12 +242,15 @@ static int do_basic_checks(struct ip_con
 			flag = 1;
 		}
 
-		/* Cookie Ack/Echo chunks not the first OR 
-		   Init / Init Ack / Shutdown compl chunks not the only chunks */
-		if ((sch->type == SCTP_CID_COOKIE_ACK 
+		/*
+		 * Cookie Ack/Echo chunks not the first OR
+		 * Init / Init Ack / Shutdown compl chunks not the only chunks
+		 * OR zero-length.
+		 */
+		if (((sch->type == SCTP_CID_COOKIE_ACK
 			|| sch->type == SCTP_CID_COOKIE_ECHO
 			|| flag)
-		     && count !=0 ) {
+		     && count != 0) || !sch->length) {
 			DEBUGP("Basic checks failed\n");
 			return 1;
 		}

Re: [netfilter-core] Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1503 bytes --]

Ingo Molnar wrote:
> * Patrick McHardy <kaber@trash.net> wrote:
> 
> 
>>I did a couple of minutes ago. Here it is again in case my last mail 
>>won't show up.
> 
> 
>>-	(sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch));	\
>>-	offset += (htons(sch->length) + 3) & ~3, count++)
>>+	(sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch)) &&  \
>>+	sch->length; offset += (htons(sch->length) + 3) & ~3, count++)
> 
> 
> but this makes do_basic_checks() not fail, and the clearly bogus packet 
> is passed further down. The reason i have put it inside the loop is to 
> be able to return 1 for the early checks. How about the fix below? It 
> should be cleaner and it will also return 1 if the initial offset is 
> oversized.

Right, that is better.


> Index: linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
> ===================================================================
> --- linux.orig/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
> +++ linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
> @@ -224,6 +224,13 @@ static int do_basic_checks(struct ip_con
>  	DEBUGP(__FUNCTION__);
>  	DEBUGP("\n");
>  
> +	/*
> +	 * Dont trust the initial offset:
> +	 */
> +	offset = skb->nh.iph->ihl * 4 + sizeof(sctp_sctphdr_t);
> +	if (offset >= skb->len)
> +		return 1;
> +

That part is unnecessary, the presence of one sctp_sctphdr_t
has already been verified by skb_header_pointer() in sctp_new().
How about this patch (based on your patch, but typos fixed and
also covers nf_conntrack)?


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2524 bytes --]

[NETFILTER]: SCTP conntrack: fix infinite loop

fix infinite loop in the SCTP-netfilter code: check SCTP chunk size to
guarantee progress of for_each_sctp_chunk(). (all other uses of
for_each_sctp_chunk() are preceded by do_basic_checks(), so this fix
should be complete.)

Based on patch from Ingo Molnar <mingo@elte.hu>

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit c14803e9e8446122312523a6b6959e7312379b2e
tree 651d4f6f1cd07703d3ec6b9367c441312e969e9e
parent 462f3ddd384045c731b3268a1b9c91c834a5a68a
author Patrick McHardy <kaber@trash.net> Tue, 02 May 2006 16:28:26 +0200
committer Patrick McHardy <kaber@trash.net> Tue, 02 May 2006 16:28:26 +0200

 net/ipv4/netfilter/ip_conntrack_proto_sctp.c |   11 +++++++----
 net/netfilter/nf_conntrack_proto_sctp.c      |   11 +++++++----
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/net/ipv4/netfilter/ip_conntrack_proto_sctp.c b/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
index 5259abd..a563534 100644
--- a/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
+++ b/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
@@ -235,12 +235,15 @@ static int do_basic_checks(struct ip_con
 			flag = 1;
 		}
 
-		/* Cookie Ack/Echo chunks not the first OR 
-		   Init / Init Ack / Shutdown compl chunks not the only chunks */
-		if ((sch->type == SCTP_CID_COOKIE_ACK 
+		/*
+		 * Cookie Ack/Echo chunks not the first OR 
+		 * Init / Init Ack / Shutdown compl chunks not the only chunks
+		 * OR zero-length.
+		 */
+		if (((sch->type == SCTP_CID_COOKIE_ACK 
 			|| sch->type == SCTP_CID_COOKIE_ECHO
 			|| flag)
-		     && count !=0 ) {
+		      && count !=0) || !sch->length) {
 			DEBUGP("Basic checks failed\n");
 			return 1;
 		}
diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index 9cccc32..730f5d6 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -240,12 +240,15 @@ static int do_basic_checks(struct nf_con
 			flag = 1;
 		}
 
-		/* Cookie Ack/Echo chunks not the first OR 
-		   Init / Init Ack / Shutdown compl chunks not the only chunks */
-		if ((sch->type == SCTP_CID_COOKIE_ACK 
+		/*
+		 * Cookie Ack/Echo chunks not the first OR
+		 * Init / Init Ack / Shutdown compl chunks not the only chunks
+		 * OR zero-length.
+		 */
+		if (((sch->type == SCTP_CID_COOKIE_ACK 
 			|| sch->type == SCTP_CID_COOKIE_ECHO
 			|| flag)
-		     && count !=0 ) {
+		      && count !=0) || !sch->length) {
 			DEBUGP("Basic checks failed\n");
 			return 1;
 		}

Re: [netfilter-core] Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Ingo Molnar]

* Patrick McHardy <kaber@trash.net> wrote:

> > +	/*
> > +	 * Dont trust the initial offset:
> > +	 */
> > +	offset = skb->nh.iph->ihl * 4 + sizeof(sctp_sctphdr_t);
> > +	if (offset >= skb->len)
> > +		return 1;
> > +
> 
> That part is unnecessary, the presence of one sctp_sctphdr_t
> has already been verified by skb_header_pointer() in sctp_new().

ok.

> How about this patch (based on your patch, but typos fixed and also 
> covers nf_conntrack)?

sure, fine with me!

	Ingo

Re: [netfilter-core] Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Patrick McHardy]

Ingo Molnar wrote:
> * Patrick McHardy <kaber@trash.net> wrote:
> 
>>How about this patch (based on your patch, but typos fixed and also 
>>covers nf_conntrack)?
> 
> 
> sure, fine with me!

OK, thanks. I'll pass it on to Dave soon.

Re: [netfilter-core] Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> > How about this patch (based on your patch, but typos fixed and also 
> > covers nf_conntrack)?
> 
> sure, fine with me!

find updated patch below - quilt complained about 3 cases of 
whitespace-at-end-of-line. (btw., this file at quite a distance from 
proper Documentation/CodingStyle.)

	Ingo

Index: linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
===================================================================
--- linux.orig/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
+++ linux/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
@@ -235,12 +235,15 @@ static int do_basic_checks(struct ip_con
 			flag = 1;
 		}
 
-		/* Cookie Ack/Echo chunks not the first OR 
-		   Init / Init Ack / Shutdown compl chunks not the only chunks */
-		if ((sch->type == SCTP_CID_COOKIE_ACK 
+		/*
+		 * Cookie Ack/Echo chunks not the first OR
+		 * Init / Init Ack / Shutdown compl chunks not the only chunks
+		 * OR zero-length.
+		 */
+		if (((sch->type == SCTP_CID_COOKIE_ACK
 			|| sch->type == SCTP_CID_COOKIE_ECHO
 			|| flag)
-		     && count !=0 ) {
+		      && count !=0) || !sch->length) {
 			DEBUGP("Basic checks failed\n");
 			return 1;
 		}
Index: linux/net/netfilter/nf_conntrack_proto_sctp.c
===================================================================
--- linux.orig/net/netfilter/nf_conntrack_proto_sctp.c
+++ linux/net/netfilter/nf_conntrack_proto_sctp.c
@@ -240,12 +240,15 @@ static int do_basic_checks(struct nf_con
 			flag = 1;
 		}
 
-		/* Cookie Ack/Echo chunks not the first OR 
-		   Init / Init Ack / Shutdown compl chunks not the only chunks */
-		if ((sch->type == SCTP_CID_COOKIE_ACK 
+		/*
+		 * Cookie Ack/Echo chunks not the first OR
+		 * Init / Init Ack / Shutdown compl chunks not the only chunks
+		 * OR zero-length.
+		 */
+		if (((sch->type == SCTP_CID_COOKIE_ACK
 			|| sch->type == SCTP_CID_COOKIE_ECHO
 			|| flag)
-		     && count !=0 ) {
+		      && count !=0) || !sch->length) {
 			DEBUGP("Basic checks failed\n");
 			return 1;
 		}

Re: [netfilter-core] Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Patrick McHardy]

Ingo Molnar wrote:
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
> 
>>>How about this patch (based on your patch, but typos fixed and also 
>>>covers nf_conntrack)?
>>
>>sure, fine with me!
> 
> 
> find updated patch below - quilt complained about 3 cases of 
> whitespace-at-end-of-line. (btw., this file at quite a distance from 
> proper Documentation/CodingStyle.)

My scripts take care of whitespace when exporting patches for pushing
upstream. And yes, it really could use some cleanup - unfortunately
its not even the worst one of the netfilter files.

Re: [netfilter-core] [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

Ingo Molnar wrote:
> running an "isic" stresstest on and against a testbox [which, amongst 
> other things, generates random incoming and outgoing packets] on 
> 2.6.17-rc3 (and 2.6.17-rc3-mm1) over gigabit results in a reproducible 
> lockup, after 5-10 minutes of runtime:
> 
> BUG: soft lockup detected on CPU#0!
>  [<c0104e7f>] show_trace+0xd/0xf
>  [<c0104e96>] dump_stack+0x15/0x17
>  [<c015ad02>] softlockup_tick+0xc5/0xd9
>  [<c0134c02>] run_local_timers+0x22/0x24
>  [<c0134fb7>] update_process_times+0x40/0x65
>  [<c011aa56>] smp_apic_timer_interrupt+0x58/0x60
>  [<c010492b>] apic_timer_interrupt+0x27/0x2c
>  [<c0f00df9>] sctp_new+0x8b/0x235
>  [...]
> 
> this is with FRAME_POINTERS enabled, so it's an exact stacktrace.
> 
> the lockup is at:
> 
> (gdb) list *0xc0f00df9
> 0xc0f00df9 is in sctp_new 
> (net/ipv4/netfilter/ip_conntrack_proto_sctp.c:444).
> 439
> 440             sh = skb_header_pointer(skb, iph->ihl * 4, sizeof(_sctph), &_sctph);
> 441             if (sh == NULL)
> 442                     return 0;
> 443
> 444             if (do_basic_checks(conntrack, skb, map) != 0)
> 445                     return 0;
> 446
> 447             /* If an OOTB packet has any of these chunks discard (Sec 8.4) */
> 448             if ((test_bit (SCTP_CID_ABORT, (void *)map))
> 
> most likely somewhere within do_basic_checks(). [whose stack entry is 
> obscured by the irq entry, so it's not in the stackdump.] I have SCTP 
> turned on:

Yes, it seems like it doesn't make any forward progress in
for_each_sctp_chunk() because the chunk length is zero. Can
you try this patch please?


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 2421 bytes --]

[NETFILTER]: Fix endless loop in SCTP conntrack

When a chunk length is zero, for_each_sctp_chunk() doesn't make any forward
progress and loops forever. A chunk length of 0 is invalid, so just abort
in that case.

Reported by Ingo Molnar <mingo@elte.hu>.

Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 32491b3d62bc8c3ff2400deebd46972ebc7332af
tree 7249133ec32c18f4e6f989560e8d86b5e2e2cf0c
parent 462f3ddd384045c731b3268a1b9c91c834a5a68a
author Patrick McHardy <kaber@trash.net> Tue, 02 May 2006 15:44:30 +0200
committer Patrick McHardy <kaber@trash.net> Tue, 02 May 2006 15:44:30 +0200

 net/ipv4/netfilter/ip_conntrack_proto_sctp.c |    4 ++--
 net/netfilter/nf_conntrack_proto_sctp.c      |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/netfilter/ip_conntrack_proto_sctp.c b/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
index 5259abd..ebd4ecf 100644
--- a/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
+++ b/net/ipv4/netfilter/ip_conntrack_proto_sctp.c
@@ -209,8 +209,8 @@ static int sctp_print_conntrack(struct s
 #define for_each_sctp_chunk(skb, sch, _sch, offset, count)		\
 for (offset = skb->nh.iph->ihl * 4 + sizeof(sctp_sctphdr_t), count = 0;	\
 	offset < skb->len &&						\
-	(sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch));	\
-	offset += (htons(sch->length) + 3) & ~3, count++)
+	(sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch)) &&  \
+	sch->length; offset += (htons(sch->length) + 3) & ~3, count++)
 
 /* Some validity checks to make sure the chunks are fine */
 static int do_basic_checks(struct ip_conntrack *conntrack,
diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
index 9cccc32..2e34436 100644
--- a/net/netfilter/nf_conntrack_proto_sctp.c
+++ b/net/netfilter/nf_conntrack_proto_sctp.c
@@ -213,8 +213,8 @@ static int sctp_print_conntrack(struct s
 #define for_each_sctp_chunk(skb, sch, _sch, offset, dataoff, count)	\
 for (offset = dataoff + sizeof(sctp_sctphdr_t), count = 0;		\
 	offset < skb->len &&						\
-	(sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch));	\
-	offset += (htons(sch->length) + 3) & ~3, count++)
+	(sch = skb_header_pointer(skb, offset, sizeof(_sch), &_sch)) &&	\
+	sch->length; offset += (htons(sch->length) + 3) & ~3, count++)
 
 /* Some validity checks to make sure the chunks are fine */
 static int do_basic_checks(struct nf_conn *conntrack,

Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Marcel Holtmann]

Hi Ingo,

> running an "isic" stresstest on and against a testbox [which, amongst 
> other things, generates random incoming and outgoing packets] on 
> 2.6.17-rc3 (and 2.6.17-rc3-mm1) over gigabit results in a reproducible 
> lockup, after 5-10 minutes of runtime:

does this lockup the local machine you run the stresstest on or the
remote machine you run the test against?

Regards

Marcel

Re: [netfilter-core] Re: [lockup] 2.6.17-rc3: netfilter/sctp: lockup in sctp_new(), do_basic_checks()

[Patrick McHardy]

Marcel Holtmann wrote:
> Hi Ingo,
> 
> 
>>running an "isic" stresstest on and against a testbox [which, amongst 
>>other things, generates random incoming and outgoing packets] on 
>>2.6.17-rc3 (and 2.6.17-rc3-mm1) over gigabit results in a reproducible 
>>lockup, after 5-10 minutes of runtime:
> 
> 
> does this lockup the local machine you run the stresstest on or the
> remote machine you run the test against?

It would lock up any machine with the SCTP conntrack module loaded.
I'll prepare a patch for -stable as well.