[NETFILTER -stable]: {ip, nf}_conntrack: fix use-after-free in helper destroy callback invocation

[NETFILTER -stable]: {ip, nf}_conntrack: fix use-after-free in helper destroy callback invocation

[Patrick McHardy]

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

This patch for stable-2.6.21 fixes an oops when unloading conntrack
helper modules.

Please apply, thanks.

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

[NETFILTER]: {ip,nf}_conntrack: fix use-after-free in helper destroy callback invocation

When the helper module is removed for a master connection that has a
fulfilled expectation, but has already timed out and got removed from
the hash tables, nf_conntrack_helper_unregister can't find the master
connection to unset the helper, causing a use-after-free when the
expected connection is destroyed and releases the last reference to
the master.

The helper destroy callback was introduced for the PPtP helper to clean
up expectations and expected connections when the master connection
times out, but doing this from destroy_conntrack only works for
unfulfilled expectations since expected connections hold a reference
to the master, preventing its destruction. Move the destroy callback to
the timeout function, which fixes both problems.

Reported/tested by Gabor Burjan <buga@buvoshetes.hu>.

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

---
commit 441f15ce23ef5c4d149b7e7985f63c1ddd334c45
tree 8783e067803def0fc2773ef3515190143ac47320
parent 8d8b10482fffcb72b15515231bb942e2ad6395c9
author Patrick McHardy <kaber@trash.net> Wed, 16 May 2007 18:52:36 +0200
committer Patrick McHardy <kaber@trash.net> Wed, 16 May 2007 18:52:36 +0200

 net/ipv4/netfilter/ip_conntrack_core.c |   10 +++++-----
 net/netfilter/nf_conntrack_core.c      |    8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/netfilter/ip_conntrack_core.c b/net/ipv4/netfilter/ip_conntrack_core.c
index 23b99ae..75bd597 100644
--- a/net/ipv4/netfilter/ip_conntrack_core.c
+++ b/net/ipv4/netfilter/ip_conntrack_core.c
@@ -302,7 +302,6 @@ destroy_conntrack(struct nf_conntrack *nfct)
 {
 	struct ip_conntrack *ct = (struct ip_conntrack *)nfct;
 	struct ip_conntrack_protocol *proto;
-	struct ip_conntrack_helper *helper;
 	typeof(ip_conntrack_destroyed) destroyed;
 
 	DEBUGP("destroy_conntrack(%p)\n", ct);
@@ -312,10 +311,6 @@ destroy_conntrack(struct nf_conntrack *nfct)
 	ip_conntrack_event(IPCT_DESTROY, ct);
 	set_bit(IPS_DYING_BIT, &ct->status);
 
-	helper = ct->helper;
-	if (helper && helper->destroy)
-		helper->destroy(ct);
-
 	/* To make sure we don't get any weird locking issues here:
 	 * destroy_conntrack() MUST NOT be called with a write lock
 	 * to ip_conntrack_lock!!! -HW */
@@ -356,6 +351,11 @@ destroy_conntrack(struct nf_conntrack *nfct)
 static void death_by_timeout(unsigned long ul_conntrack)
 {
 	struct ip_conntrack *ct = (void *)ul_conntrack;
+	struct ip_conntrack_helper *helper;
+
+	helper = ct->helper;
+	if (helper && helper->destroy)
+		helper->destroy(ct);
 
 	write_lock_bh(&ip_conntrack_lock);
 	/* Inside lock so preempt is disabled on module removal path.
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index b3a70eb..ce28fdd 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -315,7 +315,6 @@ static void
 destroy_conntrack(struct nf_conntrack *nfct)
 {
 	struct nf_conn *ct = (struct nf_conn *)nfct;
-	struct nf_conn_help *help = nfct_help(ct);
 	struct nf_conntrack_l3proto *l3proto;
 	struct nf_conntrack_l4proto *l4proto;
 	typeof(nf_conntrack_destroyed) destroyed;
@@ -327,9 +326,6 @@ destroy_conntrack(struct nf_conntrack *nfct)
 	nf_conntrack_event(IPCT_DESTROY, ct);
 	set_bit(IPS_DYING_BIT, &ct->status);
 
-	if (help && help->helper && help->helper->destroy)
-		help->helper->destroy(ct);
-
 	/* To make sure we don't get any weird locking issues here:
 	 * destroy_conntrack() MUST NOT be called with a write lock
 	 * to nf_conntrack_lock!!! -HW */
@@ -375,6 +371,10 @@ destroy_conntrack(struct nf_conntrack *nfct)
 static void death_by_timeout(unsigned long ul_conntrack)
 {
 	struct nf_conn *ct = (void *)ul_conntrack;
+	struct nf_conn_help *help = nfct_help(ct);
+
+	if (help && help->helper && help->helper->destroy)
+		help->helper->destroy(ct);
 
 	write_lock_bh(&nf_conntrack_lock);
 	/* Inside lock so preempt is disabled on module removal path.

patch netfilter-ip-nf-_conntrack-fix-use-after-free-in-helper-destroy-callback-invocation.patch queued to 2.6.21-stable tree

[chrisw]

This is a note to let you know that we have just queued up the patch titled

     Subject: NETFILTER: {ip,nf}_conntrack: fix use-after-free in helper destroy callback invocation

to the 2.6.21-stable tree.  Its filename is

     netfilter-ip-nf-_conntrack-fix-use-after-free-in-helper-destroy-callback-invocation.patch

A git repo of this tree can be found at 
    http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary


>From stable-bounces@linux.kernel.org  Wed May 16 09:58:34 2007
Message-ID: <464B37AB.5040802@trash.net>
Date: Wed, 16 May 2007 18:56:11 +0200
From: Patrick McHardy <kaber@trash.net>
To: stable@kernel.org
Cc: Netfilter Development Mailinglist <netfilter-devel@lists.netfilter.org>, "David S. Miller" <davem@davemloft.net>
Subject: NETFILTER: {ip,nf}_conntrack: fix use-after-free in helper destroy callback invocation

When the helper module is removed for a master connection that has a
fulfilled expectation, but has already timed out and got removed from
the hash tables, nf_conntrack_helper_unregister can't find the master
connection to unset the helper, causing a use-after-free when the
expected connection is destroyed and releases the last reference to
the master.

The helper destroy callback was introduced for the PPtP helper to clean
up expectations and expected connections when the master connection
times out, but doing this from destroy_conntrack only works for
unfulfilled expectations since expected connections hold a reference
to the master, preventing its destruction. Move the destroy callback to
the timeout function, which fixes both problems.

Reported/tested by Gabor Burjan <buga@buvoshetes.hu>.

Signed-off-by: Patrick McHardy <kaber@trash.net>
Signed-off-by: Chris Wright <chrisw@sous-sol.org>
---
commit 441f15ce23ef5c4d149b7e7985f63c1ddd334c45
tree 8783e067803def0fc2773ef3515190143ac47320
parent 8d8b10482fffcb72b15515231bb942e2ad6395c9
author Patrick McHardy <kaber@trash.net> Wed, 16 May 2007 18:52:36 +0200
committer Patrick McHardy <kaber@trash.net> Wed, 16 May 2007 18:52:36 +0200

 net/ipv4/netfilter/ip_conntrack_core.c |   10 +++++-----
 net/netfilter/nf_conntrack_core.c      |    8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

--- linux-2.6.21.1.orig/net/ipv4/netfilter/ip_conntrack_core.c
+++ linux-2.6.21.1/net/ipv4/netfilter/ip_conntrack_core.c
@@ -302,7 +302,6 @@ destroy_conntrack(struct nf_conntrack *n
 {
 	struct ip_conntrack *ct = (struct ip_conntrack *)nfct;
 	struct ip_conntrack_protocol *proto;
-	struct ip_conntrack_helper *helper;
 	typeof(ip_conntrack_destroyed) destroyed;
 
 	DEBUGP("destroy_conntrack(%p)\n", ct);
@@ -312,10 +311,6 @@ destroy_conntrack(struct nf_conntrack *n
 	ip_conntrack_event(IPCT_DESTROY, ct);
 	set_bit(IPS_DYING_BIT, &ct->status);
 
-	helper = ct->helper;
-	if (helper && helper->destroy)
-		helper->destroy(ct);
-
 	/* To make sure we don't get any weird locking issues here:
 	 * destroy_conntrack() MUST NOT be called with a write lock
 	 * to ip_conntrack_lock!!! -HW */
@@ -356,6 +351,11 @@ destroy_conntrack(struct nf_conntrack *n
 static void death_by_timeout(unsigned long ul_conntrack)
 {
 	struct ip_conntrack *ct = (void *)ul_conntrack;
+	struct ip_conntrack_helper *helper;
+
+	helper = ct->helper;
+	if (helper && helper->destroy)
+		helper->destroy(ct);
 
 	write_lock_bh(&ip_conntrack_lock);
 	/* Inside lock so preempt is disabled on module removal path.
--- linux-2.6.21.1.orig/net/netfilter/nf_conntrack_core.c
+++ linux-2.6.21.1/net/netfilter/nf_conntrack_core.c
@@ -315,7 +315,6 @@ static void
 destroy_conntrack(struct nf_conntrack *nfct)
 {
 	struct nf_conn *ct = (struct nf_conn *)nfct;
-	struct nf_conn_help *help = nfct_help(ct);
 	struct nf_conntrack_l3proto *l3proto;
 	struct nf_conntrack_l4proto *l4proto;
 	typeof(nf_conntrack_destroyed) destroyed;
@@ -327,9 +326,6 @@ destroy_conntrack(struct nf_conntrack *n
 	nf_conntrack_event(IPCT_DESTROY, ct);
 	set_bit(IPS_DYING_BIT, &ct->status);
 
-	if (help && help->helper && help->helper->destroy)
-		help->helper->destroy(ct);
-
 	/* To make sure we don't get any weird locking issues here:
 	 * destroy_conntrack() MUST NOT be called with a write lock
 	 * to nf_conntrack_lock!!! -HW */
@@ -375,6 +371,10 @@ destroy_conntrack(struct nf_conntrack *n
 static void death_by_timeout(unsigned long ul_conntrack)
 {
 	struct nf_conn *ct = (void *)ul_conntrack;
+	struct nf_conn_help *help = nfct_help(ct);
+
+	if (help && help->helper && help->helper->destroy)
+		help->helper->destroy(ct);
 
 	write_lock_bh(&nf_conntrack_lock);
 	/* Inside lock so preempt is disabled on module removal path.


Patches currently in stable-queue which might be from kaber@trash.net are

queue-2.6.21/ip-nf-_nat_proto_gre-do-not-modify-corrupt-grev0-packets-through-nat.patch
queue-2.6.21/netfilter-ip-nf-_conntrack-fix-use-after-free-in-helper-destroy-callback-invocation.patch