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
> 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