[PATCH net 1/1] netfilter: flowtable: Fix use after free after freeing flow table

[PATCH net 1/1] netfilter: flowtable: Fix use after free after freeing flow table

[Paul Blakey]

To clear the flow table on flow table free, the following sequence
normally happens in order:
  1) gc_step work is stopped to disable any further stats/del requests.
  2) All flow table entries are set to teardown state.
  3) Run gc_step which will queue HW del work for each flow table entry.
  4) Waiting for the above del work to finish (flush).
  5) Run gc_step again, deleting all entries from the flow table.
  6) Flow table is freed.

But if a flow table entry already has pending HW stats or HW add work
step 3 will not queue HW del work (it will be skipped), step 4 will wait
for the pending add/stats to finish, and step 5 will queue HW del work
which might execute after freeing of the flow table.

To fix the above, add another flush (before step 2 above) to wait for
any pending add/stats work to finish, so next steps will work as expected
(schedule HW del, wait for it, then delete the flow from the flow table).

Stack trace:
[47773.882335] BUG: KASAN: use-after-free in down_read+0x99/0x460
[47773.883634] Write of size 8 at addr ffff888103b45aa8 by task kworker/u20:6/543704
[47773.885634] CPU: 3 PID: 543704 Comm: kworker/u20:6 Not tainted 5.12.0-rc7+ #2
[47773.886745] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009)
[47773.888438] Workqueue: nf_ft_offload_del flow_offload_work_handler [nf_flow_table]
[47773.889727] Call Trace:
[47773.890214]  dump_stack+0xbb/0x107
[47773.890818]  print_address_description.constprop.0+0x18/0x140
[47773.892990]  kasan_report.cold+0x7c/0xd8
[47773.894459]  kasan_check_range+0x145/0x1a0
[47773.895174]  down_read+0x99/0x460
[47773.899706]  nf_flow_offload_tuple+0x24f/0x3c0 [nf_flow_table]
[47773.907137]  flow_offload_work_handler+0x72d/0xbe0 [nf_flow_table]
[47773.913372]  process_one_work+0x8ac/0x14e0
[47773.921325]
[47773.921325] Allocated by task 592159:
[47773.922031]  kasan_save_stack+0x1b/0x40
[47773.922730]  __kasan_kmalloc+0x7a/0x90
[47773.923411]  tcf_ct_flow_table_get+0x3cb/0x1230 [act_ct]
[47773.924363]  tcf_ct_init+0x71c/0x1156 [act_ct]
[47773.925207]  tcf_action_init_1+0x45b/0x700
[47773.925987]  tcf_action_init+0x453/0x6b0
[47773.926692]  tcf_exts_validate+0x3d0/0x600
[47773.927419]  fl_change+0x757/0x4a51 [cls_flower]
[47773.928227]  tc_new_tfilter+0x89a/0x2070
[47773.936652]
[47773.936652] Freed by task 543704:
[47773.937303]  kasan_save_stack+0x1b/0x40
[47773.938039]  kasan_set_track+0x1c/0x30
[47773.938731]  kasan_set_free_info+0x20/0x30
[47773.939467]  __kasan_slab_free+0xe7/0x120
[47773.940194]  slab_free_freelist_hook+0x86/0x190
[47773.941038]  kfree+0xce/0x3a0
[47773.941644]  tcf_ct_flow_table_cleanup_work+0x1b/0x30 [act_ct]
[47773.942656]  process_one_work+0x8ac/0x14e0

Fixes: c29f74e0df7a ("netfilter: nf_flow_table: hardware offload support")
Signed-off-by: Paul Blakey <paulb@nvidia.com>
---
 net/netfilter/nf_flow_table_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index f2def06d1070..19fd3b5f8a1b 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -605,6 +605,7 @@ void nf_flow_table_free(struct nf_flowtable *flow_table)
 	mutex_unlock(&flowtable_lock);
 
 	cancel_delayed_work_sync(&flow_table->gc_work);
+	nf_flow_table_offload_flush(flow_table);
 	nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
 	nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, NULL);
 	nf_flow_table_offload_flush(flow_table);
-- 
2.30.1

Re: [PATCH net 1/1] netfilter: flowtable: Fix use after free after freeing flow table

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

Hi Paul,

On Thu, Aug 18, 2022 at 10:27:54AM +0300, Paul Blakey wrote:
[...]
> diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
> index f2def06d1070..19fd3b5f8a1b 100644
> --- a/net/netfilter/nf_flow_table_core.c
> +++ b/net/netfilter/nf_flow_table_core.c
> @@ -605,6 +605,7 @@ void nf_flow_table_free(struct nf_flowtable *flow_table)
>  	mutex_unlock(&flowtable_lock);
>  
>  	cancel_delayed_work_sync(&flow_table->gc_work);
> +	nf_flow_table_offload_flush(flow_table);
>  	nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
>  	nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, NULL);
>  	nf_flow_table_offload_flush(flow_table);

patch looks very similar to:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/1633854320-12326-1-git-send-email-volodymyr.mytnyk@plvision.eu/

I proposed these two instead to avoid reiterative calls to flush from
the cleanup path (see attached).

It should be possible to either take your patch to nf.git (easier for
-stable backport), then look into my patches for nf-next.git, would
you pick up on these follow up?

[-- Attachment #2: 0001-netfilter-flowtable-add-nf_flow_table_gc_run.patch --]
[-- Type: text/x-diff, Size: 2625 bytes --]

From 81dd33c687b1e868b40e1aac066529d93fa396d8 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Thu, 18 Nov 2021 22:16:37 +0100
Subject: [PATCH 1/2] netfilter: flowtable: add nf_flow_table_gc_run()

Expose nf_flow_table_gc_run() to force a garbage collector run from the
offload infrastructure.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_flow_table.h |  1 +
 net/netfilter/nf_flow_table_core.c    | 13 +++++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
index a3647fadf1cc..6dc19d1ceeff 100644
--- a/include/net/netfilter/nf_flow_table.h
+++ b/include/net/netfilter/nf_flow_table.h
@@ -264,6 +264,7 @@ void flow_offload_refresh(struct nf_flowtable *flow_table,
 
 struct flow_offload_tuple_rhash *flow_offload_lookup(struct nf_flowtable *flow_table,
 						     struct flow_offload_tuple *tuple);
+void nf_flow_table_gc_run(struct nf_flowtable *flow_table);
 void nf_flow_table_gc_cleanup(struct nf_flowtable *flowtable,
 			      struct net_device *dev);
 void nf_flow_table_cleanup(struct net_device *dev);
diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 87a7388b6c89..37fd6c646ccb 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -474,12 +474,17 @@ static void nf_flow_offload_gc_step(struct flow_offload *flow, void *data)
 	}
 }
 
+void nf_flow_table_gc_run(struct nf_flowtable *flow_table)
+{
+	nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, flow_table);
+}
+
 static void nf_flow_offload_work_gc(struct work_struct *work)
 {
 	struct nf_flowtable *flow_table;
 
 	flow_table = container_of(work, struct nf_flowtable, gc_work.work);
-	nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, flow_table);
+	nf_flow_table_gc_run(flow_table);
 	queue_delayed_work(system_power_efficient_wq, &flow_table->gc_work, HZ);
 }
 
@@ -637,11 +642,11 @@ void nf_flow_table_free(struct nf_flowtable *flow_table)
 
 	cancel_delayed_work_sync(&flow_table->gc_work);
 	nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
-	nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, flow_table);
+	nf_flow_table_gc_run(flow_table);
 	nf_flow_table_offload_flush(flow_table);
 	if (nf_flowtable_hw_offload(flow_table))
-		nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step,
-				      flow_table);
+		nf_flow_table_gc_run(flow_table);
+
 	rhashtable_destroy(&flow_table->rhashtable);
 }
 EXPORT_SYMBOL_GPL(nf_flow_table_free);
-- 
2.30.2


[-- Attachment #3: 0002-netfilter-flowtable-fix-stuck-flows-on-cleanup-due-t.patch --]
[-- Type: text/x-diff, Size: 2631 bytes --]

From 737a4ff4df8b3e6791352b2b379e102f7682fa0f Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Thu, 18 Nov 2021 22:24:15 +0100
Subject: [PATCH 2/2] netfilter: flowtable: fix stuck flows on cleanup due to
 pending work

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 include/net/netfilter/nf_flow_table.h | 2 ++
 net/netfilter/nf_flow_table_core.c    | 7 +++----
 net/netfilter/nf_flow_table_offload.c | 8 ++++++++
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/include/net/netfilter/nf_flow_table.h b/include/net/netfilter/nf_flow_table.h
index 6dc19d1ceeff..4c3d537f1069 100644
--- a/include/net/netfilter/nf_flow_table.h
+++ b/include/net/netfilter/nf_flow_table.h
@@ -301,6 +301,8 @@ void nf_flow_offload_stats(struct nf_flowtable *flowtable,
 			   struct flow_offload *flow);
 
 void nf_flow_table_offload_flush(struct nf_flowtable *flowtable);
+void nf_flow_table_offload_flush_cleanup(struct nf_flowtable *flowtable);
+
 int nf_flow_table_offload_setup(struct nf_flowtable *flowtable,
 				struct net_device *dev,
 				enum flow_block_command cmd);
diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
index 37fd6c646ccb..fddc44de3550 100644
--- a/net/netfilter/nf_flow_table_core.c
+++ b/net/netfilter/nf_flow_table_core.c
@@ -641,12 +641,11 @@ void nf_flow_table_free(struct nf_flowtable *flow_table)
 	mutex_unlock(&flowtable_lock);
 
 	cancel_delayed_work_sync(&flow_table->gc_work);
+	nf_flow_table_offload_flush(flow_table);
+	/* ... no more pending work after this stage ... */
 	nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
 	nf_flow_table_gc_run(flow_table);
-	nf_flow_table_offload_flush(flow_table);
-	if (nf_flowtable_hw_offload(flow_table))
-		nf_flow_table_gc_run(flow_table);
-
+	nf_flow_table_offload_flush_cleanup(flow_table);
 	rhashtable_destroy(&flow_table->rhashtable);
 }
 EXPORT_SYMBOL_GPL(nf_flow_table_free);
diff --git a/net/netfilter/nf_flow_table_offload.c b/net/netfilter/nf_flow_table_offload.c
index b561e0a44a45..c4559fae8acd 100644
--- a/net/netfilter/nf_flow_table_offload.c
+++ b/net/netfilter/nf_flow_table_offload.c
@@ -1050,6 +1050,14 @@ void nf_flow_offload_stats(struct nf_flowtable *flowtable,
 	flow_offload_queue_work(offload);
 }
 
+void nf_flow_table_offload_flush_cleanup(struct nf_flowtable *flowtable)
+{
+	if (nf_flowtable_hw_offload(flowtable)) {
+		flush_workqueue(nf_flow_offload_del_wq);
+		nf_flow_table_gc_run(flowtable);
+	}
+}
+
 void nf_flow_table_offload_flush(struct nf_flowtable *flowtable)
 {
 	if (nf_flowtable_hw_offload(flowtable)) {
-- 
2.30.2

Re: [PATCH net 1/1] netfilter: flowtable: Fix use after free after freeing flow table

[Paul Blakey]

On 19/08/2022 02:04, Pablo Neira Ayuso wrote:
> Hi Paul,
> 
> On Thu, Aug 18, 2022 at 10:27:54AM +0300, Paul Blakey wrote:
> [...]
>> diff --git a/net/netfilter/nf_flow_table_core.c b/net/netfilter/nf_flow_table_core.c
>> index f2def06d1070..19fd3b5f8a1b 100644
>> --- a/net/netfilter/nf_flow_table_core.c
>> +++ b/net/netfilter/nf_flow_table_core.c
>> @@ -605,6 +605,7 @@ void nf_flow_table_free(struct nf_flowtable *flow_table)
>>   	mutex_unlock(&flowtable_lock);
>>   
>>   	cancel_delayed_work_sync(&flow_table->gc_work);
>> +	nf_flow_table_offload_flush(flow_table);
>>   	nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
>>   	nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, NULL);
>>   	nf_flow_table_offload_flush(flow_table);
> 
> patch looks very similar to:
> 
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/1633854320-12326-1-git-send-email-volodymyr.mytnyk@plvision.eu/
> 
> I proposed these two instead to avoid reiterative calls to flush from
> the cleanup path (see attached).
> 
> It should be possible to either take your patch to nf.git (easier for
> -stable backport), then look into my patches for nf-next.git, would
> you pick up on these follow up?


Hi!

The only functional difference here (for HW table) is your patches call 
flush just for the del workqueue instead of del/stats/add, right?

Because in the end you do:
cancel_delayed_work_sync(&flow_table->gc_work);
nf_flow_table_offload_flush(flow_table);
nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
nf_flow_table_gc_run(flow_table);
nf_flow_table_offload_flush_cleanup(flow_table);


resulting in the following sequence (after expending flush_cleanup()):

cancel_delayed_work_sync(&flow_table->gc_work);
nf_flow_table_offload_flush(flow_table);
nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
nf_flow_table_gc_run(flow_table);
flush_workqueue(nf_flow_offload_del_wq);
nf_flow_table_gc_run(flowtable);


Where as my (and Volodymyr's) patch does:

cancel_delayed_work_sync(&flow_table->gc_work);
nf_flow_table_offload_flush(flow_table);
nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, NULL);
nf_flow_table_offload_flush(flow_table);
nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, NULL);


so almost identical, I don't see "extra reiterative calls to flush" 
here,  but I'm fine with just your patch as it's more efficient, can we 
take yours to both gits?

Thanks,
Paul.

Re: [PATCH net 1/1] netfilter: flowtable: Fix use after free after freeing flow table

[Pablo Neira Ayuso]

Hi Paul,

On Sun, Aug 21, 2022 at 12:23:39PM +0300, Paul Blakey wrote:
> Hi!
> 
> The only functional difference here (for HW table) is your patches call
> flush just for the del workqueue instead of del/stats/add, right?
> 
> Because in the end you do:
> cancel_delayed_work_sync(&flow_table->gc_work);
> nf_flow_table_offload_flush(flow_table);
> nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
> nf_flow_table_gc_run(flow_table);
> nf_flow_table_offload_flush_cleanup(flow_table);
> 
> 
> resulting in the following sequence (after expending flush_cleanup()):
> 
> cancel_delayed_work_sync(&flow_table->gc_work);
> nf_flow_table_offload_flush(flow_table);
> nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
> nf_flow_table_gc_run(flow_table);
> flush_workqueue(nf_flow_offload_del_wq);
> nf_flow_table_gc_run(flowtable);
> 
> 
> Where as my (and Volodymyr's) patch does:
> 
> cancel_delayed_work_sync(&flow_table->gc_work);
> nf_flow_table_offload_flush(flow_table);
> nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
> nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, NULL);
> nf_flow_table_offload_flush(flow_table);
> nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, NULL);
> 
> 
> so almost identical, I don't see "extra reiterative calls to flush" here,
> but I'm fine with just your patch as it's more efficient, can we take yours
> to both gits?

Yes, I'll submit them. I'll re-use your patch description.

Maybe I get a Tested-by: tag from you?

Thanks!

Re: [PATCH net 1/1] netfilter: flowtable: Fix use after free after freeing flow table

[Paul Blakey]

On 23/08/2022 00:10, Pablo Neira Ayuso wrote:
> Hi Paul,
> 
> On Sun, Aug 21, 2022 at 12:23:39PM +0300, Paul Blakey wrote:
>> Hi!
>>
>> The only functional difference here (for HW table) is your patches call
>> flush just for the del workqueue instead of del/stats/add, right?
>>
>> Because in the end you do:
>> cancel_delayed_work_sync(&flow_table->gc_work);
>> nf_flow_table_offload_flush(flow_table);
>> nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
>> nf_flow_table_gc_run(flow_table);
>> nf_flow_table_offload_flush_cleanup(flow_table);
>>
>>
>> resulting in the following sequence (after expending flush_cleanup()):
>>
>> cancel_delayed_work_sync(&flow_table->gc_work);
>> nf_flow_table_offload_flush(flow_table);
>> nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
>> nf_flow_table_gc_run(flow_table);
>> flush_workqueue(nf_flow_offload_del_wq);
>> nf_flow_table_gc_run(flowtable);
>>
>>
>> Where as my (and Volodymyr's) patch does:
>>
>> cancel_delayed_work_sync(&flow_table->gc_work);
>> nf_flow_table_offload_flush(flow_table);
>> nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
>> nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, NULL);
>> nf_flow_table_offload_flush(flow_table);
>> nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, NULL);
>>
>>
>> so almost identical, I don't see "extra reiterative calls to flush" here,
>> but I'm fine with just your patch as it's more efficient, can we take yours
>> to both gits?
> 
> Yes, I'll submit them. I'll re-use your patch description.
> 
> Maybe I get a Tested-by: tag from you?
> 
> Thanks!

Sure I'll test and post.
Thanks.

Re: [PATCH net 1/1] netfilter: flowtable: Fix use after free after freeing flow table

[Paul Blakey]

On 23/08/2022 10:56, Paul Blakey wrote:
> 
> 
> On 23/08/2022 00:10, Pablo Neira Ayuso wrote:
>> Hi Paul,
>>
>> On Sun, Aug 21, 2022 at 12:23:39PM +0300, Paul Blakey wrote:
>>> Hi!
>>>
>>> The only functional difference here (for HW table) is your patches call
>>> flush just for the del workqueue instead of del/stats/add, right?
>>>
>>> Because in the end you do:
>>> cancel_delayed_work_sync(&flow_table->gc_work);
>>> nf_flow_table_offload_flush(flow_table);
>>> nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
>>> nf_flow_table_gc_run(flow_table);
>>> nf_flow_table_offload_flush_cleanup(flow_table);
>>>
>>>
>>> resulting in the following sequence (after expending flush_cleanup()):
>>>
>>> cancel_delayed_work_sync(&flow_table->gc_work);
>>> nf_flow_table_offload_flush(flow_table);
>>> nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
>>> nf_flow_table_gc_run(flow_table);
>>> flush_workqueue(nf_flow_offload_del_wq);
>>> nf_flow_table_gc_run(flowtable);
>>>
>>>
>>> Where as my (and Volodymyr's) patch does:
>>>
>>> cancel_delayed_work_sync(&flow_table->gc_work);
>>> nf_flow_table_offload_flush(flow_table);
>>> nf_flow_table_iterate(flow_table, nf_flow_table_do_cleanup, NULL);
>>> nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, NULL);
>>> nf_flow_table_offload_flush(flow_table);
>>> nf_flow_table_iterate(flow_table, nf_flow_offload_gc_step, NULL);
>>>
>>>
>>> so almost identical, I don't see "extra reiterative calls to flush" 
>>> here,
>>> but I'm fine with just your patch as it's more efficient, can we take 
>>> yours
>>> to both gits?
>>
>> Yes, I'll submit them. I'll re-use your patch description.
>>
>> Maybe I get a Tested-by: tag from you?
>>
>> Thanks!
> 
> Sure I'll test and post.
> Thanks.

Tested-By: Paul Blakey <paulb@nvidia.com>

Works, thanks.