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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <266de015-7712-8672-9ca0-67199817d587@virtuozzo.com>
Date:   Mon, 27 Feb 2023 15:43:33 +0200
From:   Alexander Atanasov <alexander.atanasov@...tuozzo.com>
To:     Florian Westphal <fw@...len.de>
Cc:     netdev@...r.kernel.org, ",Pablo Neira Ayuso" <pablo@...filter.org>,
        ",Jozsef Kadlecsik" <kadlec@...filter.org>,
        ",Eric Dumazet" <edumazet@...gle.com>,
        ",David S. Miller" <davem@...emloft.net>,
        ",Jakub Kicinski" <kuba@...nel.org>,
        ",Paolo Abeni" <pabeni@...hat.com>, ",kernel"@openvz.org
Subject: Re: [PATCH] netfilter: nf_tables: always synchronize with readers
 before releasing tables

Hello,

On 27.02.23 14:44, Florian Westphal wrote:
> Alexander Atanasov <alexander.atanasov@...tuozzo.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@...tuozzo.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