Hi,
On Thu, 2024-07-04 at 00:33 +0200, Pablo Neira Ayuso wrote:
> From: Florian Westphal <fw@xxxxxxxxx>
>
> syzbot reports:
>
> KASAN: slab-uaf in nft_ctx_update include/net/netfilter/nf_tables.h:1831
> KASAN: slab-uaf in nft_commit_release net/netfilter/nf_tables_api.c:9530
> KASAN: slab-uaf int nf_tables_trans_destroy_work+0x152b/0x1750 net/netfilter/nf_tables_api.c:9597
> Read of size 2 at addr ffff88802b0051c4 by task kworker/1:1/45
> [..]
> Workqueue: events nf_tables_trans_destroy_work
> Call Trace:
> nft_ctx_update include/net/netfilter/nf_tables.h:1831 [inline]
> nft_commit_release net/netfilter/nf_tables_api.c:9530 [inline]
> nf_tables_trans_destroy_work+0x152b/0x1750 net/netfilter/nf_tables_api.c:9597
>
> Problem is that the notifier does a conditional flush, but its possible
> that the table-to-be-removed is still referenced by transactions being
> processed by the worker, so we need to flush unconditionally.
>
> We could make the flush_work depend on whether we found a table to delete
> in nf-next to avoid the flush for most cases.
>
> AFAICS this problem is only exposed in nf-next, with
> commit e169285f8c56 ("netfilter: nf_tables: do not store nft_ctx in transaction objects"),
> with this commit applied there is an unconditional fetch of
> table->family which is whats triggering the above splat.
>
> Fixes: 2c9f0293280e ("netfilter: nf_tables: flush pending destroy work before netlink notifier")
> Reported-and-tested-by: syzbot+4fd66a69358fc15ae2ad@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=4fd66a69358fc15ae2ad
> Signed-off-by: Florian Westphal <fw@xxxxxxxxx>
> Signed-off-by: Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>
> ---
> net/netfilter/nf_tables_api.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index e8dcf41d360d..081c08536d0f 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -11483,8 +11483,7 @@ static int nft_rcv_nl_event(struct notifier_block *this, unsigned long event,
>
> gc_seq = nft_gc_seq_begin(nft_net);
>
> - if (!list_empty(&nf_tables_destroy_list))
> - nf_tables_trans_destroy_flush_work();
> + nf_tables_trans_destroy_flush_work();
> again:
> list_for_each_entry(table, &nft_net->tables, list) {
> if (nft_table_has_owner(table) &&
It look like there is still some discussion around this patch, but I
guess we can still take it and in the worst case scenario a follow-up
will surface, right?
Thanks!
Paolo
