[PATCH] netfilter: nf_tables: always synchronize with readers before releasing tables

[PATCH] netfilter: nf_tables: always synchronize with readers before releasing tables

[Alexander Atanasov]

general protection fault, probably for non-canonical
address 0xdead000000000115: 0000 [#1] PREEMPT SMP NOPTI
RIP: 0010:__nf_tables_dump_rules+0x10d/0x170 [nf_tables]

__nf_tables_dump_rules runs under rcu_read_lock while __nft_release_table
is called from nf_tables_exit_net. commit_mutex is held inside
nf_tables_exit_net but this is not enough to guard against
lockless readers. When __nft_release_table does list_del(&rule->list)
next ptr is poisoned and it crashes while walking the list.

Before calling __nft_release_tables all lockless readers must be done -
to ensure this a call to synchronize_rcu() is required.

nf_tables_exit_net does this in case there is something to abort
inside __nf_tables_abort but it does not do so otherwise.
Fix this by add the missing synchronize_rcu() call before calling
__nft_release_table in the nothing to abort case.

Fixes: 6001a930ce03 ("netfilter: nftables: introduce table ownership")
Signed-off-by: Alexander Atanasov <alexander.atanasov@virtuozzo.com>
---
  net/netfilter/nf_tables_api.c | 6 ++++++
  1 file changed, 6 insertions(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index d73edbd4eec4..849523ece109 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -10333,9 +10333,15 @@ static void __net_exit 
nf_tables_exit_net(struct net *net)
  	struct nftables_pernet *nft_net = nft_pernet(net);
   	mutex_lock(&nft_net->commit_mutex);
+	/* Need to call synchronize_rcu() to let any active rcu lockless
+	 * readers to finish. __nf_tables_abort does this internaly so
+	 * only call it here if there is nothing to abort.
+	 */
  	if (!list_empty(&nft_net->commit_list) ||
  	    !list_empty(&nft_net->module_list))
  		__nf_tables_abort(net, NFNL_ABORT_NONE);
+	else
+		synchronize_rcu();
  	__nft_release_tables(net);
  	mutex_unlock(&nft_net->commit_mutex);
  	WARN_ON_ONCE(!list_empty(&nft_net->tables));
-- 
2.31.1

Re: [PATCH] netfilter: nf_tables: always synchronize with readers before releasing tables

[Florian Westphal]

Alexander Atanasov <alexander.atanasov@virtuozzo.com> wrote:
> general protection fault, probably for non-canonical
> address 0xdead000000000115: 0000 [#1] PREEMPT SMP NOPTI
> RIP: 0010:__nf_tables_dump_rules+0x10d/0x170 [nf_tables]
> 
> __nf_tables_dump_rules runs under rcu_read_lock while __nft_release_table
> is called from nf_tables_exit_net. commit_mutex is held inside
> nf_tables_exit_net but this is not enough to guard against
> lockless readers. When __nft_release_table does list_del(&rule->list)
> next ptr is poisoned and it crashes while walking the list.
> 
> Before calling __nft_release_tables all lockless readers must be done -
> to ensure this a call to synchronize_rcu() is required.
> 
> nf_tables_exit_net does this in case there is something to abort
> inside __nf_tables_abort but it does not do so otherwise.
> Fix this by add the missing synchronize_rcu() call before calling
> __nft_release_table in the nothing to abort case.
> 
> Fixes: 6001a930ce03 ("netfilter: nftables: introduce table ownership")
> Signed-off-by: Alexander Atanasov <alexander.atanasov@virtuozzo.com>
> ---
>  net/netfilter/nf_tables_api.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index d73edbd4eec4..849523ece109 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -10333,9 +10333,15 @@ static void __net_exit nf_tables_exit_net(struct
> net *net)
>  	struct nftables_pernet *nft_net = nft_pernet(net);
>   	mutex_lock(&nft_net->commit_mutex);
> +	/* Need to call synchronize_rcu() to let any active rcu lockless
> +	 * readers to finish. __nf_tables_abort does this internaly so
> +	 * only call it here if there is nothing to abort.
> +	 */
>  	if (!list_empty(&nft_net->commit_list) ||
>  	    !list_empty(&nft_net->module_list))
>  		__nf_tables_abort(net, NFNL_ABORT_NONE);
> +	else
> +		synchronize_rcu();

Wouldn't it be better to just drop those list_empty() checks?
AFAICS __nf_tables_abort will DTRT in that case.

You can still add a comment like the one you added above to make
it clear that we also need to wait for those readers to finish.

Lastly, that list_del() in __nft_release_basechain should probably
be list_del_rcu()?

Re: [PATCH] netfilter: nf_tables: always synchronize with readers before releasing tables

[Alexander Atanasov]

Hello,

On 27.02.23 14:44, Florian Westphal wrote:
> Alexander Atanasov <alexander.atanasov@virtuozzo.com> wrote:
>> general protection fault, probably for non-canonical
>> address 0xdead000000000115: 0000 [#1] PREEMPT SMP NOPTI
>> RIP: 0010:__nf_tables_dump_rules+0x10d/0x170 [nf_tables]
>>
>> __nf_tables_dump_rules runs under rcu_read_lock while __nft_release_table
>> is called from nf_tables_exit_net. commit_mutex is held inside
>> nf_tables_exit_net but this is not enough to guard against
>> lockless readers. When __nft_release_table does list_del(&rule->list)
>> next ptr is poisoned and it crashes while walking the list.
>>
>> Before calling __nft_release_tables all lockless readers must be done -
>> to ensure this a call to synchronize_rcu() is required.
>>
>> nf_tables_exit_net does this in case there is something to abort
>> inside __nf_tables_abort but it does not do so otherwise.
>> Fix this by add the missing synchronize_rcu() call before calling
>> __nft_release_table in the nothing to abort case.
>>
>> Fixes: 6001a930ce03 ("netfilter: nftables: introduce table ownership")
>> Signed-off-by: Alexander Atanasov <alexander.atanasov@virtuozzo.com>
>> ---
>>   net/netfilter/nf_tables_api.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>> index d73edbd4eec4..849523ece109 100644
>> --- a/net/netfilter/nf_tables_api.c
>> +++ b/net/netfilter/nf_tables_api.c
>> @@ -10333,9 +10333,15 @@ static void __net_exit nf_tables_exit_net(struct
>> net *net)
>>   	struct nftables_pernet *nft_net = nft_pernet(net);
>>    	mutex_lock(&nft_net->commit_mutex);
>> +	/* Need to call synchronize_rcu() to let any active rcu lockless
>> +	 * readers to finish. __nf_tables_abort does this internaly so
>> +	 * only call it here if there is nothing to abort.
>> +	 */
>>   	if (!list_empty(&nft_net->commit_list) ||
>>   	    !list_empty(&nft_net->module_list))
>>   		__nf_tables_abort(net, NFNL_ABORT_NONE);
>> +	else
>> +		synchronize_rcu();
> 
> Wouldn't it be better to just drop those list_empty() checks?
> AFAICS __nf_tables_abort will DTRT in that case.

Ok, i will drop the checks.

> You can still add a comment like the one you added above to make
> it clear that we also need to wait for those readers to finish.

Ok.

> Lastly, that list_del() in __nft_release_basechain should probably
> be list_del_rcu()?

I am still in process of untwisting that place but so far.
Simple change to list_del_rcu wouldn't help as it wouldn't in 
__nft_release_table:

list_del(&rule->list);
ctx->chain->use--;
nf_tables_rule_release(ctx, rule) {
	nft_rule_expr_deactivate(ctx, rule, NFT_TRANS_RELEASE);
	nf_tables_rule_destroy(ctx, rule) {
		kfree(rule); <-- freed here
	}
}

List traversal would work but instead of crash it would become use after 
free.
Adding synchronize_rcu() before list iterattion there will probably do, 
it is already under commit_mutex when called from nf_tables_netdev_event.


-- 
Regards,
Alexander Atanasov

Re: [PATCH] netfilter: nf_tables: always synchronize with readers before releasing tables

[Alexander Atanasov]

On 27.02.23 18:11, Florian Westphal wrote:
> Alexander Atanasov <alexander.atanasov@virtuozzo.com> wrote:
>>> Lastly, that list_del() in __nft_release_basechain should probably
>>> be list_del_rcu()?
>>
>> I am still in process of untwisting that place but so far.
>> Simple change to list_del_rcu wouldn't help as it wouldn't in
>> __nft_release_table:
>>
>> list_del(&rule->list);
>> ctx->chain->use--;
>> nf_tables_rule_release(ctx, rule) {
>> 	nft_rule_expr_deactivate(ctx, rule, NFT_TRANS_RELEASE);
>> 	nf_tables_rule_destroy(ctx, rule) {
>> 		kfree(rule); <-- freed here
>> 	}
>> }
>>
>> List traversal would work but instead of crash it would become use after
>> free.
>> Adding synchronize_rcu() before list iterattion there will probably do, it
>> is already under commit_mutex when called from nf_tables_netdev_event.
> 
> Hmm, please wait.  I have to look at this in more detail.
> I don't see a race conditon in the first place.
>
> netns dismantling already does synchronize_rcu(), so I don't see how we > can have this uaf in the first place.

As i said i am still trying to figure out the basechain place,
where is that synchronize_rcu() call done?

> Do you see this with current kernels or did the splat happen with
> an older version?

It's with a bit older kernel but there is no significant difference
wrt nf_tables_api code.
I will prepare a more detailed report for you. Unfortunately there is
no reproducer.

-- 
Regards,
Alexander Atanasov

Re: [PATCH] netfilter: nf_tables: always synchronize with readers before releasing tables

[Florian Westphal]

Alexander Atanasov <alexander.atanasov@virtuozzo.com> wrote:
> As i said i am still trying to figure out the basechain place,
> where is that synchronize_rcu() call done?

cleanup_net() in net/core/net_namespace.c.

pre_exit handlers run, then synchronize_rcu, then the
normal exit handlers, then exit_batch.

> > Do you see this with current kernels or did the splat happen with
> > an older version?
> 
> It's with a bit older kernel but there is no significant difference
> wrt nf_tables_api code.
> I will prepare a more detailed report for you.

Thanks.

Re: [PATCH] netfilter: nf_tables: always synchronize with readers before releasing tables

[Alexander Atanasov]

On 28.02.23 1:31, Florian Westphal wrote:
> Alexander Atanasov <alexander.atanasov@virtuozzo.com> wrote:
>> As i said i am still trying to figure out the basechain place,
>> where is that synchronize_rcu() call done?
> 
> cleanup_net() in net/core/net_namespace.c.
> 
> pre_exit handlers run, then synchronize_rcu, then the
> normal exit handlers, then exit_batch.

It prevents anyone new to find the namespace but it does not guard 
against the ones that have already found it.
What stops them to enter a rcu_read_lock() section after the synchronize 
call in cleanup_net() is done and race with the exit handler?

synchronize_rcu() must be called with the commit_mutex held to be safe 
against lock less readers using data protected with commit_mutext.

-- 
Regards,
Alexander Atanasov

Re: [PATCH] netfilter: nf_tables: always synchronize with readers before releasing tables

[Florian Westphal]

Alexander Atanasov <alexander.atanasov@virtuozzo.com> wrote:
> On 28.02.23 1:31, Florian Westphal wrote:
> > Alexander Atanasov <alexander.atanasov@virtuozzo.com> wrote:
> > > As i said i am still trying to figure out the basechain place,
> > > where is that synchronize_rcu() call done?
> > 
> > cleanup_net() in net/core/net_namespace.c.
> > 
> > pre_exit handlers run, then synchronize_rcu, then the
> > normal exit handlers, then exit_batch.
> 
> It prevents anyone new to find the namespace but it does not guard against
> the ones that have already found it.

The netns is being dismantled, how can there be any process left?

> What stops them to enter a rcu_read_lock() section after the synchronize
> call in cleanup_net() is done and race with the exit handler?

There should be no task in the first place.

> synchronize_rcu() must be called with the commit_mutex held to be safe
> against lock less readers using data protected with commit_mutext.

Sorry, I do not understand this bug nor the fix.