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

[Ingo Molnar <mingo@elte.hu>]

* 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;
 		}