Re: [PATCH nft] netfilter: nf_tables: Fix pertential data-race in __nft_flowtable_type_get()

[Florian Westphal <fw@strlen.de>]

Ziyang Xuan (William) <william.xuanziyang@huawei.com> 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@huawei.com>
> >> ---
> >>  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).

> So I think replace with list_for_each_entry_rcu() can resolve the tear-down problem now.

I don't think so.