[nf,1/1] netfilter: ctlink: Fix one possible use-after-free in ctnetlink_create_expect

Message ID 1489808770-29622-1-git-send-email-fgao@ikuai8.com
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

高峰 March 18, 2017, 3:46 a.m.
From: Gao Feng <fgao@ikuai8.com>

There is no rcu_read_lock during ctlink gets the helper and inserts the
expectation. So there is one possible use-after-free issue when unload
the helper module.

For example:

        CPU1                                CPU2
ctlink gets the helper
                            helper module unload and remove all expectations
insert the expectation

Now there is one expectation which references one helper whose module is
unloaded.

Signed-off-by: Gao Feng <fgao@ikuai8.com>
---
 net/netfilter/nf_conntrack_netlink.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Feng Gao March 18, 2017, 4:26 a.m. | #1
Hi Pablo,

On Sat, Mar 18, 2017 at 11:46 AM,  <fgao@ikuai8.com> wrote:
> From: Gao Feng <fgao@ikuai8.com>
>
> There is no rcu_read_lock during ctlink gets the helper and inserts the
> expectation. So there is one possible use-after-free issue when unload
> the helper module.
>
> For example:
>
>         CPU1                                CPU2
> ctlink gets the helper
>                             helper module unload and remove all expectations
> insert the expectation
>
> Now there is one expectation which references one helper whose module is
> unloaded.
>
> Signed-off-by: Gao Feng <fgao@ikuai8.com>
> ---
>  net/netfilter/nf_conntrack_netlink.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> index 6806b5e..f6d1d63 100644
> --- a/net/netfilter/nf_conntrack_netlink.c
> +++ b/net/netfilter/nf_conntrack_netlink.c
> @@ -3133,23 +3133,27 @@ static int ctnetlink_del_expect(struct net *net, struct sock *ctnl,
>                 return -ENOENT;
>         ct = nf_ct_tuplehash_to_ctrack(h);
>
> +       rcu_read_lock();
>         if (cda[CTA_EXPECT_HELP_NAME]) {
>                 const char *helpname = nla_data(cda[CTA_EXPECT_HELP_NAME]);
>
>                 helper = __nf_conntrack_helper_find(helpname, u3,
>                                                     nf_ct_protonum(ct));
>                 if (helper == NULL) {
> +                       rcu_read_unlock();
>  #ifdef CONFIG_MODULES
>                         if (request_module("nfct-helper-%s", helpname) < 0) {
>                                 err = -EOPNOTSUPP;
>                                 goto err_ct;
>                         }
> +                       rcu_read_lock();
>                         helper = __nf_conntrack_helper_find(helpname, u3,
>                                                             nf_ct_protonum(ct));
>                         if (helper) {
>                                 err = -EAGAIN;
> -                               goto err_ct;
> +                               goto err_rcu;
>                         }
> +                       rcu_read_unlock();
>  #endif
>                         err = -EOPNOTSUPP;
>                         goto err_ct;
> @@ -3159,11 +3163,13 @@ static int ctnetlink_del_expect(struct net *net, struct sock *ctnl,
>         exp = ctnetlink_alloc_expect(cda, ct, helper, &tuple, &mask);
>         if (IS_ERR(exp)) {
>                 err = PTR_ERR(exp);
> -               goto err_ct;
> +               goto err_rcu;
>         }
>
>         err = nf_ct_expect_related_report(exp, portid, report);
>         nf_ct_expect_put(exp);
> +err_rcu:
> +       rcu_read_unlock();
>  err_ct:
>         nf_ct_put(ct);
>         return err;
> --
> 1.9.1
>
>

This patch covers part fix of the another patch “Fix possible panic
caused by invoking expectfn unloaded” whose link is
https://patchwork.ozlabs.org/patch/738566/ about RCU lock.

Because the current patch also fixes the use-after-free helper issue,
so I think it is better to commit the two patches.
The first is current one, it fixes the use-after-free bug in
ctnetlink_create_expect.
The second is used to fixes the expectfn issue that we need to remove
all expectations who reference the expectfn when unload the module.

I am thinking about that if there is one better solution than my
original patch.

Regards
Feng
--
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
Feng Gao March 18, 2017, 4:39 a.m. | #2
On Sat, Mar 18, 2017 at 12:26 PM, Feng Gao <gfree.wind@gmail.com> wrote:
> Hi Pablo,
>
> On Sat, Mar 18, 2017 at 11:46 AM,  <fgao@ikuai8.com> wrote:
>> From: Gao Feng <fgao@ikuai8.com>
>>
>> There is no rcu_read_lock during ctlink gets the helper and inserts the
>> expectation. So there is one possible use-after-free issue when unload
>> the helper module.
>>
>> For example:
>>
>>         CPU1                                CPU2
>> ctlink gets the helper
>>                             helper module unload and remove all expectations
>> insert the expectation
>>
>> Now there is one expectation which references one helper whose module is
>> unloaded.
>>
>> Signed-off-by: Gao Feng <fgao@ikuai8.com>
>> ---
>>  net/netfilter/nf_conntrack_netlink.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
>> index 6806b5e..f6d1d63 100644
>> --- a/net/netfilter/nf_conntrack_netlink.c
>> +++ b/net/netfilter/nf_conntrack_netlink.c
>> @@ -3133,23 +3133,27 @@ static int ctnetlink_del_expect(struct net *net, struct sock *ctnl,
>>                 return -ENOENT;
>>         ct = nf_ct_tuplehash_to_ctrack(h);
>>
>> +       rcu_read_lock();
>>         if (cda[CTA_EXPECT_HELP_NAME]) {
>>                 const char *helpname = nla_data(cda[CTA_EXPECT_HELP_NAME]);
>>
>>                 helper = __nf_conntrack_helper_find(helpname, u3,
>>                                                     nf_ct_protonum(ct));
>>                 if (helper == NULL) {
>> +                       rcu_read_unlock();
>>  #ifdef CONFIG_MODULES
>>                         if (request_module("nfct-helper-%s", helpname) < 0) {
>>                                 err = -EOPNOTSUPP;
>>                                 goto err_ct;
>>                         }
>> +                       rcu_read_lock();
>>                         helper = __nf_conntrack_helper_find(helpname, u3,
>>                                                             nf_ct_protonum(ct));
>>                         if (helper) {
>>                                 err = -EAGAIN;
>> -                               goto err_ct;
>> +                               goto err_rcu;
>>                         }
>> +                       rcu_read_unlock();
>>  #endif
>>                         err = -EOPNOTSUPP;
>>                         goto err_ct;
>> @@ -3159,11 +3163,13 @@ static int ctnetlink_del_expect(struct net *net, struct sock *ctnl,
>>         exp = ctnetlink_alloc_expect(cda, ct, helper, &tuple, &mask);
>>         if (IS_ERR(exp)) {
>>                 err = PTR_ERR(exp);
>> -               goto err_ct;
>> +               goto err_rcu;
>>         }
>>
>>         err = nf_ct_expect_related_report(exp, portid, report);
>>         nf_ct_expect_put(exp);
>> +err_rcu:
>> +       rcu_read_unlock();
>>  err_ct:
>>         nf_ct_put(ct);
>>         return err;
>> --
>> 1.9.1
>>
>>
>
> This patch covers part fix of the another patch “Fix possible panic
> caused by invoking expectfn unloaded” whose link is
> https://patchwork.ozlabs.org/patch/738566/ about RCU lock.
>
> Because the current patch also fixes the use-after-free helper issue,
> so I think it is better to commit the two patches.
> The first is current one, it fixes the use-after-free bug in
> ctnetlink_create_expect.
> The second is used to fixes the expectfn issue that we need to remove
> all expectations who reference the expectfn when unload the module.
>
> I am thinking about that if there is one better solution than my
> original patch.

I mean the second patch, not this one.
I am thinking about how to remove all expectation with one common
function for helper and expectfn.

I think this patch which fixes the use-after-free should be committed
as one single patch.

Regards
Feng

>
> Regards
> Feng
--
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/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 6806b5e..f6d1d63 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -3133,23 +3133,27 @@  static int ctnetlink_del_expect(struct net *net, struct sock *ctnl,
 		return -ENOENT;
 	ct = nf_ct_tuplehash_to_ctrack(h);
 
+	rcu_read_lock();
 	if (cda[CTA_EXPECT_HELP_NAME]) {
 		const char *helpname = nla_data(cda[CTA_EXPECT_HELP_NAME]);
 
 		helper = __nf_conntrack_helper_find(helpname, u3,
 						    nf_ct_protonum(ct));
 		if (helper == NULL) {
+			rcu_read_unlock();
 #ifdef CONFIG_MODULES
 			if (request_module("nfct-helper-%s", helpname) < 0) {
 				err = -EOPNOTSUPP;
 				goto err_ct;
 			}
+			rcu_read_lock();
 			helper = __nf_conntrack_helper_find(helpname, u3,
 							    nf_ct_protonum(ct));
 			if (helper) {
 				err = -EAGAIN;
-				goto err_ct;
+				goto err_rcu;
 			}
+			rcu_read_unlock();
 #endif
 			err = -EOPNOTSUPP;
 			goto err_ct;
@@ -3159,11 +3163,13 @@  static int ctnetlink_del_expect(struct net *net, struct sock *ctnl,
 	exp = ctnetlink_alloc_expect(cda, ct, helper, &tuple, &mask);
 	if (IS_ERR(exp)) {
 		err = PTR_ERR(exp);
-		goto err_ct;
+		goto err_rcu;
 	}
 
 	err = nf_ct_expect_related_report(exp, portid, report);
 	nf_ct_expect_put(exp);
+err_rcu:
+	rcu_read_unlock();
 err_ct:
 	nf_ct_put(ct);
 	return err;