* [PATCH nf 1/1] netfilter: ctlink: Fix one possible use-after-free in ctnetlink_create_expect
@ 2017-03-18 3:46 fgao
2017-03-18 4:26 ` Feng Gao
0 siblings, 1 reply; 3+ messages in thread
From: fgao @ 2017-03-18 3:46 UTC (permalink / raw)
To: pablo, netfilter-devel, gfree.wind; +Cc: Gao Feng
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH nf 1/1] netfilter: ctlink: Fix one possible use-after-free in ctnetlink_create_expect
2017-03-18 3:46 [PATCH nf 1/1] netfilter: ctlink: Fix one possible use-after-free in ctnetlink_create_expect fgao
@ 2017-03-18 4:26 ` Feng Gao
2017-03-18 4:39 ` Feng Gao
0 siblings, 1 reply; 3+ messages in thread
From: Feng Gao @ 2017-03-18 4:26 UTC (permalink / raw)
To: Pablo Neira Ayuso, Netfilter Developer Mailing List, Feng Gao; +Cc: Gao Feng
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH nf 1/1] netfilter: ctlink: Fix one possible use-after-free in ctnetlink_create_expect
2017-03-18 4:26 ` Feng Gao
@ 2017-03-18 4:39 ` Feng Gao
0 siblings, 0 replies; 3+ messages in thread
From: Feng Gao @ 2017-03-18 4:39 UTC (permalink / raw)
To: Pablo Neira Ayuso, Netfilter Developer Mailing List, Feng Gao; +Cc: Gao Feng
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-03-18 4:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-18 3:46 [PATCH nf 1/1] netfilter: ctlink: Fix one possible use-after-free in ctnetlink_create_expect fgao
2017-03-18 4:26 ` Feng Gao
2017-03-18 4:39 ` Feng Gao
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).