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] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 27 Feb 2023 13:44:02 +0100
From:   Florian Westphal <fw@...len.de>
To:     Alexander Atanasov <alexander.atanasov@...tuozzo.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH] netfilter: nf_tables: always synchronize with readers
 before releasing tables

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.

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()?