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
| ||
|
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
Powered by blists - more mailing lists