[nf,5/5] netfilter: nfnl_cthelper: fix a race when walk the nf_ct_helper_hash table

Message ID 1489934162-7415-6-git-send-email-zlpnobody@163.com
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

Liping Zhang March 19, 2017, 2:36 p.m.
From: Liping Zhang <zlpnobody@gmail.com>

The nf_ct_helper_hash table is protected by nf_ct_helper_mutex, while
nfct_helper operation is protected by nfnl_lock(NFNL_SUBSYS_CTHELPER).
So it's possible that one CPU is walking the nf_ct_helper_hash for
cthelper add/get/del, another cpu is doing nf_conntrack_helpers_unregister
at the same time. This is dangrous, and may cause use after free error.

Note, delete operation will flush all cthelpers added via nfnetlink, so
using rcu to do protect is not easy.

Now introduce a dummy list to record all the cthelpers added via
nfnetlink, then we can walk the dummy list instead of walking the
nf_ct_helper_hash. Also, keep nfnl_cthelper_dump_table unchanged, it
may be invoked without nfnl_lock(NFNL_SUBSYS_CTHELPER) held.

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 net/netfilter/nfnetlink_cthelper.c | 176 +++++++++++++++++++------------------
 1 file changed, 89 insertions(+), 87 deletions(-)

Comments

Pablo Neira Ayuso March 21, 2017, 10:33 a.m. | #1
On Sun, Mar 19, 2017 at 10:36:02PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> The nf_ct_helper_hash table is protected by nf_ct_helper_mutex, while
> nfct_helper operation is protected by nfnl_lock(NFNL_SUBSYS_CTHELPER).
> So it's possible that one CPU is walking the nf_ct_helper_hash for
> cthelper add/get/del, another cpu is doing nf_conntrack_helpers_unregister
> at the same time. This is dangrous, and may cause use after free error.
> 
> Note, delete operation will flush all cthelpers added via nfnetlink, so
> using rcu to do protect is not easy.
> 
> Now introduce a dummy list to record all the cthelpers added via
> nfnetlink, then we can walk the dummy list instead of walking the
> nf_ct_helper_hash. Also, keep nfnl_cthelper_dump_table unchanged, it
> may be invoked without nfnl_lock(NFNL_SUBSYS_CTHELPER) held.
> 
> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
> ---
>  net/netfilter/nfnetlink_cthelper.c | 176 +++++++++++++++++++------------------
>  1 file changed, 89 insertions(+), 87 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
> index fc4733f..47424ec 100644
> --- a/net/netfilter/nfnetlink_cthelper.c
> +++ b/net/netfilter/nfnetlink_cthelper.c
> @@ -32,6 +32,13 @@ MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Pablo Neira Ayuso <pablo@netfilter.org>");
>  MODULE_DESCRIPTION("nfnl_cthelper: User-space connection tracking helpers");
>  
> +struct nfnl_cthelper {
> +	struct list_head		list;
> +	struct nf_conntrack_helper	*helper;
> +};
> +
> +static LIST_HEAD(nfnl_cthelper_list);

We need a field possible_net_t so we can store what netns this helper
belongs to, thus in case of flush command, we just remove the helpers
that this netns owns.
--
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 March 21, 2017, 2:48 p.m. | #2
2017-03-21 18:33 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> On Sun, Mar 19, 2017 at 10:36:02PM +0800, Liping Zhang wrote:
>> From: Liping Zhang <zlpnobody@gmail.com>
>>
>> The nf_ct_helper_hash table is protected by nf_ct_helper_mutex, while
>> nfct_helper operation is protected by nfnl_lock(NFNL_SUBSYS_CTHELPER).
>> So it's possible that one CPU is walking the nf_ct_helper_hash for
>> cthelper add/get/del, another cpu is doing nf_conntrack_helpers_unregister
>> at the same time. This is dangrous, and may cause use after free error.
>>
>> Note, delete operation will flush all cthelpers added via nfnetlink, so
>> using rcu to do protect is not easy.
>>
>> Now introduce a dummy list to record all the cthelpers added via
>> nfnetlink, then we can walk the dummy list instead of walking the
>> nf_ct_helper_hash. Also, keep nfnl_cthelper_dump_table unchanged, it
>> may be invoked without nfnl_lock(NFNL_SUBSYS_CTHELPER) held.
>>
>> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
>> ---
>>  net/netfilter/nfnetlink_cthelper.c | 176 +++++++++++++++++++------------------
>>  1 file changed, 89 insertions(+), 87 deletions(-)
>>
>> diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
>> index fc4733f..47424ec 100644
>> --- a/net/netfilter/nfnetlink_cthelper.c
>> +++ b/net/netfilter/nfnetlink_cthelper.c
>> @@ -32,6 +32,13 @@ MODULE_LICENSE("GPL");
>>  MODULE_AUTHOR("Pablo Neira Ayuso <pablo@netfilter.org>");
>>  MODULE_DESCRIPTION("nfnl_cthelper: User-space connection tracking helpers");
>>
>> +struct nfnl_cthelper {
>> +     struct list_head                list;
>> +     struct nf_conntrack_helper      *helper;
>> +};
>> +
>> +static LIST_HEAD(nfnl_cthelper_list);
>
> We need a field possible_net_t so we can store what netns this helper
> belongs to, thus in case of flush command, we just remove the helpers
> that this netns owns.

Yes, good point, I will send v2.

Thanks Pablo.
--
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 March 21, 2017, 3:19 p.m. | #3
Hi Pablo,

2017-03-21 22:48 GMT+08:00 Liping Zhang <zlpnobody@gmail.com>:
> 2017-03-21 18:33 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
>>> +struct nfnl_cthelper {
>>> +     struct list_head                list;
>>> +     struct nf_conntrack_helper      *helper;
>>> +};
>>> +
>>> +static LIST_HEAD(nfnl_cthelper_list);
>>
>> We need a field possible_net_t so we can store what netns this helper
>> belongs to, thus in case of flush command, we just remove the helpers
>> that this netns owns.

After I have a closer look, I find that we do not support netns for the
nfct_helper currently. So this possible_net_t field is not necessary for
the time being.

I have a quick glance look, supporting netns for helper need a lot works
to do. We need to both change the nfnetlink_cthelper, nf_conntrack_help
and so on.

But if you think it's worth to support netns for cthelper, I can finish it in my
spare time:)

>
> Yes, good point, I will send v2.
>
> Thanks Pablo.
--
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 March 21, 2017, 3:26 p.m. | #4
On Tue, Mar 21, 2017 at 11:19:11PM +0800, Liping Zhang wrote:
> Hi Pablo,
> 
> 2017-03-21 22:48 GMT+08:00 Liping Zhang <zlpnobody@gmail.com>:
> > 2017-03-21 18:33 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> >>> +struct nfnl_cthelper {
> >>> +     struct list_head                list;
> >>> +     struct nf_conntrack_helper      *helper;
> >>> +};
> >>> +
> >>> +static LIST_HEAD(nfnl_cthelper_list);
> >>
> >> We need a field possible_net_t so we can store what netns this helper
> >> belongs to, thus in case of flush command, we just remove the helpers
> >> that this netns owns.
> 
> After I have a closer look, I find that we do not support netns for the
> nfct_helper currently. So this possible_net_t field is not necessary for
> the time being.

Oh, I see. This is probably one of the remaining subsystems not having
netns support.

> I have a quick glance look, supporting netns for helper need a lot works
> to do. We need to both change the nfnetlink_cthelper, nf_conntrack_help
> and so on.
> 
> But if you think it's worth to support netns for cthelper, I can
> finish it in my spare time:)

Let's focus on fixing up the existing issues. It would be great if you
can add that later on, once changes in nf.git propagate to
nf-next.git.

BTW, let me also pushed out what I have here into nf.git. I'd
appreciate if you can rebase this 5/5 patch on top of it.

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
Liping Zhang March 22, 2017, 12:06 a.m. | #5
2017-03-21 23:26 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
[...]
>>
>> After I have a closer look, I find that we do not support netns for the
>> nfct_helper currently. So this possible_net_t field is not necessary for
>> the time being.
>
> Oh, I see. This is probably one of the remaining subsystems not having
> netns support.
>
>> I have a quick glance look, supporting netns for helper need a lot works
>> to do. We need to both change the nfnetlink_cthelper, nf_conntrack_help
>> and so on.
>>
>> But if you think it's worth to support netns for cthelper, I can
>> finish it in my spare time:)
>
> Let's focus on fixing up the existing issues. It would be great if you
> can add that later on, once changes in nf.git propagate to
> nf-next.git.

OK, I will keep follow up on it.

> BTW, let me also pushed out what I have here into nf.git. I'd
> appreciate if you can rebase this 5/5 patch on top of it.

No problem!
--
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

Patch

diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index fc4733f..47424ec 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -32,6 +32,13 @@  MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Pablo Neira Ayuso <pablo@netfilter.org>");
 MODULE_DESCRIPTION("nfnl_cthelper: User-space connection tracking helpers");
 
+struct nfnl_cthelper {
+	struct list_head		list;
+	struct nf_conntrack_helper	*helper;
+};
+
+static LIST_HEAD(nfnl_cthelper_list);
+
 static int
 nfnl_userspace_cthelper(struct sk_buff *skb, unsigned int protoff,
 			struct nf_conn *ct, enum ip_conntrack_info ctinfo)
@@ -214,14 +221,21 @@  nfnl_cthelper_create(const struct nlattr * const tb[],
 		     struct nf_conntrack_tuple *tuple)
 {
 	struct nf_conntrack_helper *helper;
+	struct nfnl_cthelper *nfcth;
 	int ret;
 
 	if (!tb[NFCTH_TUPLE] || !tb[NFCTH_POLICY])
 		return -EINVAL;
 
+	nfcth = kmalloc(sizeof(struct nfnl_cthelper), GFP_KERNEL);
+	if (nfcth == NULL)
+		return -ENOMEM;
+
 	helper = kzalloc(sizeof(struct nf_conntrack_helper), GFP_KERNEL);
-	if (helper == NULL)
+	if (helper == NULL) {
+		kfree(nfcth);
 		return -ENOMEM;
+	}
 
 	ret = nfnl_cthelper_parse_expect_policy(helper, tb[NFCTH_POLICY],
 						false);
@@ -260,10 +274,13 @@  nfnl_cthelper_create(const struct nlattr * const tb[],
 	if (ret < 0)
 		goto err2;
 
+	nfcth->helper = helper;
+	list_add_tail(&nfcth->list, &nfnl_cthelper_list);
 	return 0;
 err2:
 	kfree(helper->expect_policy);
 err1:
+	kfree(nfcth);
 	kfree(helper);
 	return ret;
 }
@@ -309,7 +326,8 @@  static int nfnl_cthelper_new(struct net *net, struct sock *nfnl,
 	const char *helper_name;
 	struct nf_conntrack_helper *cur, *helper = NULL;
 	struct nf_conntrack_tuple tuple;
-	int ret = 0, i;
+	struct nfnl_cthelper *nlcth;
+	int ret = 0;
 
 	if (!tb[NFCTH_NAME] || !tb[NFCTH_TUPLE])
 		return -EINVAL;
@@ -320,28 +338,22 @@  static int nfnl_cthelper_new(struct net *net, struct sock *nfnl,
 	if (ret < 0)
 		return ret;
 
-	for (i = 0; i < nf_ct_helper_hsize && !helper; i++) {
-		hlist_for_each_entry(cur, &nf_ct_helper_hash[i], hnode) {
-
-			/* skip non-userspace conntrack helpers. */
-			if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
-				continue;
+	list_for_each_entry(nlcth, &nfnl_cthelper_list, list) {
+		cur = nlcth->helper;
 
-			if (strncmp(cur->name, helper_name,
-					NF_CT_HELPER_NAME_LEN) != 0)
-				continue;
+		if (strncmp(cur->name, helper_name, NF_CT_HELPER_NAME_LEN))
+			continue;
 
-			if ((tuple.src.l3num != cur->tuple.src.l3num ||
-			     tuple.dst.protonum != cur->tuple.dst.protonum))
-				continue;
+		if ((tuple.src.l3num != cur->tuple.src.l3num ||
+		     tuple.dst.protonum != cur->tuple.dst.protonum))
+			continue;
 
-			if (nlh->nlmsg_flags & NLM_F_EXCL) {
-				ret = -EEXIST;
-				goto err;
-			}
-			helper = cur;
-			break;
+		if (nlh->nlmsg_flags & NLM_F_EXCL) {
+			ret = -EEXIST;
+			goto err;
 		}
+		helper = cur;
+		break;
 	}
 
 	if (helper == NULL)
@@ -513,11 +525,12 @@  static int nfnl_cthelper_get(struct net *net, struct sock *nfnl,
 			     struct sk_buff *skb, const struct nlmsghdr *nlh,
 			     const struct nlattr * const tb[])
 {
-	int ret = -ENOENT, i;
+	int ret = -ENOENT;
 	struct nf_conntrack_helper *cur;
 	struct sk_buff *skb2;
 	char *helper_name = NULL;
 	struct nf_conntrack_tuple tuple;
+	struct nfnl_cthelper *nlcth;
 	bool tuple_set = false;
 
 	if (nlh->nlmsg_flags & NLM_F_DUMP) {
@@ -538,45 +551,39 @@  static int nfnl_cthelper_get(struct net *net, struct sock *nfnl,
 		tuple_set = true;
 	}
 
-	for (i = 0; i < nf_ct_helper_hsize; i++) {
-		hlist_for_each_entry_rcu(cur, &nf_ct_helper_hash[i], hnode) {
+	list_for_each_entry(nlcth, &nfnl_cthelper_list, list) {
+		cur = nlcth->helper;
+		if (helper_name &&
+		    strncmp(cur->name, helper_name, NF_CT_HELPER_NAME_LEN))
+			continue;
 
-			/* skip non-userspace conntrack helpers. */
-			if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
-				continue;
-
-			if (helper_name && strncmp(cur->name, helper_name,
-						NF_CT_HELPER_NAME_LEN) != 0) {
-				continue;
-			}
-			if (tuple_set &&
-			    (tuple.src.l3num != cur->tuple.src.l3num ||
-			     tuple.dst.protonum != cur->tuple.dst.protonum))
-				continue;
+		if (tuple_set &&
+		    (tuple.src.l3num != cur->tuple.src.l3num ||
+		     tuple.dst.protonum != cur->tuple.dst.protonum))
+			continue;
 
-			skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
-			if (skb2 == NULL) {
-				ret = -ENOMEM;
-				break;
-			}
+		skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+		if (skb2 == NULL) {
+			ret = -ENOMEM;
+			break;
+		}
 
-			ret = nfnl_cthelper_fill_info(skb2, NETLINK_CB(skb).portid,
-						nlh->nlmsg_seq,
-						NFNL_MSG_TYPE(nlh->nlmsg_type),
-						NFNL_MSG_CTHELPER_NEW, cur);
-			if (ret <= 0) {
-				kfree_skb(skb2);
-				break;
-			}
+		ret = nfnl_cthelper_fill_info(skb2, NETLINK_CB(skb).portid,
+					      nlh->nlmsg_seq,
+					      NFNL_MSG_TYPE(nlh->nlmsg_type),
+					      NFNL_MSG_CTHELPER_NEW, cur);
+		if (ret <= 0) {
+			kfree_skb(skb2);
+			break;
+		}
 
-			ret = netlink_unicast(nfnl, skb2, NETLINK_CB(skb).portid,
-						MSG_DONTWAIT);
-			if (ret > 0)
-				ret = 0;
+		ret = netlink_unicast(nfnl, skb2, NETLINK_CB(skb).portid,
+				      MSG_DONTWAIT);
+		if (ret > 0)
+			ret = 0;
 
-			/* this avoids a loop in nfnetlink. */
-			return ret == -EAGAIN ? -ENOBUFS : ret;
-		}
+		/* this avoids a loop in nfnetlink. */
+		return ret == -EAGAIN ? -ENOBUFS : ret;
 	}
 	return ret;
 }
@@ -587,10 +594,10 @@  static int nfnl_cthelper_del(struct net *net, struct sock *nfnl,
 {
 	char *helper_name = NULL;
 	struct nf_conntrack_helper *cur;
-	struct hlist_node *tmp;
 	struct nf_conntrack_tuple tuple;
 	bool tuple_set = false, found = false;
-	int i, j = 0, ret;
+	struct nfnl_cthelper *nlcth, *n;
+	int j = 0, ret;
 
 	if (tb[NFCTH_NAME])
 		helper_name = nla_data(tb[NFCTH_NAME]);
@@ -603,30 +610,28 @@  static int nfnl_cthelper_del(struct net *net, struct sock *nfnl,
 		tuple_set = true;
 	}
 
-	for (i = 0; i < nf_ct_helper_hsize; i++) {
-		hlist_for_each_entry_safe(cur, tmp, &nf_ct_helper_hash[i],
-								hnode) {
-			/* skip non-userspace conntrack helpers. */
-			if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
-				continue;
+	list_for_each_entry_safe(nlcth, n, &nfnl_cthelper_list, list) {
+		cur = nlcth->helper;
+		j++;
 
-			j++;
+		if (helper_name &&
+		    strncmp(cur->name, helper_name, NF_CT_HELPER_NAME_LEN))
+			continue;
 
-			if (helper_name && strncmp(cur->name, helper_name,
-						NF_CT_HELPER_NAME_LEN) != 0) {
-				continue;
-			}
-			if (tuple_set &&
-			    (tuple.src.l3num != cur->tuple.src.l3num ||
-			     tuple.dst.protonum != cur->tuple.dst.protonum))
-				continue;
+		if (tuple_set &&
+		    (tuple.src.l3num != cur->tuple.src.l3num ||
+		     tuple.dst.protonum != cur->tuple.dst.protonum))
+			continue;
 
-			found = true;
-			nf_conntrack_helper_unregister(cur);
-			kfree(cur->expect_policy);
-			kfree(cur);
-		}
+		found = true;
+		nf_conntrack_helper_unregister(cur);
+		kfree(cur->expect_policy);
+		kfree(cur);
+
+		list_del(&nlcth->list);
+		kfree(nlcth);
 	}
+
 	/* Make sure we return success if we flush and there is no helpers */
 	return (found || j == 0) ? 0 : -ENOENT;
 }
@@ -675,20 +680,17 @@  static int __init nfnl_cthelper_init(void)
 static void __exit nfnl_cthelper_exit(void)
 {
 	struct nf_conntrack_helper *cur;
-	struct hlist_node *tmp;
-	int i;
+	struct nfnl_cthelper *nlcth, *n;
 
 	nfnetlink_subsys_unregister(&nfnl_cthelper_subsys);
 
-	for (i=0; i<nf_ct_helper_hsize; i++) {
-		hlist_for_each_entry_safe(cur, tmp, &nf_ct_helper_hash[i],
-									hnode) {
-			/* skip non-userspace conntrack helpers. */
-			if (!(cur->flags & NF_CT_HELPER_F_USERSPACE))
-				continue;
+	list_for_each_entry_safe(nlcth, n, &nfnl_cthelper_list, list) {
+		cur = nlcth->helper;
 
-			nf_conntrack_helper_unregister(cur);
-		}
+		nf_conntrack_helper_unregister(cur);
+		kfree(cur->expect_policy);
+		kfree(cur);
+		kfree(nlcth);
 	}
 }