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

[Feng Gao <gfree.wind@gmail.com>]

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