diff mbox

[nf,2/2] netfilter: nfnl_cthelper: reject del request if helper obj is in use

Message ID 1491726134-23686-3-git-send-email-zlpnobody@163.com
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

Liping Zhang April 9, 2017, 8:22 a.m. UTC 
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(-)

Comments

Pablo Neira Ayuso April 13, 2017, 11:16 p.m. UTC | #1 
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Liping Zhang April 13, 2017, 11:37 p.m. UTC | #2 
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso April 13, 2017, 11:42 p.m. UTC | #3 
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso April 13, 2017, 11:42 p.m. UTC | #4 
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.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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] = {