Re: [PATCH nf 5/5] netfilter: nfnl_cthelper: fix a race when walk the nf_ct_helper_hash table

[Liping Zhang <zlpnobody@gmail.com>]

2017-03-21 18:33 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> On Sun, Mar 19, 2017 at 10:36:02PM +0800, Liping Zhang wrote:
>> From: Liping Zhang <zlpnobody@gmail.com>
>>
>> The nf_ct_helper_hash table is protected by nf_ct_helper_mutex, while
>> nfct_helper operation is protected by nfnl_lock(NFNL_SUBSYS_CTHELPER).
>> So it's possible that one CPU is walking the nf_ct_helper_hash for
>> cthelper add/get/del, another cpu is doing nf_conntrack_helpers_unregister
>> at the same time. This is dangrous, and may cause use after free error.
>>
>> Note, delete operation will flush all cthelpers added via nfnetlink, so
>> using rcu to do protect is not easy.
>>
>> Now introduce a dummy list to record all the cthelpers added via
>> nfnetlink, then we can walk the dummy list instead of walking the
>> nf_ct_helper_hash. Also, keep nfnl_cthelper_dump_table unchanged, it
>> may be invoked without nfnl_lock(NFNL_SUBSYS_CTHELPER) held.
>>
>> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
>> ---
>>  net/netfilter/nfnetlink_cthelper.c | 176 +++++++++++++++++++------------------
>>  1 file changed, 89 insertions(+), 87 deletions(-)
>>
>> diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
>> index fc4733f..47424ec 100644
>> --- a/net/netfilter/nfnetlink_cthelper.c
>> +++ b/net/netfilter/nfnetlink_cthelper.c
>> @@ -32,6 +32,13 @@ MODULE_LICENSE("GPL");
>>  MODULE_AUTHOR("Pablo Neira Ayuso <pablo@netfilter.org>");
>>  MODULE_DESCRIPTION("nfnl_cthelper: User-space connection tracking helpers");
>>
>> +struct nfnl_cthelper {
>> +     struct list_head                list;
>> +     struct nf_conntrack_helper      *helper;
>> +};
>> +
>> +static LIST_HEAD(nfnl_cthelper_list);
>
> We need a field possible_net_t so we can store what netns this helper
> belongs to, thus in case of flush command, we just remove the helpers
> that this netns owns.

Yes, good point, I will send v2.

Thanks Pablo.