diff mbox series

[net,1/1] netfilter: nf_tables: unconditionally flush pending work before notifier

Message ID 20240703223304.1455-2-pablo@netfilter.org
State Accepted
Headers show
Series [net,1/1] netfilter: nf_tables: unconditionally flush pending work before notifier | expand

Commit Message

Pablo Neira Ayuso July 3, 2024, 10:33 p.m. UTC 
From: Florian Westphal <fw@strlen.de>

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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=4fd66a69358fc15ae2ad
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Paolo Abeni July 4, 2024, 1:24 p.m. UTC | #1 
Hi,

On Thu, 2024-07-04 at 00:33 +0200, Pablo Neira Ayuso wrote:
> From: Florian Westphal <fw@strlen.de>
> 
> 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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=4fd66a69358fc15ae2ad
> Signed-off-by: Florian Westphal <fw@strlen.de>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  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
Pablo Neira Ayuso July 4, 2024, 1:29 p.m. UTC | #2 
On Thu, Jul 04, 2024 at 03:24:17PM +0200, Paolo Abeni wrote:
> Hi,
> 
> On Thu, 2024-07-04 at 00:33 +0200, Pablo Neira Ayuso wrote:
> > From: Florian Westphal <fw@strlen.de>
> > 
> > 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@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=4fd66a69358fc15ae2ad
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  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?

Let's do that.

Thanks.
patchwork-bot+netdevbpf@kernel.org July 4, 2024, 1:40 p.m. UTC | #3 
Hello:

This patch was applied to netdev/net.git (main)
by Pablo Neira Ayuso <pablo@netfilter.org>:

On Thu,  4 Jul 2024 00:33:04 +0200 you wrote:
> From: Florian Westphal <fw@strlen.de>
> 
> 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
> 
> [...]

Here is the summary with links:
  - [net,1/1] netfilter: nf_tables: unconditionally flush pending work before notifier
    https://git.kernel.org/netdev/net/c/9f6958ba2e90

You are awesome, thank you!
diff mbox series

Patch

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) &&