On Sat, Mar 18, 2017 at 12:26 PM, Feng Gao <gfree.wind@xxxxxxxxx> wrote:
> Hi Pablo,
>
> On Sat, Mar 18, 2017 at 11:46 AM, <fgao@xxxxxxxxxx> wrote:
>> 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
>>
>>
>
> 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
--
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
