FIX: connlimit NULL pointer kernel panic (was: connlimit patch crashes 2.6.11 kernel)

FIX: connlimit NULL pointer kernel panic (was: connlimit patch crashes 2.6.11 kernel)

[Damon Gray]

I have already submitted this to "the powers that be" but haven't heard 
back yet. Here is how to fix it yourself.

The problem is in ipt_connlimit.c(line 67):

  found = ip_conntrack_find_get(&conn->tuple,ct);
  if (0 == memcmp(&conn->tuple,&tuple,sizeof(tuple)) &&
    found != NULL && (found_ct = tuplehash_to_ctrack(found)) != NULL &&
    found_ct->proto.tcp.state != TCP_CONNTRACK_TIME_WAIT) {
      /* Just to be sure we have it only once in the list.
         We should'nt see tuples twice unless someone hooks this
         into a table without "-p tcp --syn" */
     addit = 0;
  }

The problem is that it is the usual case that "found" will not equal NULL, 
but the memcmp will also not equal 0. This makes it so 
tuplehash_to_ctrack(found) is never run so "found_ct" is always NULL. 
Later in the function "found_ct" is dereferenced when it is NULL, which 
causes the kernel panic. These operations need to be reordered so it is 
guarantee that if "found" != NULL then tuplehash_to_ctrack will always be 
run.

Basically it needs to be changed to:

  if (found != NULL && (found_ct = tuplehash_to_ctrack(found)) != NULL &&
    0 == memcmp(&conn->tuple,&tuple,sizeof(tuple)) &&
    found_ct->proto.tcp.state != TCP_CONNTRACK_TIME_WAIT) {


Change the file to the above and recompile the kernel. I've tested on 
2.6.11.7.

-Damon-

+-------------------------------------------------------+
| Damon Gray                | Core Network Development  |
| (404)302-9756             | Internap Network Services |
| dgray@internap.com        | www.internap.com          |
+-------------------------------------------------------+

Re: FIX: connlimit NULL pointer kernel panic (was: connlimit patch crashes 2.6.11 kernel)

[Pablo Neira]

Damon Gray wrote:
> The problem is that it is the usual case that "found" will not equal 
> NULL, but the memcmp will also not equal 0. This makes it so 
> tuplehash_to_ctrack(found) is never run so "found_ct" is always NULL. 
> Later in the function "found_ct" is dereferenced when it is NULL, which 
> causes the kernel panic. These operations need to be reordered so it is 
> guarantee that if "found" != NULL then tuplehash_to_ctrack will always 
> be run.
> 
> Basically it needs to be changed to:
> 
>  if (found != NULL && (found_ct = tuplehash_to_ctrack(found)) != NULL &&
>    0 == memcmp(&conn->tuple,&tuple,sizeof(tuple)) &&
>    found_ct->proto.tcp.state != TCP_CONNTRACK_TIME_WAIT) {

nice, could you post a patch against SVN? so it can be applied easily. 
While you are at it, could you sed 's/spin_lock/spin_lock_bh/g' as well? 
a quick look tells me that current locking is wrong.

For the record, this patch should be re-made to work on top of the 
conntrack-event-api, that way it will support more protocols than just 
tcp. Actually, someone already did this thing some time ago [1].

[1] 
https://lists.netfilter.org/pipermail/netfilter-devel/2004-November/017314.html

--
Pablo

RE: FIX: connlimit NULL pointer kernel panic (was: connlimit patch crashes 2.6.11 kernel)

[Forte Systems - Iosif Peterfi]

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

Here is a diff -urN patch for what has been discussed except the conntrack
event-api. Works fine on 2.6.1-gentoo-r8, patch-o-matic-ng-20050516. 

Thanks a lot.

-----Original Message-----
From: netfilter-devel-bounces@lists.netfilter.org
[mailto:netfilter-devel-bounces@lists.netfilter.org] On Behalf Of Pablo
Neira
Sent: Thursday, May 19, 2005 6:11 AM
To: Damon Gray
Cc: mateusz@republika.pl; netfilter-devel@lists.netfilter.org;
kaber@trash.net
Subject: Re: FIX: connlimit NULL pointer kernel panic (was: connlimit patch
crashes 2.6.11 kernel)

Damon Gray wrote:
> The problem is that it is the usual case that "found" will not equal 
> NULL, but the memcmp will also not equal 0. This makes it so 
> tuplehash_to_ctrack(found) is never run so "found_ct" is always NULL. 
> Later in the function "found_ct" is dereferenced when it is NULL, which 
> causes the kernel panic. These operations need to be reordered so it is 
> guarantee that if "found" != NULL then tuplehash_to_ctrack will always 
> be run.
> 
> Basically it needs to be changed to:
> 
>  if (found != NULL && (found_ct = tuplehash_to_ctrack(found)) != NULL &&
>    0 == memcmp(&conn->tuple,&tuple,sizeof(tuple)) &&
>    found_ct->proto.tcp.state != TCP_CONNTRACK_TIME_WAIT) {

nice, could you post a patch against SVN? so it can be applied easily. 
While you are at it, could you sed 's/spin_lock/spin_lock_bh/g' as well? 
a quick look tells me that current locking is wrong.

For the record, this patch should be re-made to work on top of the 
conntrack-event-api, that way it will support more protocols than just 
tcp. Actually, someone already did this thing some time ago [1].

[1] 
https://lists.netfilter.org/pipermail/netfilter-devel/2004-November/017314.h
tml

--
Pablo



-- 
This message was scanned for spam and viruses by BitDefender.
For more information please visit http://linux.bitdefender.com/


[-- Attachment #2: ipt_connlimit.c.patch --]
[-- Type: application/octet-stream, Size: 1165 bytes --]

--- ipt_connlimit.c.orig        2005-05-19 10:15:30.000000000 +0300
+++ ipt_connlimit.c     2005-05-19 10:27:31.000000000 +0300
@@ -55,7 +55,7 @@
        struct ipt_connlimit_conn *conn;
        struct list_head *hash,*lh;

-       spin_lock(&data->lock);
+       spin_lock_bh(&data->lock);
        tuple = ct->tuplehash[0].tuple;
        hash = &data->iphash[ipt_iphash(addr & mask)];

@@ -64,8 +64,8 @@
                struct ip_conntrack *found_ct = NULL;
                conn = list_entry(lh,struct ipt_connlimit_conn,list);
                found = ip_conntrack_find_get(&conn->tuple,ct);
-               if (0 == memcmp(&conn->tuple,&tuple,sizeof(tuple)) &&
-                   found != NULL && (found_ct = tuplehash_to_ctrack(found)) != NULL &&
+               if (found != NULL && (found_ct = tuplehash_to_ctrack(found)) != NULL &&
+                   0 == memcmp(&conn->tuple,&tuple,sizeof(tuple)) &&
                    found_ct->proto.tcp.state != TCP_CONNTRACK_TIME_WAIT) {
                        /* Just to be sure we have it only once in the list.
                           We should'nt see tuples twice unless someone hooks this

[-- Attachment #3: BitDefender.txt --]
[-- Type: text/plain, Size: 131 bytes --]


-- 
This message was scanned for spam and viruses by BitDefender.
For more information please visit http://linux.bitdefender.com/

Re: FIX: connlimit NULL pointer kernel panic (was: connlimit patch crashes 2.6.11 kernel)

[Pablo Neira]

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

Forte Systems - Iosif Peterfi wrote:
> Here is a diff -urN patch for what has been discussed except the conntrack
> event-api. Works fine on 2.6.1-gentoo-r8, patch-o-matic-ng-20050516. 

Hm, didn't I also tell you that you have to sed 
's/spin_unlock/spin_unlock_bh'?

well, it doesn't matter, attached the correct patch that applies cleanly 
to pom-ng.

--
Pablo

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

Index: linux-2.6.11/net/ipv4/netfilter/ipt_connlimit.c
===================================================================
--- linux-2.6.11/net/ipv4/netfilter/ipt_connlimit.c	(revision 3922)
+++ linux-2.6.11/net/ipv4/netfilter/ipt_connlimit.c	(working copy)
@@ -55,7 +55,7 @@
 	struct ipt_connlimit_conn *conn;
 	struct list_head *hash,*lh;
 
-	spin_lock(&data->lock);
+	spin_lock_bh(&data->lock);
 	tuple = ct->tuplehash[0].tuple;
 	hash = &data->iphash[ipt_iphash(addr & mask)];
 
@@ -64,9 +64,10 @@
 		struct ip_conntrack *found_ct = NULL;
 		conn = list_entry(lh,struct ipt_connlimit_conn,list);
 		found = ip_conntrack_find_get(&conn->tuple,ct);
-		if (0 == memcmp(&conn->tuple,&tuple,sizeof(tuple)) &&
-		    found != NULL && (found_ct = tuplehash_to_ctrack(found)) != NULL &&
-		    found_ct->proto.tcp.state != TCP_CONNTRACK_TIME_WAIT) {
+		 if (found != NULL 
+		     && (found_ct = tuplehash_to_ctrack(found)) != NULL
+		     && 0 == memcmp(&conn->tuple,&tuple,sizeof(tuple)) 
+		     && found_ct->proto.tcp.state != TCP_CONNTRACK_TIME_WAIT) {
 			/* Just to be sure we have it only once in the list.
 			   We should'nt see tuples twice unless someone hooks this
 			   into a table without "-p tcp --syn" */
@@ -111,7 +112,7 @@
 #endif
 		conn = kmalloc(sizeof(*conn),GFP_ATOMIC);
 		if (NULL == conn) {
-			spin_unlock(&data->lock);
+			spin_unlock_bh(&data->lock);
 			return -1;
 		}
 		memset(conn,0,sizeof(*conn));
@@ -120,7 +121,7 @@
 		list_add(&conn->list,hash);
 		matches++;
 	}
-	spin_unlock(&data->lock);
+	spin_unlock_bh(&data->lock);
 	return matches;
 }