From c5f95710c2e70dc84ecdbca786ac83415f24dc79 Mon Sep 17 00:00:00 2001 From: Patrick McHardy Date: Wed, 16 May 2007 18:56:11 +0200 Subject: [PATCH] [PATCH] 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 . Signed-off-by: Patrick McHardy Signed-off-by: Chris Wright --- 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 23b99ae2cc3..75bd597f8bb 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 b3a70eb6d42..ce28fdd4218 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. -- 2.11.4.GIT