* [PATCH RESENT nf 1/1] netfilter: ctlink: Fix one possible use-after-free in ctnetlink_create_expect
@ 2017-03-22 1:25 gfree.wind
2017-03-22 1:36 ` Gao Feng
0 siblings, 1 reply; 3+ messages in thread
From: gfree.wind @ 2017-03-22 1:25 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 RESENT nf 1/1] netfilter: ctlink: Fix one possible use-after-free in ctnetlink_create_expect
2017-03-22 1:25 [PATCH RESENT nf 1/1] netfilter: ctlink: Fix one possible use-after-free in ctnetlink_create_expect gfree.wind
@ 2017-03-22 1:36 ` Gao Feng
2017-03-22 3:29 ` Gao Feng
0 siblings, 1 reply; 3+ messages in thread
From: Gao Feng @ 2017-03-22 1:36 UTC (permalink / raw)
To: pablo, netfilter-devel; +Cc: 'Gao Feng'
Hi Pablo,
> -----Original Message-----
> From: netfilter-devel-owner@vger.kernel.org
> [mailto:netfilter-devel-owner@vger.kernel.org] On Behalf Of
> gfree.wind@foxmail.com
> Sent: Wednesday, March 22, 2017 9:25 AM
> To: pablo@netfilter.org; netfilter-devel@vger.kernel.org;
> gfree.wind@foxmail.com
> Cc: Gao Feng <fgao@ikuai8.com>
> Subject: [PATCH RESENT nf 1/1] netfilter: ctlink: Fix one possible
use-after-free
> in ctnetlink_create_expect
>
> 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
>
>
>
>
> --
> 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
Let's me explain why I resend this patch without any change.
Actually I did checked all callers of __nf_conntrack_helper_find before.
There are
1. ctnetlink_create_expect: No rcu lock, and it is my fix;
2. ctnetlink_change_helper: It is protected by
spin_lock_bh(&nf_conntrack_expect_lock), although no rcu. I think it is not
necessary to add rcu again;
3. ctnetlink_create_conntrack: rcu lock already
4. ctnetlink_glue_attach_expect: Its caller holds the rcu lock already.
nfnl_ct_hook->ctnetlink_glue_hook->ctnetlink_glue_attach_expect.
The nfnl_ct_hook is one RCU pointer, it should be protected by RCU.
5. nf_conntrack_helper_try_module_get: It returns the helper pointer. So it
is necessary that its caller hold the rcu lock.
But I checked the caller of nf_conntrack_helper_try_module_get.
They are xt_ct_set_helper and ovs_ct_add_helper, and it seems they don't
hold the rcu lock.
It should be another patch I think.
What's your opinion, Pablo?
Best Regards
Feng
^ permalink raw reply [flat|nested] 3+ messages in thread* RE: [PATCH RESENT nf 1/1] netfilter: ctlink: Fix one possible use-after-free in ctnetlink_create_expect
2017-03-22 1:36 ` Gao Feng
@ 2017-03-22 3:29 ` Gao Feng
0 siblings, 0 replies; 3+ messages in thread
From: Gao Feng @ 2017-03-22 3:29 UTC (permalink / raw)
To: pablo, netfilter-devel; +Cc: 'Gao Feng'
Hi Pablo,
> -----Original Message-----
> From: netfilter-devel-owner@vger.kernel.org
> [mailto:netfilter-devel-owner@vger.kernel.org] On Behalf Of Gao Feng
> Sent: Wednesday, March 22, 2017 9:37 AM
> To: pablo@netfilter.org; netfilter-devel@vger.kernel.org
> Cc: 'Gao Feng' <fgao@ikuai8.com>
> Subject: RE: [PATCH RESENT nf 1/1] netfilter: ctlink: Fix one possible
> use-after-free in ctnetlink_create_expect
>
> Hi Pablo,
>
> > -----Original Message-----
> > From: netfilter-devel-owner@vger.kernel.org
> > [mailto:netfilter-devel-owner@vger.kernel.org] On Behalf Of
> > gfree.wind@foxmail.com
> > Sent: Wednesday, March 22, 2017 9:25 AM
> > To: pablo@netfilter.org; netfilter-devel@vger.kernel.org;
> > gfree.wind@foxmail.com
> > Cc: Gao Feng <fgao@ikuai8.com>
> > Subject: [PATCH RESENT nf 1/1] netfilter: ctlink: Fix one possible
> use-after-free
> > in ctnetlink_create_expect
> >
> > 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
> >
> >
> >
> >
> > --
> > 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
>
> Let's me explain why I resend this patch without any change.
> Actually I did checked all callers of __nf_conntrack_helper_find before.
>
> There are
> 1. ctnetlink_create_expect: No rcu lock, and it is my fix; 2.
> ctnetlink_change_helper: It is protected by
> spin_lock_bh(&nf_conntrack_expect_lock), although no rcu. I think it is
not
> necessary to add rcu again; 3. ctnetlink_create_conntrack: rcu lock
already 4.
> ctnetlink_glue_attach_expect: Its caller holds the rcu lock already.
> nfnl_ct_hook->ctnetlink_glue_hook->ctnetlink_glue_attach_expect.
> The nfnl_ct_hook is one RCU pointer, it should be protected by RCU.
> 5. nf_conntrack_helper_try_module_get: It returns the helper pointer. So
it is
> necessary that its caller hold the rcu lock.
>
> But I checked the caller of nf_conntrack_helper_try_module_get.
> They are xt_ct_set_helper and ovs_ct_add_helper, and it seems they don't
hold
> the rcu lock.
> It should be another patch I think.
Now I change my thought, the nf_conntrack_helper_try_module_get should be
fixed in this patch.
Because when try_module_get return successfully, we have hold the module.
So it is ok to use rcu lock in this function inside.
I would send the v2 patch.
Best Regards
Feng
>
> What's your opinion, Pablo?
>
> Best 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-03-22 3:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-22 1:25 [PATCH RESENT nf 1/1] netfilter: ctlink: Fix one possible use-after-free in ctnetlink_create_expect gfree.wind
2017-03-22 1:36 ` Gao Feng
2017-03-22 3:29 ` Gao Feng
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).