[PATCH nf 1/2] netfilter: nfnetlink_acct: fix race between nfacct del and xt_nfacct destroy

[PATCH nf 1/2] netfilter: nfnetlink_acct: fix race between nfacct del and xt_nfacct destroy

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

Suppose that we input the following commands at first:
  # nfacct add test
  # iptables -A INPUT -m nfacct --nfacct-name test

And now "test" acct's refcnt is 2, but later when we try to delete the
"test" nfacct and the related iptables rule at the same time, race maybe
happen:
      CPU0                                    CPU1
  nfnl_acct_try_del                      nfnl_acct_put
  atomic_dec_and_test //ref=1,testfail          -
       -                                 atomic_dec_and_test //ref=0,testok
       -                                 kfree_rcu
  atomic_inc //ref=1                            -

So after the rcu grace period, nf_acct will be freed but it is still linked
in the nfnl_acct_list, and we can access it later, then oops will happen.

Convert atomic_dec_and_test and atomic_inc combinaiton to one atomic
operation atomic_cmpxchg here to fix this problem.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/nfnetlink_acct.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 1b4de4b..1f15535 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -326,16 +326,18 @@ static int nfnl_acct_try_del(struct nf_acct *cur)
 {
 	int ret = 0;
 
-	/* we want to avoid races with nfnl_acct_find_get. */
-	if (atomic_dec_and_test(&cur->refcnt)) {
+	/*
+	 * We want to avoid races with nfnl_acct_put. So only when the current
+	 * refcnt is 1, we decrease it to 0.
+	 */
+	if (atomic_cmpxchg(&cur->refcnt, 1, 0) == 1) {
 		/* We are protected by nfnl mutex. */
 		list_del_rcu(&cur->head);
 		kfree_rcu(cur, rcu_head);
 	} else {
-		/* still in use, restore reference counter. */
-		atomic_inc(&cur->refcnt);
 		ret = -EBUSY;
 	}
+
 	return ret;
 }
 
-- 
2.5.5

[PATCH nf 2/2] netfilter: nfnetlink_acct: report overquota to the right netns

[Liping Zhang]

From: Liping Zhang <liping.zhang@spreadtrum.com>

We should report the over quota message to the right net namespace
instead of the init netns.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 include/linux/netfilter/nfnetlink_acct.h | 4 ++--
 net/netfilter/nfnetlink_acct.c           | 9 +++++----
 net/netfilter/xt_nfacct.c                | 2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/linux/netfilter/nfnetlink_acct.h b/include/linux/netfilter/nfnetlink_acct.h
index 80ca889..664da00 100644
--- a/include/linux/netfilter/nfnetlink_acct.h
+++ b/include/linux/netfilter/nfnetlink_acct.h
@@ -15,6 +15,6 @@ struct nf_acct;
 struct nf_acct *nfnl_acct_find_get(struct net *net, const char *filter_name);
 void nfnl_acct_put(struct nf_acct *acct);
 void nfnl_acct_update(const struct sk_buff *skb, struct nf_acct *nfacct);
-extern int nfnl_acct_overquota(const struct sk_buff *skb,
-			      struct nf_acct *nfacct);
+int nfnl_acct_overquota(struct net *net, const struct sk_buff *skb,
+			struct nf_acct *nfacct);
 #endif /* _NFNL_ACCT_H */
diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 1f15535..216b853 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -445,7 +445,7 @@ void nfnl_acct_update(const struct sk_buff *skb, struct nf_acct *nfacct)
 }
 EXPORT_SYMBOL_GPL(nfnl_acct_update);
 
-static void nfnl_overquota_report(struct nf_acct *nfacct)
+static void nfnl_overquota_report(struct net *net, struct nf_acct *nfacct)
 {
 	int ret;
 	struct sk_buff *skb;
@@ -460,11 +460,12 @@ static void nfnl_overquota_report(struct nf_acct *nfacct)
 		kfree_skb(skb);
 		return;
 	}
-	netlink_broadcast(init_net.nfnl, skb, 0, NFNLGRP_ACCT_QUOTA,
+	netlink_broadcast(net->nfnl, skb, 0, NFNLGRP_ACCT_QUOTA,
 			  GFP_ATOMIC);
 }
 
-int nfnl_acct_overquota(const struct sk_buff *skb, struct nf_acct *nfacct)
+int nfnl_acct_overquota(struct net *net, const struct sk_buff *skb,
+			struct nf_acct *nfacct)
 {
 	u64 now;
 	u64 *quota;
@@ -482,7 +483,7 @@ int nfnl_acct_overquota(const struct sk_buff *skb, struct nf_acct *nfacct)
 
 	if (now >= *quota &&
 	    !test_and_set_bit(NFACCT_OVERQUOTA_BIT, &nfacct->flags)) {
-		nfnl_overquota_report(nfacct);
+		nfnl_overquota_report(net, nfacct);
 	}
 
 	return ret;
diff --git a/net/netfilter/xt_nfacct.c b/net/netfilter/xt_nfacct.c
index 3048a7e..cf32759 100644
--- a/net/netfilter/xt_nfacct.c
+++ b/net/netfilter/xt_nfacct.c
@@ -26,7 +26,7 @@ static bool nfacct_mt(const struct sk_buff *skb, struct xt_action_param *par)
 
 	nfnl_acct_update(skb, info->nfacct);
 
-	overquota = nfnl_acct_overquota(skb, info->nfacct);
+	overquota = nfnl_acct_overquota(par->net, skb, info->nfacct);
 
 	return overquota == NFACCT_UNDERQUOTA ? false : true;
 }
-- 
2.5.5

Re: [PATCH nf 2/2] netfilter: nfnetlink_acct: report overquota to the right netns

[Pablo Neira Ayuso]

On Sat, Aug 13, 2016 at 11:13:02PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> We should report the over quota message to the right net namespace
> instead of the init netns.

Also applied, thanks.

Re: [PATCH nf 1/2] netfilter: nfnetlink_acct: fix race between nfacct del and xt_nfacct destroy

[Pablo Neira Ayuso]

On Sat, Aug 13, 2016 at 11:13:01PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
> 
> Suppose that we input the following commands at first:
>   # nfacct add test
>   # iptables -A INPUT -m nfacct --nfacct-name test
> 
> And now "test" acct's refcnt is 2, but later when we try to delete the
> "test" nfacct and the related iptables rule at the same time, race maybe
> happen:
>       CPU0                                    CPU1
>   nfnl_acct_try_del                      nfnl_acct_put
>   atomic_dec_and_test //ref=1,testfail          -
>        -                                 atomic_dec_and_test //ref=0,testok
>        -                                 kfree_rcu
>   atomic_inc //ref=1                            -
> 
> So after the rcu grace period, nf_acct will be freed but it is still linked
> in the nfnl_acct_list, and we can access it later, then oops will happen.
> 
> Convert atomic_dec_and_test and atomic_inc combinaiton to one atomic
> operation atomic_cmpxchg here to fix this problem.

Applied, thanks.

Re: [PATCH nf 1/2] netfilter: nfnetlink_acct: fix race between nfacct del and xt_nfacct destroy

[Pablo Neira Ayuso]

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

On Thu, Aug 18, 2016 at 12:26:34AM +0200, Pablo Neira Ayuso wrote:
> On Sat, Aug 13, 2016 at 11:13:01PM +0800, Liping Zhang wrote:
> > From: Liping Zhang <liping.zhang@spreadtrum.com>
> > 
> > Suppose that we input the following commands at first:
> >   # nfacct add test
> >   # iptables -A INPUT -m nfacct --nfacct-name test
> > 
> > And now "test" acct's refcnt is 2, but later when we try to delete the
> > "test" nfacct and the related iptables rule at the same time, race maybe
> > happen:
> >       CPU0                                    CPU1
> >   nfnl_acct_try_del                      nfnl_acct_put
> >   atomic_dec_and_test //ref=1,testfail          -
> >        -                                 atomic_dec_and_test //ref=0,testok
> >        -                                 kfree_rcu
> >   atomic_inc //ref=1                            -
> > 
> > So after the rcu grace period, nf_acct will be freed but it is still linked
> > in the nfnl_acct_list, and we can access it later, then oops will happen.
> > 
> > Convert atomic_dec_and_test and atomic_inc combinaiton to one atomic
> > operation atomic_cmpxchg here to fix this problem.
> 
> Applied, thanks.

Wait. I noticed we have the same problem in cttimeout, so it would be
good to fix this in the same logical change.

I'm attaching your original patch that I have mangled here, including
the cttimeout chunk.

Let me know if you have any concern, otherwise I'll apply this new
version, thanks!

[-- Attachment #2: 0001-netfilter-nfnetlink-fix-race-between-netlink-deletio.patch --]
[-- Type: text/x-diff, Size: 3100 bytes --]

>From b6651665ee7c533bfe4a10060dcd9aee92c80cf6 Mon Sep 17 00:00:00 2001
From: Liping Zhang <liping.zhang@spreadtrum.com>
Date: Sat, 13 Aug 2016 23:13:01 +0800
Subject: [PATCH] netfilter: nfnetlink: fix race between netlink deletion and
 refcnt put via xtables

Suppose that we input the following commands at first:
  # nfacct add test
  # iptables -A INPUT -m nfacct --nfacct-name test

And now "test" acct's refcnt is 2, but later when we try to delete the
"test" nfacct and the related iptables rule at the same time, race maybe
happen:
      CPU0                                    CPU1
  nfnl_acct_try_del                      nfnl_acct_put
  atomic_dec_and_test //ref=1,testfail          -
       -                                 atomic_dec_and_test //ref=0,testok
       -                                 kfree_rcu
  atomic_inc //ref=1                            -

So after the rcu grace period, nf_acct will be freed but it is still linked
in the nfnl_acct_list, and we can access it later, then oops will happen.

Convert atomic_dec_and_test and atomic_inc combinaiton to one atomic
operation atomic_cmpxchg here to fix this problem.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nfnetlink_acct.c      | 8 ++++----
 net/netfilter/nfnetlink_cttimeout.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 1b4de4b..54c7d5b 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -326,14 +326,14 @@ static int nfnl_acct_try_del(struct nf_acct *cur)
 {
 	int ret = 0;
 
-	/* we want to avoid races with nfnl_acct_find_get. */
-	if (atomic_dec_and_test(&cur->refcnt)) {
+	/* We want to avoid races with nfnl_acct_put. So only when the current
+	 * refcnt is 1, we decrease it to 0.
+	 */
+	if (atomic_cmpxchg(&cur->refcnt, 1, 0) == 1) {
 		/* We are protected by nfnl mutex. */
 		list_del_rcu(&cur->head);
 		kfree_rcu(cur, rcu_head);
 	} else {
-		/* still in use, restore reference counter. */
-		atomic_inc(&cur->refcnt);
 		ret = -EBUSY;
 	}
 	return ret;
diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
index 4cdcd96..77c0178 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -330,16 +330,16 @@ static int ctnl_timeout_try_del(struct net *net, struct ctnl_timeout *timeout)
 {
 	int ret = 0;
 
-	/* we want to avoid races with nf_ct_timeout_find_get. */
-	if (atomic_dec_and_test(&timeout->refcnt)) {
+	/* We want to avoid races with ctnl_timeout_put. So only when the
+	 * current refcnt is 1, we decrease it to 0.
+	 */
+	if (atomic_cmpxchg(&timeout->refcnt, 1, 0) == 1) {
 		/* We are protected by nfnl mutex. */
 		list_del_rcu(&timeout->head);
 		nf_ct_l4proto_put(timeout->l4proto);
 		ctnl_untimeout(net, timeout);
 		kfree_rcu(timeout, rcu_head);
 	} else {
-		/* still in use, restore reference counter. */
-		atomic_inc(&timeout->refcnt);
 		ret = -EBUSY;
 	}
 	return ret;
-- 
2.1.4

Re: [PATCH nf 1/2] netfilter: nfnetlink_acct: fix race between nfacct del and xt_nfacct destroy

[Liping Zhang]

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

Hi Pablo,

2016-08-18 6:37 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
>
> Wait. I noticed we have the same problem in cttimeout, so it would be
> good to fix this in the same logical change.
>
> I'm attaching your original patch that I have mangled here, including
> the cttimeout chunk.
>
> Let me know if you have any concern, otherwise I'll apply this new
> version, thanks!

Not exactly right.

Currently ctnl_timeout_try_del will not cause race with ctnl_timeout_put.
In ctnl_timeout_put, it only decreases the timeout object's refcnt,
but will not try to free it. This is different with nfnetlink_acct.

But when we do "ip netns del xxx", this will cause a use after free error.
In general, when we delete a netns, cttimeout_net_exit will be called
before ipt_unregister_table, i.e. before ctnl_timeout_put.

So after call kfree_rcu in cttimeout_net_exit, we will still decrease
the timeout object's refcnt in ctnl_timeout_put. Kernel will complain
about this:

  =======================================================================
  BUG kmalloc-96 (Tainted: G    B       E  ): Poison overwritten
  -----------------------------------------------------------------------
  INFO: 0xffff88002b5161e8-0xffff88002b5161e8. First byte 0x6a instead of
  0x6b
  INFO: Allocated in cttimeout_new_timeout+0xd4/0x240 [nfnetlink_cttimeout]
  age=104 cpu=0 pid=3330
  ___slab_alloc+0x4da/0x540
  __slab_alloc+0x20/0x40
  __kmalloc+0x1c8/0x240
  cttimeout_new_timeout+0xd4/0x240 [nfnetlink_cttimeout]
  nfnetlink_rcv_msg+0x21a/0x230 [nfnetlink]

So I think it seems better that we take another patch to fix the
problem in cttimeout.

Attachment is my patch.

[-- Attachment #2: 0001-netfilter-cttimeout-fix-use-after-free-error-when-de.patch --]
[-- Type: application/octet-stream, Size: 3282 bytes --]

From d3bb48f73914df831516f99d775b454259a4058d Mon Sep 17 00:00:00 2001
From: Liping Zhang <liping.zhang@spreadtrum.com>
Date: Thu, 18 Aug 2016 16:58:46 +0800
Subject: [PATCH nf] netfilter: cttimeout: fix use after free error when delete
 netns

In general, when we want to delete a netns, cttimeout_net_exit will
be called before ipt_unregister_table, i.e. before ctnl_timeout_put.

But after call kfree_rcu in cttimeout_net_exit, we will still decrease
the timeout object's refcnt in ctnl_timeout_put, this is incorrect,
and will cause a use after free error.

It is easy to reproduce this problem:
  # while : ; do
  ip netns add xxx
  ip netns exec xxx nfct add timeout testx inet icmp timeout 200
  ip netns exec xxx iptables -t raw -p icmp -I OUTPUT -j CT --timeout testx
  ip netns del xxx
  done

  =======================================================================
  BUG kmalloc-96 (Tainted: G    B       E  ): Poison overwritten
  -----------------------------------------------------------------------
  INFO: 0xffff88002b5161e8-0xffff88002b5161e8. First byte 0x6a instead of
  0x6b
  INFO: Allocated in cttimeout_new_timeout+0xd4/0x240 [nfnetlink_cttimeout]
  age=104 cpu=0 pid=3330
  ___slab_alloc+0x4da/0x540
  __slab_alloc+0x20/0x40
  __kmalloc+0x1c8/0x240
  cttimeout_new_timeout+0xd4/0x240 [nfnetlink_cttimeout]
  nfnetlink_rcv_msg+0x21a/0x230 [nfnetlink]
  [ ... ]

So only when the refcnt decreased to 0, we call kfree_rcu to free the
timeout object. And like nfnetlink_acct do, use atomic_cmpxchg to
avoid race between ctnl_timeout_try_del and ctnl_timeout_put.

Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
 net/netfilter/nfnetlink_cttimeout.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
index 4cdcd96..68216cd 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -330,16 +330,16 @@ static int ctnl_timeout_try_del(struct net *net, struct ctnl_timeout *timeout)
 {
 	int ret = 0;
 
-	/* we want to avoid races with nf_ct_timeout_find_get. */
-	if (atomic_dec_and_test(&timeout->refcnt)) {
+	/* We want to avoid races with ctnl_timeout_put. So only when the
+	 * current refcnt is 1, we decrease it to 0.
+	 */
+	if (atomic_cmpxchg(&timeout->refcnt, 1, 0) == 1) {
 		/* We are protected by nfnl mutex. */
 		list_del_rcu(&timeout->head);
 		nf_ct_l4proto_put(timeout->l4proto);
 		ctnl_untimeout(net, timeout);
 		kfree_rcu(timeout, rcu_head);
 	} else {
-		/* still in use, restore reference counter. */
-		atomic_inc(&timeout->refcnt);
 		ret = -EBUSY;
 	}
 	return ret;
@@ -543,7 +543,9 @@ err:
 
 static void ctnl_timeout_put(struct ctnl_timeout *timeout)
 {
-	atomic_dec(&timeout->refcnt);
+	if (atomic_dec_and_test(&timeout->refcnt))
+		kfree_rcu(timeout, rcu_head);
+
 	module_put(THIS_MODULE);
 }
 #endif /* CONFIG_NF_CONNTRACK_TIMEOUT */
@@ -591,7 +593,9 @@ static void __net_exit cttimeout_net_exit(struct net *net)
 	list_for_each_entry_safe(cur, tmp, &net->nfct_timeout_list, head) {
 		list_del_rcu(&cur->head);
 		nf_ct_l4proto_put(cur->l4proto);
-		kfree_rcu(cur, rcu_head);
+
+		if (atomic_dec_and_test(&cur->refcnt))
+			kfree_rcu(cur, rcu_head);
 	}
 }
 
-- 
2.5.5

Re: [PATCH nf 1/2] netfilter: nfnetlink_acct: fix race between nfacct del and xt_nfacct destroy

[Pablo Neira Ayuso]

On Thu, Aug 18, 2016 at 06:41:12PM +0800, Liping Zhang wrote:
> So I think it seems better that we take another patch to fix the
> problem in cttimeout.

Fine with me, will take your patch, thanks!