Hi Pablo, > -----Original Message----- > From: netfilter-devel-owner@xxxxxxxxxxxxxxx > [mailto:netfilter-devel-owner@xxxxxxxxxxxxxxx] On Behalf Of Gao Feng > Sent: Wednesday, March 22, 2017 9:37 AM > To: pablo@xxxxxxxxxxxxx; netfilter-devel@xxxxxxxxxxxxxxx > Cc: 'Gao Feng' <fgao@xxxxxxxxxx> > 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@xxxxxxxxxxxxxxx > > [mailto:netfilter-devel-owner@xxxxxxxxxxxxxxx] On Behalf Of > > gfree.wind@xxxxxxxxxxx > > Sent: Wednesday, March 22, 2017 9:25 AM > > To: pablo@xxxxxxxxxxxxx; netfilter-devel@xxxxxxxxxxxxxxx; > > gfree.wind@xxxxxxxxxxx > > Cc: Gao Feng <fgao@xxxxxxxxxx> > > Subject: [PATCH RESENT nf 1/1] netfilter: ctlink: Fix one possible > use-after-free > > in ctnetlink_create_expect > > > > From: Gao Feng <fgao@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx More majordomo info at > http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
