* [PATCH nf 0/2] netfilter: reject cthelper del request if it is in use @ 2017-04-09 8:22 Liping Zhang 2017-04-09 8:22 ` [PATCH nf 1/2] netfilter: introduce nf_conntrack_helper_put helper function Liping Zhang 2017-04-09 8:22 ` [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use Liping Zhang 0 siblings, 2 replies; 7+ messages in thread From: Liping Zhang @ 2017-04-09 8:22 UTC (permalink / raw) To: pablo; +Cc: netfilter-devel, Liping Zhang From: Liping Zhang <zlpnobody@gmail.com> User can still delete the cthelper even if it is in use: # nfct helper add ssdp inet udp # iptables -t raw -A OUTPUT -p udp -j CT --helper ssdp # nfct helper delete ssdp //--> succeed! This will cause a use-after-free error. So we shoule add a refcnt to fix this issue. Before accomplishing this, it's better to introduce a nf_conntrack_helper_put helper function. Note, this patch set is based on http://patchwork.ozlabs.org/patch/748533/. But I think it may still conflict with other patches. If so, I can rebase it. Liping Zhang (2): netfilter: introduce nf_conntrack_helper_put helper function netfilter: nfnl_cthelper: reject del request if helper obj is in use include/net/netfilter/nf_conntrack_helper.h | 4 ++++ net/netfilter/nf_conntrack_helper.c | 12 ++++++++++++ net/netfilter/nfnetlink_cthelper.c | 17 +++++++++++------ net/netfilter/xt_CT.c | 6 +++--- net/openvswitch/conntrack.c | 4 ++-- 5 files changed, 32 insertions(+), 11 deletions(-) -- 2.5.5 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH nf 1/2] netfilter: introduce nf_conntrack_helper_put helper function 2017-04-09 8:22 [PATCH nf 0/2] netfilter: reject cthelper del request if it is in use Liping Zhang @ 2017-04-09 8:22 ` Liping Zhang 2017-04-09 8:22 ` [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use Liping Zhang 1 sibling, 0 replies; 7+ messages in thread From: Liping Zhang @ 2017-04-09 8:22 UTC (permalink / raw) To: pablo; +Cc: netfilter-devel, Liping Zhang From: Liping Zhang <zlpnobody@gmail.com> And convert module_put invocation to nf_conntrack_helper_put, this is prepared for the later patch, which will add a refcnt for cthelper. Signed-off-by: Liping Zhang <zlpnobody@gmail.com> --- include/net/netfilter/nf_conntrack_helper.h | 2 ++ net/netfilter/nf_conntrack_helper.c | 6 ++++++ net/netfilter/xt_CT.c | 6 +++--- net/openvswitch/conntrack.c | 4 ++-- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h index 1eaac1f..65e1dcf 100644 --- a/include/net/netfilter/nf_conntrack_helper.h +++ b/include/net/netfilter/nf_conntrack_helper.h @@ -58,6 +58,8 @@ struct nf_conntrack_helper *__nf_conntrack_helper_find(const char *name, struct nf_conntrack_helper *nf_conntrack_helper_try_module_get(const char *name, u16 l3num, u8 protonum); +void nf_conntrack_helper_put(struct nf_conntrack_helper *helper); + void nf_ct_helper_init(struct nf_conntrack_helper *helper, u16 l3num, u16 protonum, const char *name, u16 default_port, u16 spec_port, u32 id, diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c index 6dc44d9..5275e9a 100644 --- a/net/netfilter/nf_conntrack_helper.c +++ b/net/netfilter/nf_conntrack_helper.c @@ -172,6 +172,12 @@ nf_conntrack_helper_try_module_get(const char *name, u16 l3num, u8 protonum) } EXPORT_SYMBOL_GPL(nf_conntrack_helper_try_module_get); +void nf_conntrack_helper_put(struct nf_conntrack_helper *helper) +{ + module_put(helper->me); +} +EXPORT_SYMBOL_GPL(nf_conntrack_helper_put); + struct nf_conn_help * nf_ct_helper_ext_add(struct nf_conn *ct, struct nf_conntrack_helper *helper, gfp_t gfp) diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c index 841cfba..2db42ca 100644 --- a/net/netfilter/xt_CT.c +++ b/net/netfilter/xt_CT.c @@ -95,7 +95,7 @@ xt_ct_set_helper(struct nf_conn *ct, const char *helper_name, help = nf_ct_helper_ext_add(ct, helper, GFP_KERNEL); if (help == NULL) { - module_put(helper->me); + nf_conntrack_helper_put(helper); return -ENOMEM; } @@ -260,7 +260,7 @@ static int xt_ct_tg_check(const struct xt_tgchk_param *par, err4: help = nfct_help(ct); if (help) - module_put(help->helper->me); + nf_conntrack_helper_put(help->helper); err3: nf_ct_tmpl_free(ct); err2: @@ -343,7 +343,7 @@ static void xt_ct_tg_destroy(const struct xt_tgdtor_param *par, if (ct && !nf_ct_is_untracked(ct)) { help = nfct_help(ct); if (help) - module_put(help->helper->me); + nf_conntrack_helper_put(help->helper); nf_ct_netns_put(par->net, par->family); diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c index 7b2c2fc..fd861d9 100644 --- a/net/openvswitch/conntrack.c +++ b/net/openvswitch/conntrack.c @@ -1084,7 +1084,7 @@ static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char *name, help = nf_ct_helper_ext_add(info->ct, helper, GFP_KERNEL); if (!help) { - module_put(helper->me); + nf_conntrack_helper_put(helper); return -ENOMEM; } @@ -1534,7 +1534,7 @@ void ovs_ct_free_action(const struct nlattr *a) static void __ovs_ct_free_action(struct ovs_conntrack_info *ct_info) { if (ct_info->helper) - module_put(ct_info->helper->me); + nf_conntrack_helper_put(ct_info->helper); if (ct_info->ct) nf_ct_tmpl_free(ct_info->ct); } -- 2.5.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use 2017-04-09 8:22 [PATCH nf 0/2] netfilter: reject cthelper del request if it is in use Liping Zhang 2017-04-09 8:22 ` [PATCH nf 1/2] netfilter: introduce nf_conntrack_helper_put helper function Liping Zhang @ 2017-04-09 8:22 ` Liping Zhang 2017-04-13 23:16 ` Pablo Neira Ayuso 1 sibling, 1 reply; 7+ messages in thread From: Liping Zhang @ 2017-04-09 8:22 UTC (permalink / raw) To: pablo; +Cc: netfilter-devel, Liping Zhang From: Liping Zhang <zlpnobody@gmail.com> 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 //--> succeed! 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: Liping Zhang <zlpnobody@gmail.com> --- Also note, nft ct helper obj only exists in nf-next tree, so after this patch appeared in nf-next, I will send another patch to fix it. include/net/netfilter/nf_conntrack_helper.h | 2 ++ net/netfilter/nf_conntrack_helper.c | 6 ++++++ net/netfilter/nfnetlink_cthelper.c | 17 +++++++++++------ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h index 65e1dcf..c7a9ad7 100644 --- a/include/net/netfilter/nf_conntrack_helper.h +++ b/include/net/netfilter/nf_conntrack_helper.h @@ -9,6 +9,7 @@ #ifndef _NF_CONNTRACK_HELPER_H #define _NF_CONNTRACK_HELPER_H +#include <linux/refcount.h> #include <net/netfilter/nf_conntrack.h> #include <net/netfilter/nf_conntrack_extend.h> #include <net/netfilter/nf_conntrack_expect.h> @@ -26,6 +27,7 @@ struct nf_conntrack_helper { struct hlist_node hnode; /* Internal use. */ char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */ + refcount_t refcnt; struct module *me; /* pointer to self */ const struct nf_conntrack_expect_policy *expect_policy; diff --git a/net/netfilter/nf_conntrack_helper.c b/net/netfilter/nf_conntrack_helper.c index 5275e9a..8fdd03b 100644 --- a/net/netfilter/nf_conntrack_helper.c +++ b/net/netfilter/nf_conntrack_helper.c @@ -167,6 +167,10 @@ nf_conntrack_helper_try_module_get(const char *name, u16 l3num, u8 protonum) #endif if (h != NULL && !try_module_get(h->me)) h = NULL; + if (h != NULL && !refcount_inc_not_zero(&h->refcnt)) { + module_put(h->me); + h = NULL; + } return h; } @@ -174,6 +178,7 @@ EXPORT_SYMBOL_GPL(nf_conntrack_helper_try_module_get); void nf_conntrack_helper_put(struct nf_conntrack_helper *helper) { + refcount_dec(&helper->refcnt); module_put(helper->me); } EXPORT_SYMBOL_GPL(nf_conntrack_helper_put); @@ -398,6 +403,7 @@ int nf_conntrack_helper_register(struct nf_conntrack_helper *me) goto out; } } + refcount_set(&me->refcnt, 1); hlist_add_head_rcu(&me->hnode, &nf_ct_helper_hash[h]); nf_ct_helper_count++; out: diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c index d455581..6e9e3c4 100644 --- a/net/netfilter/nfnetlink_cthelper.c +++ b/net/netfilter/nfnetlink_cthelper.c @@ -672,6 +672,7 @@ static int nfnl_cthelper_del(struct net *net, struct sock *nfnl, tuple_set = true; } + ret = -ENOENT; list_for_each_entry_safe(nlcth, n, &nfnl_cthelper_list, list) { cur = &nlcth->helper; j++; @@ -685,16 +686,20 @@ static int nfnl_cthelper_del(struct net *net, struct sock *nfnl, tuple.dst.protonum != cur->tuple.dst.protonum)) continue; - found = true; - nf_conntrack_helper_unregister(cur); - kfree(cur->expect_policy); + if (refcount_dec_if_one(&cur->refcnt)) { + found = true; + nf_conntrack_helper_unregister(cur); + kfree(cur->expect_policy); - list_del(&nlcth->list); - kfree(nlcth); + list_del(&nlcth->list); + kfree(nlcth); + } else { + ret = -EBUSY; + } } /* 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] = { -- 2.5.5 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use 2017-04-09 8:22 ` [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use Liping Zhang @ 2017-04-13 23:16 ` Pablo Neira Ayuso 2017-04-13 23:37 ` Liping Zhang 0 siblings, 1 reply; 7+ messages in thread From: Pablo Neira Ayuso @ 2017-04-13 23:16 UTC (permalink / raw) To: Liping Zhang; +Cc: netfilter-devel, Liping Zhang On Sun, Apr 09, 2017 at 04:22:14PM +0800, Liping Zhang wrote: > From: Liping Zhang <zlpnobody@gmail.com> > > 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 //--> succeed! > > 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: Liping Zhang <zlpnobody@gmail.com> > --- > Also note, nft ct helper obj only exists in nf-next tree, so after this > patch appeared in nf-next, I will send another patch to fix it. > > include/net/netfilter/nf_conntrack_helper.h | 2 ++ > net/netfilter/nf_conntrack_helper.c | 6 ++++++ > net/netfilter/nfnetlink_cthelper.c | 17 +++++++++++------ > 3 files changed, 19 insertions(+), 6 deletions(-) > > diff --git a/include/net/netfilter/nf_conntrack_helper.h b/include/net/netfilter/nf_conntrack_helper.h > index 65e1dcf..c7a9ad7 100644 > --- a/include/net/netfilter/nf_conntrack_helper.h > +++ b/include/net/netfilter/nf_conntrack_helper.h > @@ -9,6 +9,7 @@ > > #ifndef _NF_CONNTRACK_HELPER_H > #define _NF_CONNTRACK_HELPER_H > +#include <linux/refcount.h> > #include <net/netfilter/nf_conntrack.h> > #include <net/netfilter/nf_conntrack_extend.h> > #include <net/netfilter/nf_conntrack_expect.h> > @@ -26,6 +27,7 @@ struct nf_conntrack_helper { > struct hlist_node hnode; /* Internal use. */ > > char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */ > + refcount_t refcnt; Should this new refcnt; thing be in the new struct nfnl_cthelper? I think this refcount is only required by the userspace helper infrastructure, not existing in-kernel helpers. I think like that we can skip patch 1/2 in this series. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use 2017-04-13 23:16 ` Pablo Neira Ayuso @ 2017-04-13 23:37 ` Liping Zhang 2017-04-13 23:42 ` Pablo Neira Ayuso 0 siblings, 1 reply; 7+ messages in thread From: Liping Zhang @ 2017-04-13 23:37 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: Liping Zhang, Netfilter Developer Mailing List Hi Pablo, 2017-04-14 7:16 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>: [...] >> #ifndef _NF_CONNTRACK_HELPER_H >> #define _NF_CONNTRACK_HELPER_H >> +#include <linux/refcount.h> >> #include <net/netfilter/nf_conntrack.h> >> #include <net/netfilter/nf_conntrack_extend.h> >> #include <net/netfilter/nf_conntrack_expect.h> >> @@ -26,6 +27,7 @@ struct nf_conntrack_helper { >> struct hlist_node hnode; /* Internal use. */ >> >> char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */ >> + refcount_t refcnt; > > Should this new refcnt; thing be in the new struct nfnl_cthelper? > > I think this refcount is only required by the userspace helper > infrastructure, not existing in-kernel helpers. > > I think like that we can skip patch 1/2 in this series. We must call nf_conntrack_helper_try_module_get to get the helper, either it is userspace or in-kernel helpers. Also the caller didn't care the helpers is userspace or in-kernel, so I think introducing the nf_conntrack_helper_put is necessary. Also, this path set is prepared for per-net helper. For in-kernel helpers, we will still need to kmemdup the original one. I mean: helper = kmemdup(ftp_helper); helper->expect_policy = kmemdup(ftp_exp_policy); nf_conntrack_helper_register(net, helper); And similar to nfnetlink_cttimeout, for net_exit, we should: list_for_each_entry_safe() unregister_helper(); if (refcount_dec_and_test(&cur->refcnt)) call_rcu(free_xxx); For nf_conntrack_helper_put, we should: if (refcount_dec_and_test(&cur->refcnt)) call_rcu(free_xxx); So I think put this "refcount_t refcnt" to struct nf_conntrack_helper is better. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use 2017-04-13 23:37 ` Liping Zhang @ 2017-04-13 23:42 ` Pablo Neira Ayuso 2017-04-13 23:42 ` Pablo Neira Ayuso 0 siblings, 1 reply; 7+ messages in thread From: Pablo Neira Ayuso @ 2017-04-13 23:42 UTC (permalink / raw) To: Liping Zhang; +Cc: Liping Zhang, Netfilter Developer Mailing List On Fri, Apr 14, 2017 at 07:37:50AM +0800, Liping Zhang wrote: > Hi Pablo, > > 2017-04-14 7:16 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>: > [...] > >> #ifndef _NF_CONNTRACK_HELPER_H > >> #define _NF_CONNTRACK_HELPER_H > >> +#include <linux/refcount.h> > >> #include <net/netfilter/nf_conntrack.h> > >> #include <net/netfilter/nf_conntrack_extend.h> > >> #include <net/netfilter/nf_conntrack_expect.h> > >> @@ -26,6 +27,7 @@ struct nf_conntrack_helper { > >> struct hlist_node hnode; /* Internal use. */ > >> > >> char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */ > >> + refcount_t refcnt; > > > > Should this new refcnt; thing be in the new struct nfnl_cthelper? > > > > I think this refcount is only required by the userspace helper > > infrastructure, not existing in-kernel helpers. > > > > I think like that we can skip patch 1/2 in this series. > > We must call nf_conntrack_helper_try_module_get to get the helper, > either it is userspace or in-kernel helpers. Also the caller didn't care > the helpers is userspace or in-kernel, so I think introducing the > nf_conntrack_helper_put is necessary. > > Also, this path set is prepared for per-net helper. For in-kernel helpers, > we will still need to kmemdup the original one. I mean: > helper = kmemdup(ftp_helper); > helper->expect_policy = kmemdup(ftp_exp_policy); > nf_conntrack_helper_register(net, helper); > > And similar to nfnetlink_cttimeout, for net_exit, we should: > list_for_each_entry_safe() > unregister_helper(); > if (refcount_dec_and_test(&cur->refcnt)) > call_rcu(free_xxx); > > For nf_conntrack_helper_put, we should: > if (refcount_dec_and_test(&cur->refcnt)) > call_rcu(free_xxx); > > So I think put this "refcount_t refcnt" to struct nf_conntrack_helper > is better. OK, please resubmit target to nf-next. It's too late for patches already for nf, we're at -rc6 and this problem has been there for a bit of time. So waiting a bit more to get this fixed should be OK. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use 2017-04-13 23:42 ` Pablo Neira Ayuso @ 2017-04-13 23:42 ` Pablo Neira Ayuso 0 siblings, 0 replies; 7+ messages in thread From: Pablo Neira Ayuso @ 2017-04-13 23:42 UTC (permalink / raw) To: Liping Zhang; +Cc: Liping Zhang, Netfilter Developer Mailing List On Fri, Apr 14, 2017 at 01:42:04AM +0200, Pablo Neira Ayuso wrote: > On Fri, Apr 14, 2017 at 07:37:50AM +0800, Liping Zhang wrote: > > Hi Pablo, > > > > 2017-04-14 7:16 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>: > > [...] > > >> #ifndef _NF_CONNTRACK_HELPER_H > > >> #define _NF_CONNTRACK_HELPER_H > > >> +#include <linux/refcount.h> > > >> #include <net/netfilter/nf_conntrack.h> > > >> #include <net/netfilter/nf_conntrack_extend.h> > > >> #include <net/netfilter/nf_conntrack_expect.h> > > >> @@ -26,6 +27,7 @@ struct nf_conntrack_helper { > > >> struct hlist_node hnode; /* Internal use. */ > > >> > > >> char name[NF_CT_HELPER_NAME_LEN]; /* name of the module */ > > >> + refcount_t refcnt; > > > > > > Should this new refcnt; thing be in the new struct nfnl_cthelper? > > > > > > I think this refcount is only required by the userspace helper > > > infrastructure, not existing in-kernel helpers. > > > > > > I think like that we can skip patch 1/2 in this series. > > > > We must call nf_conntrack_helper_try_module_get to get the helper, > > either it is userspace or in-kernel helpers. Also the caller didn't care > > the helpers is userspace or in-kernel, so I think introducing the > > nf_conntrack_helper_put is necessary. > > > > Also, this path set is prepared for per-net helper. For in-kernel helpers, > > we will still need to kmemdup the original one. I mean: > > helper = kmemdup(ftp_helper); > > helper->expect_policy = kmemdup(ftp_exp_policy); > > nf_conntrack_helper_register(net, helper); > > > > And similar to nfnetlink_cttimeout, for net_exit, we should: > > list_for_each_entry_safe() > > unregister_helper(); > > if (refcount_dec_and_test(&cur->refcnt)) > > call_rcu(free_xxx); > > > > For nf_conntrack_helper_put, we should: > > if (refcount_dec_and_test(&cur->refcnt)) > > call_rcu(free_xxx); > > > > So I think put this "refcount_t refcnt" to struct nf_conntrack_helper > > is better. > > OK, please resubmit target to nf-next. It's too late for patches > already for nf, we're at -rc6 and this problem has been there for a > bit of time. So waiting a bit more to get this fixed should be OK. BTW, you may have to wait until dependencies (ie. patches in nf.git) show up in nf-next.git, it may take a little while to propagate. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-04-13 23:43 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-04-09 8:22 [PATCH nf 0/2] netfilter: reject cthelper del request if it is in use Liping Zhang 2017-04-09 8:22 ` [PATCH nf 1/2] netfilter: introduce nf_conntrack_helper_put helper function Liping Zhang 2017-04-09 8:22 ` [PATCH nf 2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use Liping Zhang 2017-04-13 23:16 ` Pablo Neira Ayuso 2017-04-13 23:37 ` Liping Zhang 2017-04-13 23:42 ` Pablo Neira Ayuso 2017-04-13 23:42 ` Pablo Neira Ayuso
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).