Commit b75911b6 authored by Liping Zhang's avatar Liping Zhang Committed by Pablo Neira Ayuso

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: default avatarLiping Zhang <liping.zhang@spreadtrum.com>
Signed-off-by: default avatarPablo Neira Ayuso <pablo@netfilter.org>
parent 12be15dd
...@@ -330,16 +330,16 @@ static int ctnl_timeout_try_del(struct net *net, struct ctnl_timeout *timeout) ...@@ -330,16 +330,16 @@ static int ctnl_timeout_try_del(struct net *net, struct ctnl_timeout *timeout)
{ {
int ret = 0; int ret = 0;
/* we want to avoid races with nf_ct_timeout_find_get. */ /* We want to avoid races with ctnl_timeout_put. So only when the
if (atomic_dec_and_test(&timeout->refcnt)) { * current refcnt is 1, we decrease it to 0.
*/
if (atomic_cmpxchg(&timeout->refcnt, 1, 0) == 1) {
/* We are protected by nfnl mutex. */ /* We are protected by nfnl mutex. */
list_del_rcu(&timeout->head); list_del_rcu(&timeout->head);
nf_ct_l4proto_put(timeout->l4proto); nf_ct_l4proto_put(timeout->l4proto);
ctnl_untimeout(net, timeout); ctnl_untimeout(net, timeout);
kfree_rcu(timeout, rcu_head); kfree_rcu(timeout, rcu_head);
} else { } else {
/* still in use, restore reference counter. */
atomic_inc(&timeout->refcnt);
ret = -EBUSY; ret = -EBUSY;
} }
return ret; return ret;
...@@ -543,7 +543,9 @@ ctnl_timeout_find_get(struct net *net, const char *name) ...@@ -543,7 +543,9 @@ ctnl_timeout_find_get(struct net *net, const char *name)
static void ctnl_timeout_put(struct ctnl_timeout *timeout) 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); module_put(THIS_MODULE);
} }
#endif /* CONFIG_NF_CONNTRACK_TIMEOUT */ #endif /* CONFIG_NF_CONNTRACK_TIMEOUT */
...@@ -591,6 +593,8 @@ static void __net_exit cttimeout_net_exit(struct net *net) ...@@ -591,6 +593,8 @@ static void __net_exit cttimeout_net_exit(struct net *net)
list_for_each_entry_safe(cur, tmp, &net->nfct_timeout_list, head) { list_for_each_entry_safe(cur, tmp, &net->nfct_timeout_list, head) {
list_del_rcu(&cur->head); list_del_rcu(&cur->head);
nf_ct_l4proto_put(cur->l4proto); nf_ct_l4proto_put(cur->l4proto);
if (atomic_dec_and_test(&cur->refcnt))
kfree_rcu(cur, rcu_head); kfree_rcu(cur, rcu_head);
} }
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment