[PATCH nf] netfilter: conntrack: use nf_ct_tmpl_free in CT/synproxy error paths

[PATCH nf] netfilter: conntrack: use nf_ct_tmpl_free in CT/synproxy error paths

[Daniel Borkmann]

Commit 0838aa7fcfcd ("netfilter: fix netns dependencies with conntrack
templates") migrated templates to the new allocator api, but forgot to
update error paths for them in CT and synproxy to use nf_ct_tmpl_free()
instead of nf_conntrack_free().

Due to that, memory is being freed into the wrong kmemcache, but also
we drop the per net reference count of ct objects causing an imbalance.

In Brad's case, this leads to a wrap-around of net->ct.count and thus
lets __nf_conntrack_alloc() refuse to create a new ct object:

  [   10.340913] xt_addrtype: ipv6 does not support BROADCAST matching
  [   10.810168] nf_conntrack: table full, dropping packet
  [   11.917416] r8169 0000:07:00.0 eth0: link up
  [   11.917438] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
  [   12.815902] nf_conntrack: table full, dropping packet
  [   15.688561] nf_conntrack: table full, dropping packet
  [   15.689365] nf_conntrack: table full, dropping packet
  [   15.690169] nf_conntrack: table full, dropping packet
  [   15.690967] nf_conntrack: table full, dropping packet
  [...]

With slab debugging, it also reports the wrong kmemcache (kmalloc-512 vs.
nf_conntrack_ffffffff81ce75c0) and reports poison overwrites, etc. Thus,
to fix the problem, export and use nf_ct_tmpl_free() instead.

Fixes: 0838aa7fcfcd ("netfilter: fix netns dependencies with conntrack templates")
Reported-by: Brad Jackson <bjackson0971@gmail.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 include/net/netfilter/nf_conntrack.h | 1 +
 net/netfilter/nf_conntrack_core.c    | 3 ++-
 net/netfilter/nf_synproxy_core.c     | 2 +-
 net/netfilter/xt_CT.c                | 2 +-
 4 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/net/netfilter/nf_conntrack.h b/include/net/netfilter/nf_conntrack.h
index 37cd391..4023c4c 100644
--- a/include/net/netfilter/nf_conntrack.h
+++ b/include/net/netfilter/nf_conntrack.h
@@ -292,6 +292,7 @@ extern unsigned int nf_conntrack_hash_rnd;
 void init_nf_conntrack_hash_rnd(void);
 
 struct nf_conn *nf_ct_tmpl_alloc(struct net *net, u16 zone, gfp_t flags);
+void nf_ct_tmpl_free(struct nf_conn *tmpl);
 
 #define NF_CT_STAT_INC(net, count)	  __this_cpu_inc((net)->ct.stat->count)
 #define NF_CT_STAT_INC_ATOMIC(net, count) this_cpu_inc((net)->ct.stat->count)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index 3c20d02..0625a42 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -320,12 +320,13 @@ out_free:
 }
 EXPORT_SYMBOL_GPL(nf_ct_tmpl_alloc);
 
-static void nf_ct_tmpl_free(struct nf_conn *tmpl)
+void nf_ct_tmpl_free(struct nf_conn *tmpl)
 {
 	nf_ct_ext_destroy(tmpl);
 	nf_ct_ext_free(tmpl);
 	kfree(tmpl);
 }
+EXPORT_SYMBOL_GPL(nf_ct_tmpl_free);
 
 static void
 destroy_conntrack(struct nf_conntrack *nfct)
diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
index d7f1685..d6ee8f8 100644
--- a/net/netfilter/nf_synproxy_core.c
+++ b/net/netfilter/nf_synproxy_core.c
@@ -378,7 +378,7 @@ static int __net_init synproxy_net_init(struct net *net)
 err3:
 	free_percpu(snet->stats);
 err2:
-	nf_conntrack_free(ct);
+	nf_ct_tmpl_free(ct);
 err1:
 	return err;
 }
diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 43ddeee..f3377ce 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -233,7 +233,7 @@ out:
 	return 0;
 
 err3:
-	nf_conntrack_free(ct);
+	nf_ct_tmpl_free(ct);
 err2:
 	nf_ct_l3proto_module_put(par->family);
 err1:
-- 
1.9.3

Re: [PATCH nf] netfilter: conntrack: use nf_ct_tmpl_free in CT/synproxy error paths

[Pablo Neira Ayuso]

On Mon, Aug 31, 2015 at 07:11:02PM +0200, Daniel Borkmann wrote:
> Commit 0838aa7fcfcd ("netfilter: fix netns dependencies with conntrack
> templates") migrated templates to the new allocator api, but forgot to
> update error paths for them in CT and synproxy to use nf_ct_tmpl_free()
> instead of nf_conntrack_free().
> 
> Due to that, memory is being freed into the wrong kmemcache, but also
> we drop the per net reference count of ct objects causing an imbalance.
> 
> In Brad's case, this leads to a wrap-around of net->ct.count and thus
> lets __nf_conntrack_alloc() refuse to create a new ct object:
> 
>   [   10.340913] xt_addrtype: ipv6 does not support BROADCAST matching
>   [   10.810168] nf_conntrack: table full, dropping packet
>   [   11.917416] r8169 0000:07:00.0 eth0: link up
>   [   11.917438] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
>   [   12.815902] nf_conntrack: table full, dropping packet
>   [   15.688561] nf_conntrack: table full, dropping packet
>   [   15.689365] nf_conntrack: table full, dropping packet
>   [   15.690169] nf_conntrack: table full, dropping packet
>   [   15.690967] nf_conntrack: table full, dropping packet
>   [...]
> 
> With slab debugging, it also reports the wrong kmemcache (kmalloc-512 vs.
> nf_conntrack_ffffffff81ce75c0) and reports poison overwrites, etc. Thus,
> to fix the problem, export and use nf_ct_tmpl_free() instead.

Applied, thanks Daniel.