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

["Gao Feng" <gfree.wind@foxmail.com>]

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