[patch net-next] net: sched: fix use-after-free in tcf_block_put_ext

[patch net-next] net: sched: fix use-after-free in tcf_block_put_ext

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Since the block is freed with last chain being put, once we reach the
end of iteration of list_for_each_entry_safe, the block may be
already freed. I'm hitting this only by creating and deleting clsact:

[  202.171952] ==================================================================
[  202.180182] BUG: KASAN: use-after-free in tcf_block_put_ext+0x240/0x390
[  202.187590] Read of size 8 at addr ffff880225539a80 by task tc/796
[  202.194508]
[  202.196185] CPU: 0 PID: 796 Comm: tc Not tainted 4.15.0-rc2jiri+ #5
[  202.203200] Hardware name: Mellanox Technologies Ltd. "MSN2100-CB2F"/"SA001017", BIOS 5.6.5 06/07/2016
[  202.213613] Call Trace:
[  202.216369]  dump_stack+0xda/0x169
[  202.220192]  ? dma_virt_map_sg+0x147/0x147
[  202.224790]  ? show_regs_print_info+0x54/0x54
[  202.229691]  ? tcf_chain_destroy+0x1dc/0x250
[  202.234494]  print_address_description+0x83/0x3d0
[  202.239781]  ? tcf_block_put_ext+0x240/0x390
[  202.244575]  kasan_report+0x1ba/0x460
[  202.248707]  ? tcf_block_put_ext+0x240/0x390
[  202.253518]  tcf_block_put_ext+0x240/0x390
[  202.258117]  ? tcf_chain_flush+0x290/0x290
[  202.262708]  ? qdisc_hash_del+0x82/0x1a0
[  202.267111]  ? qdisc_hash_add+0x50/0x50
[  202.271411]  ? __lock_is_held+0x5f/0x1a0
[  202.275843]  clsact_destroy+0x3d/0x80 [sch_ingress]
[  202.281323]  qdisc_destroy+0xcb/0x240
[  202.285445]  qdisc_graft+0x216/0x7b0
[  202.289497]  tc_get_qdisc+0x260/0x560

Fix this by holding the block also by chain 0 and put chain 0
explicitly, out of the list_for_each_entry_safe loop at the very
end of tcf_block_put_ext.

Fixes: efbf78973978 ("net_sched: get rid of rcu_barrier() in tcf_block_put_ext()")
Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
 net/sched/cls_api.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index d51051d..5b9b8a6 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -342,23 +342,24 @@ void tcf_block_put_ext(struct tcf_block *block, struct Qdisc *q,
 {
 	struct tcf_chain *chain, *tmp;
 
-	/* Hold a refcnt for all chains, except 0, so that they don't disappear
+	/* Hold a refcnt for all chains, so that they don't disappear
 	 * while we are iterating.
 	 */
 	list_for_each_entry(chain, &block->chain_list, list)
-		if (chain->index)
-			tcf_chain_hold(chain);
+		tcf_chain_hold(chain);
 
 	list_for_each_entry(chain, &block->chain_list, list)
 		tcf_chain_flush(chain);
 
 	tcf_block_offload_unbind(block, q, ei);
 
-	/* At this point, all the chains should have refcnt >= 1. Block will be
-	 * freed after all chains are gone.
-	 */
+	/* At this point, all the chains should have refcnt >= 1. */
 	list_for_each_entry_safe(chain, tmp, &block->chain_list, list)
 		tcf_chain_put(chain);
+
+	/* Finally, put chain 0 and allow block to be freed. */
+	chain = list_first_entry(&block->chain_list, struct tcf_chain, list);
+	tcf_chain_put(chain);
 }
 EXPORT_SYMBOL(tcf_block_put_ext);
 
-- 
2.9.5

Re: [patch net-next] net: sched: fix use-after-free in tcf_block_put_ext

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Fri,  8 Dec 2017 19:27:27 +0100

> From: Jiri Pirko <jiri@mellanox.com>
> 
> Since the block is freed with last chain being put, once we reach the
> end of iteration of list_for_each_entry_safe, the block may be
> already freed. I'm hitting this only by creating and deleting clsact:
 ...
> Fix this by holding the block also by chain 0 and put chain 0
> explicitly, out of the list_for_each_entry_safe loop at the very
> end of tcf_block_put_ext.
> 
> Fixes: efbf78973978 ("net_sched: get rid of rcu_barrier() in tcf_block_put_ext()")
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Applied, thanks Jiri.

Re: [patch net-next] net: sched: fix use-after-free in tcf_block_put_ext

[Cong Wang]

On Fri, Dec 8, 2017 at 10:27 AM, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> Since the block is freed with last chain being put, once we reach the
> end of iteration of list_for_each_entry_safe, the block may be
> already freed. I'm hitting this only by creating and deleting clsact:

List is still too tricky to work with refcnt.

>
> [  202.171952] ==================================================================
> [  202.180182] BUG: KASAN: use-after-free in tcf_block_put_ext+0x240/0x390


I should enable KASAN now, otherwise can't catch this
even though I tested this code path for hundreds of times.