> Ziyang Xuan (William) <william.xuanziyang@xxxxxxxxxx> wrote:
>>>> Use list_for_each_entry_rcu() with rcu_read_lock() to Iterate over
>>>> nf_tables_flowtables list in __nft_flowtable_type_get() to resolve it.
>>>
>>> I don't think this resolves the described race.
>>>
>>>> Signed-off-by: Ziyang Xuan <william.xuanziyang@xxxxxxxxxx>
>>>> ---
>>>> net/netfilter/nf_tables_api.c | 8 ++++++--
>>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>>>> index fd86f2720c9e..fbf38e32f11d 100644
>>>> --- a/net/netfilter/nf_tables_api.c
>>>> +++ b/net/netfilter/nf_tables_api.c
>>>> @@ -8297,10 +8297,14 @@ static const struct nf_flowtable_type *__nft_flowtable_type_get(u8 family)
>>>> {
>>>> const struct nf_flowtable_type *type;
>>>>
>>>> - list_for_each_entry(type, &nf_tables_flowtables, list) {
>>>> - if (family == type->family)
>>>> + rcu_read_lock()
>>>> + list_for_each_entry_rcu(type, &nf_tables_flowtables, list) {
>>>> + if (family == type->family) {
>>>> + rcu_read_unlock();
>>>> return type;
>>>
>>> This means 'type' can be non-null while module is being unloaded,
>>> before refcount increment.
>>>
>>> You need acquire rcu_read_lock() in the caller, nft_flowtable_type_get,
>>> and release it after refcount on module owner failed or succeeded.
>>> .
>> In fact, I just want to resolve the potential tear-down problem about list entry here.
>
> cpu1 cpu2
> rmmod
> flowtable_type
>
> nft_flowtable_type_get
> __nft_flowtable_type_get
> finds family == type->family
> list_del_rcu(type)
>
> CPU INTERRUPTED
> rmmod completes
>
> nft_flowtable_type_get calls
> if (type != NULL && try_module_get(type->owner))
> ---> UaF
>
> Skeleton fix:
>
> nft_flowtable_type_get(struct net *net, u8 family)
> {
> const struct nf_flowtable_type *type;
>
> + rcu_read_lock();
> type = __nft_flowtable_type_get(family);
> ....
> if (type != NULL && try_module_get(type->owner)) {
> rcu_read_unlock();
> return type;
> }
>
> rcu_read_unlock();
>
> This avoids the above UaF, rmmod cannot complete fully until after
> rcu read lock section is done. (There is a synchronize_rcu in module
> teardown path before the data section is freed).
I see. Thank you for your patient analysis!
>> So I think replace with list_for_each_entry_rcu() can resolve the tear-down problem now.
>
> I don't think so.
> .
>
