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

netfilter: nfnl_cthelper: reject del request if helper obj is in use

We can still delete the ct helper even if it is in use, this will cause
a use-after-free error. In more detail, I mean:
  # nfct helper add ssdp inet udp
  # iptables -t raw -A OUTPUT -p udp -j CT --helper ssdp
  # nfct helper delete ssdp //--> oops, succeed!
  BUG: unable to handle kernel paging request at 000026ca
  IP: 0x26ca
  [...]
  Call Trace:
   ? ipv4_helper+0x62/0x80 [nf_conntrack_ipv4]
   nf_hook_slow+0x21/0xb0
   ip_output+0xe9/0x100
   ? ip_fragment.constprop.54+0xc0/0xc0
   ip_local_out+0x33/0x40
   ip_send_skb+0x16/0x80
   udp_send_skb+0x84/0x240
   udp_sendmsg+0x35d/0xa50

So add reference count to fix this issue, if ct helper is used by
others, reject the delete request.

Apply this patch:
  # nfct helper delete ssdp
  nfct v1.4.3: netlink error: Device or resource busy
Signed-off-by: default avatarLiping Zhang <zlpnobody@gmail.com>
Signed-off-by: default avatarPablo Neira Ayuso <pablo@netfilter.org>
parent d91fc59c
...@@ -9,6 +9,7 @@ ...@@ -9,6 +9,7 @@
#ifndef _NF_CONNTRACK_HELPER_H #ifndef _NF_CONNTRACK_HELPER_H
#define _NF_CONNTRACK_HELPER_H #define _NF_CONNTRACK_HELPER_H
#include <linux/refcount.h>
#include <net/netfilter/nf_conntrack.h> #include <net/netfilter/nf_conntrack.h>
#include <net/netfilter/nf_conntrack_extend.h> #include <net/netfilter/nf_conntrack_extend.h>
#include <net/netfilter/nf_conntrack_expect.h> #include <net/netfilter/nf_conntrack_expect.h>
...@@ -26,6 +27,7 @@ struct nf_conntrack_helper { ...@@ -26,6 +27,7 @@ struct nf_conntrack_helper {
struct hlist_node hnode; /* Internal use. */ struct hlist_node hnode; /* Internal use. */
char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */ char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */
refcount_t refcnt;
struct module *me; /* pointer to self */ struct module *me; /* pointer to self */
const struct nf_conntrack_expect_policy *expect_policy; const struct nf_conntrack_expect_policy *expect_policy;
......
...@@ -174,6 +174,10 @@ nf_conntrack_helper_try_module_get(const char *name, u16 l3num, u8 protonum) ...@@ -174,6 +174,10 @@ nf_conntrack_helper_try_module_get(const char *name, u16 l3num, u8 protonum)
#endif #endif
if (h != NULL && !try_module_get(h->me)) if (h != NULL && !try_module_get(h->me))
h = NULL; h = NULL;
if (h != NULL && !refcount_inc_not_zero(&h->refcnt)) {
module_put(h->me);
h = NULL;
}
rcu_read_unlock(); rcu_read_unlock();
...@@ -183,6 +187,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_helper_try_module_get); ...@@ -183,6 +187,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_helper_try_module_get);
void nf_conntrack_helper_put(struct nf_conntrack_helper *helper) void nf_conntrack_helper_put(struct nf_conntrack_helper *helper)
{ {
refcount_dec(&helper->refcnt);
module_put(helper->me); module_put(helper->me);
} }
EXPORT_SYMBOL_GPL(nf_conntrack_helper_put); EXPORT_SYMBOL_GPL(nf_conntrack_helper_put);
...@@ -423,6 +428,7 @@ int nf_conntrack_helper_register(struct nf_conntrack_helper *me) ...@@ -423,6 +428,7 @@ int nf_conntrack_helper_register(struct nf_conntrack_helper *me)
} }
} }
} }
refcount_set(&me->refcnt, 1);
hlist_add_head_rcu(&me->hnode, &nf_ct_helper_hash[h]); hlist_add_head_rcu(&me->hnode, &nf_ct_helper_hash[h]);
nf_ct_helper_count++; nf_ct_helper_count++;
out: out:
......
...@@ -686,6 +686,7 @@ static int nfnl_cthelper_del(struct net *net, struct sock *nfnl, ...@@ -686,6 +686,7 @@ static int nfnl_cthelper_del(struct net *net, struct sock *nfnl,
tuple_set = true; tuple_set = true;
} }
ret = -ENOENT;
list_for_each_entry_safe(nlcth, n, &nfnl_cthelper_list, list) { list_for_each_entry_safe(nlcth, n, &nfnl_cthelper_list, list) {
cur = &nlcth->helper; cur = &nlcth->helper;
j++; j++;
...@@ -699,16 +700,20 @@ static int nfnl_cthelper_del(struct net *net, struct sock *nfnl, ...@@ -699,16 +700,20 @@ static int nfnl_cthelper_del(struct net *net, struct sock *nfnl,
tuple.dst.protonum != cur->tuple.dst.protonum)) tuple.dst.protonum != cur->tuple.dst.protonum))
continue; continue;
found = true; if (refcount_dec_if_one(&cur->refcnt)) {
nf_conntrack_helper_unregister(cur); found = true;
kfree(cur->expect_policy); nf_conntrack_helper_unregister(cur);
kfree(cur->expect_policy);
list_del(&nlcth->list); list_del(&nlcth->list);
kfree(nlcth); kfree(nlcth);
} else {
ret = -EBUSY;
}
} }
/* Make sure we return success if we flush and there is no helpers */ /* Make sure we return success if we flush and there is no helpers */
return (found || j == 0) ? 0 : -ENOENT; return (found || j == 0) ? 0 : ret;
} }
static const struct nla_policy nfnl_cthelper_policy[NFCTH_MAX+1] = { static const struct nla_policy nfnl_cthelper_policy[NFCTH_MAX+1] = {
......
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