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

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

[Ziyang Xuan]

nft_unregister_flowtable_type() within nf_flow_inet_module_exit() can
concurrent with __nft_flowtable_type_get() within nf_tables_newflowtable().
And thhere is not any protection when iterate over nf_tables_flowtables
list in __nft_flowtable_type_get(). Therefore, there is pertential
data-race of nf_tables_flowtables list entry.

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.

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;
+		}
 	}
+	rcu_read_unlock();
 	return NULL;
 }
 
-- 
2.25.1

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

[Florian Westphal]

Ziyang Xuan <william.xuanziyang@huawei.com> wrote:
> nft_unregister_flowtable_type() within nf_flow_inet_module_exit() can
> concurrent with __nft_flowtable_type_get() within nf_tables_newflowtable().
> And thhere is not any protection when iterate over nf_tables_flowtables
> list in __nft_flowtable_type_get(). Therefore, there is pertential
> data-race of nf_tables_flowtables list entry.
> 
> 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.

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

[Pablo Neira Ayuso]

On Tue, Apr 02, 2024 at 12:56:42PM +0200, Florian Westphal wrote:
> Ziyang Xuan <william.xuanziyang@huawei.com> wrote:
> > nft_unregister_flowtable_type() within nf_flow_inet_module_exit() can
> > concurrent with __nft_flowtable_type_get() within nf_tables_newflowtable().
> > And thhere is not any protection when iterate over nf_tables_flowtables
> > list in __nft_flowtable_type_get(). Therefore, there is pertential
> > data-race of nf_tables_flowtables list entry.
> > 
> > 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.

And these need to be rcu protected:

static LIST_HEAD(nf_tables_expressions);
static LIST_HEAD(nf_tables_objects);
static LIST_HEAD(nf_tables_flowtables);

for a complete fix for:

f102d66b335a ("netfilter: nf_tables: use dedicated mutex to guard transactions")

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

[Ziyang Xuan (William)]

> On Tue, Apr 02, 2024 at 12:56:42PM +0200, Florian Westphal wrote:
>> Ziyang Xuan <william.xuanziyang@huawei.com> wrote:
>>> nft_unregister_flowtable_type() within nf_flow_inet_module_exit() can
>>> concurrent with __nft_flowtable_type_get() within nf_tables_newflowtable().
>>> And thhere is not any protection when iterate over nf_tables_flowtables
>>> list in __nft_flowtable_type_get(). Therefore, there is pertential
>>> data-race of nf_tables_flowtables list entry.
>>>
>>> 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.
> 
> And these need to be rcu protected:
> 
> static LIST_HEAD(nf_tables_expressions);
> static LIST_HEAD(nf_tables_objects);
> static LIST_HEAD(nf_tables_flowtables);
> 
> for a complete fix for:
> 
> f102d66b335a ("netfilter: nf_tables: use dedicated mutex to guard transactions")
> .
> 

I would be happy to do these. Thank you for your kind remind!


William Xuan
Best regards.

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

[Ziyang Xuan (William)]

> Ziyang Xuan <william.xuanziyang@huawei.com> wrote:
>> nft_unregister_flowtable_type() within nf_flow_inet_module_exit() can
>> concurrent with __nft_flowtable_type_get() within nf_tables_newflowtable().
>> And thhere is not any protection when iterate over nf_tables_flowtables
>> list in __nft_flowtable_type_get(). Therefore, there is pertential
>> data-race of nf_tables_flowtables list entry.
>>
>> 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.

As following:

void nft_unregister_flowtable_type(struct nf_flowtable_type *type)
{
	nfnl_lock(NFNL_SUBSYS_NFTABLES);
	/* just use WRITE_ONCE() inside, but not rcu_assign_pointer().
	 * It can not form competition protection with rcu_read_lock().
	 */
	list_del_rcu(&type->list);
	nfnl_unlock(NFNL_SUBSYS_NFTABLES);
}

static const struct nf_flowtable_type *__nft_flowtable_type_get(u8 family)
{
	const struct nf_flowtable_type *type;

	/* Get list entry use READ_ONCE() inside. And I think rcu_read_lock() maybe unnecessary. */
	list_for_each_entry_rcu(type, &nf_tables_flowtables, list) {
		if (family == type->family)
			return type;
	}
	return NULL;
}

static const struct nf_flowtable_type *
nft_flowtable_type_get(struct net *net, u8 family)
{
	const struct nf_flowtable_type *type;

	type = __nft_flowtable_type_get(family);
	/* If type is non-NULL, try to get module. If the module
	 * state is not ok, it will fail here.
	 */
	if (type != NULL && try_module_get(type->owner))
		return type;

	lockdep_nfnl_nft_mutex_not_held();
#ifdef CONFIG_MODULES
	if (type == NULL) {
		if (nft_request_module(net, "nf-flowtable-%u", family) == -EAGAIN)
			return ERR_PTR(-EAGAIN);
	}
#endif
	return ERR_PTR(-ENOENT);
}

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


William Xuan
Best regards.

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

[Florian Westphal]

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.

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

[Ziyang Xuan (William)]

> 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).

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.
> .
>