[PATCH net v4 1/3] net: sched: fix use-after-free in taprio_change()

[PATCH net v4 1/3] net: sched: fix use-after-free in taprio_change()

[Dmitry Antipov]

In 'taprio_change()', 'admin' pointer may become dangling due to sched
switch / removal caused by 'advance_sched()', and critical section
protected by 'q->current_entry_lock' is too small to prevent from such
a scenario (which causes use-after-free detected by KASAN). Fix this
by prefer 'rcu_replace_pointer()' over 'rcu_assign_pointer()' to update
'admin' immediately before an attempt to schedule freeing.

Fixes: a3d43c0d56f1 ("taprio: Add support adding an admin schedule")
Reported-by: syzbot+b65e0af58423fc8a73aa@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=b65e0af58423fc8a73aa
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v4: adjust subject to target net tree
v3: unchanged since v2
v2: unchanged since v1
---
 net/sched/sch_taprio.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index cc2df9f8c14a..59fad74d5ff9 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1963,7 +1963,8 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 
 		taprio_start_sched(sch, start, new_admin);
 
-		rcu_assign_pointer(q->admin_sched, new_admin);
+		admin = rcu_replace_pointer(q->admin_sched, new_admin,
+					    lockdep_rtnl_is_held());
 		if (admin)
 			call_rcu(&admin->rcu, taprio_free_sched_cb);
 
-- 
2.46.0

[PATCH net v4 2/3] net: sched: consistently use rcu_replace_pointer() in taprio_change()

[Dmitry Antipov]

According to Vinicius (and carefully looking through the whole thing
once again), txtime branch of 'taprio_change()' is not going to race
against 'advance_sched()'. But using 'rcu_replace_pointer()' in the
former may be a good idea as well.

Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v4: adjust subject to target net tree
v3: unchanged since v2
v2: added to the series
---
 net/sched/sch_taprio.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 59fad74d5ff9..9f4e004cdb8b 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1952,7 +1952,9 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 			goto unlock;
 		}
 
-		rcu_assign_pointer(q->admin_sched, new_admin);
+		/* Not going to race against advance_sched(), but still */
+		admin = rcu_replace_pointer(q->admin_sched, new_admin,
+					    lockdep_rtnl_is_held());
 		if (admin)
 			call_rcu(&admin->rcu, taprio_free_sched_cb);
 	} else {
-- 
2.46.0

Re: [PATCH net v4 2/3] net: sched: consistently use rcu_replace_pointer() in taprio_change()

[Simon Horman]

On Tue, Sep 03, 2024 at 05:07:07PM +0300, Dmitry Antipov wrote:
> According to Vinicius (and carefully looking through the whole thing
> once again), txtime branch of 'taprio_change()' is not going to race
> against 'advance_sched()'. But using 'rcu_replace_pointer()' in the
> former may be a good idea as well.

Hi Dmitry,

If this is not a fix, then it should be targeted at net-next.
As the other two patches in the series do appear to be fixes,
that means the patch-set should be split into two: one for net
and one for net-next.

Also, please consider including a cover letter with
patch-sets with more than one patch.

> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
> v4: adjust subject to target net tree
> v3: unchanged since v2
> v2: added to the series

...

[PATCH net v4 3/3] net: sched: use RCU read-side critical section in taprio_dump()

[Dmitry Antipov]

Fix possible use-after-free in 'taprio_dump()' by adding RCU
read-side critical section there. Never seen on x86 but
found on a KASAN-enabled arm64 system when investigating
https://syzkaller.appspot.com/bug?extid=b65e0af58423fc8a73aa:

[T15862] BUG: KASAN: slab-use-after-free in taprio_dump+0xa0c/0xbb0
[T15862] Read of size 4 at addr ffff0000d4bb88f8 by task repro/15862
[T15862]
[T15862] CPU: 0 UID: 0 PID: 15862 Comm: repro Not tainted 6.11.0-rc1-00293-gdefaf1a2113a-dirty #2
[T15862] Hardware name: QEMU QEMU Virtual Machine, BIOS edk2-20240524-5.fc40 05/24/2024
[T15862] Call trace:
[T15862]  dump_backtrace+0x20c/0x220
[T15862]  show_stack+0x2c/0x40
[T15862]  dump_stack_lvl+0xf8/0x174
[T15862]  print_report+0x170/0x4d8
[T15862]  kasan_report+0xb8/0x1d4
[T15862]  __asan_report_load4_noabort+0x20/0x2c
[T15862]  taprio_dump+0xa0c/0xbb0
[T15862]  tc_fill_qdisc+0x540/0x1020
[T15862]  qdisc_notify.isra.0+0x330/0x3a0
[T15862]  tc_modify_qdisc+0x7b8/0x1838
[T15862]  rtnetlink_rcv_msg+0x3c8/0xc20
[T15862]  netlink_rcv_skb+0x1f8/0x3d4
[T15862]  rtnetlink_rcv+0x28/0x40
[T15862]  netlink_unicast+0x51c/0x790
[T15862]  netlink_sendmsg+0x79c/0xc20
[T15862]  __sock_sendmsg+0xe0/0x1a0
[T15862]  ____sys_sendmsg+0x6c0/0x840
[T15862]  ___sys_sendmsg+0x1ac/0x1f0
[T15862]  __sys_sendmsg+0x110/0x1d0
[T15862]  __arm64_sys_sendmsg+0x74/0xb0
[T15862]  invoke_syscall+0x88/0x2e0
[T15862]  el0_svc_common.constprop.0+0xe4/0x2a0
[T15862]  do_el0_svc+0x44/0x60
[T15862]  el0_svc+0x50/0x184
[T15862]  el0t_64_sync_handler+0x120/0x12c
[T15862]  el0t_64_sync+0x190/0x194
[T15862]
[T15862] Allocated by task 15857:
[T15862]  kasan_save_stack+0x3c/0x70
[T15862]  kasan_save_track+0x20/0x3c
[T15862]  kasan_save_alloc_info+0x40/0x60
[T15862]  __kasan_kmalloc+0xd4/0xe0
[T15862]  __kmalloc_cache_noprof+0x194/0x334
[T15862]  taprio_change+0x45c/0x2fe0
[T15862]  tc_modify_qdisc+0x6a8/0x1838
[T15862]  rtnetlink_rcv_msg+0x3c8/0xc20
[T15862]  netlink_rcv_skb+0x1f8/0x3d4
[T15862]  rtnetlink_rcv+0x28/0x40
[T15862]  netlink_unicast+0x51c/0x790
[T15862]  netlink_sendmsg+0x79c/0xc20
[T15862]  __sock_sendmsg+0xe0/0x1a0
[T15862]  ____sys_sendmsg+0x6c0/0x840
[T15862]  ___sys_sendmsg+0x1ac/0x1f0
[T15862]  __sys_sendmsg+0x110/0x1d0
[T15862]  __arm64_sys_sendmsg+0x74/0xb0
[T15862]  invoke_syscall+0x88/0x2e0
[T15862]  el0_svc_common.constprop.0+0xe4/0x2a0
[T15862]  do_el0_svc+0x44/0x60
[T15862]  el0_svc+0x50/0x184
[T15862]  el0t_64_sync_handler+0x120/0x12c
[T15862]  el0t_64_sync+0x190/0x194
[T15862]
[T15862] Freed by task 6192:
[T15862]  kasan_save_stack+0x3c/0x70
[T15862]  kasan_save_track+0x20/0x3c
[T15862]  kasan_save_free_info+0x4c/0x80
[T15862]  poison_slab_object+0x110/0x160
[T15862]  __kasan_slab_free+0x3c/0x74
[T15862]  kfree+0x134/0x3c0
[T15862]  taprio_free_sched_cb+0x18c/0x220
[T15862]  rcu_core+0x920/0x1b7c
[T15862]  rcu_core_si+0x10/0x1c
[T15862]  handle_softirqs+0x2e8/0xd64
[T15862]  __do_softirq+0x14/0x20

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v4: redesign to preserve original code as much as possible,
    adjust commit message and subject to target net tree
v3: tweak commit message as suggested by Vinicius
v2: added to the series
---
 net/sched/sch_taprio.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 9f4e004cdb8b..8623dc0bafc0 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -2374,9 +2374,6 @@ static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb)
 	struct tc_mqprio_qopt opt = { 0 };
 	struct nlattr *nest, *sched_nest;
 
-	oper = rtnl_dereference(q->oper_sched);
-	admin = rtnl_dereference(q->admin_sched);
-
 	mqprio_qopt_reconstruct(dev, &opt);
 
 	nest = nla_nest_start_noflag(skb, TCA_OPTIONS);
@@ -2397,18 +2394,23 @@ static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb)
 	    nla_put_u32(skb, TCA_TAPRIO_ATTR_TXTIME_DELAY, q->txtime_delay))
 		goto options_error;
 
+	rcu_read_lock();
+
+	oper = rtnl_dereference(q->oper_sched);
+	admin = rtnl_dereference(q->admin_sched);
+
 	if (oper && taprio_dump_tc_entries(skb, q, oper))
-		goto options_error;
+		goto options_error_rcu;
 
 	if (oper && dump_schedule(skb, oper))
-		goto options_error;
+		goto options_error_rcu;
 
 	if (!admin)
 		goto done;
 
 	sched_nest = nla_nest_start_noflag(skb, TCA_TAPRIO_ATTR_ADMIN_SCHED);
 	if (!sched_nest)
-		goto options_error;
+		goto options_error_rcu;
 
 	if (dump_schedule(skb, admin))
 		goto admin_error;
@@ -2416,11 +2418,15 @@ static int taprio_dump(struct Qdisc *sch, struct sk_buff *skb)
 	nla_nest_end(skb, sched_nest);
 
 done:
+	rcu_read_unlock();
 	return nla_nest_end(skb, nest);
 
 admin_error:
 	nla_nest_cancel(skb, sched_nest);
 
+options_error_rcu:
+	rcu_read_unlock();
+
 options_error:
 	nla_nest_cancel(skb, nest);
 
-- 
2.46.0

Re: [PATCH net v4 3/3] net: sched: use RCU read-side critical section in taprio_dump()

[Simon Horman]

On Tue, Sep 03, 2024 at 05:07:08PM +0300, Dmitry Antipov wrote:
> Fix possible use-after-free in 'taprio_dump()' by adding RCU
> read-side critical section there. Never seen on x86 but
> found on a KASAN-enabled arm64 system when investigating
> https://syzkaller.appspot.com/bug?extid=b65e0af58423fc8a73aa:
> 
> [T15862] BUG: KASAN: slab-use-after-free in taprio_dump+0xa0c/0xbb0
> [T15862] Read of size 4 at addr ffff0000d4bb88f8 by task repro/15862
> [T15862]
> [T15862] CPU: 0 UID: 0 PID: 15862 Comm: repro Not tainted 6.11.0-rc1-00293-gdefaf1a2113a-dirty #2
> [T15862] Hardware name: QEMU QEMU Virtual Machine, BIOS edk2-20240524-5.fc40 05/24/2024
> [T15862] Call trace:
> [T15862]  dump_backtrace+0x20c/0x220
> [T15862]  show_stack+0x2c/0x40
> [T15862]  dump_stack_lvl+0xf8/0x174
> [T15862]  print_report+0x170/0x4d8
> [T15862]  kasan_report+0xb8/0x1d4
> [T15862]  __asan_report_load4_noabort+0x20/0x2c
> [T15862]  taprio_dump+0xa0c/0xbb0
> [T15862]  tc_fill_qdisc+0x540/0x1020
> [T15862]  qdisc_notify.isra.0+0x330/0x3a0
> [T15862]  tc_modify_qdisc+0x7b8/0x1838
> [T15862]  rtnetlink_rcv_msg+0x3c8/0xc20
> [T15862]  netlink_rcv_skb+0x1f8/0x3d4
> [T15862]  rtnetlink_rcv+0x28/0x40
> [T15862]  netlink_unicast+0x51c/0x790
> [T15862]  netlink_sendmsg+0x79c/0xc20
> [T15862]  __sock_sendmsg+0xe0/0x1a0
> [T15862]  ____sys_sendmsg+0x6c0/0x840
> [T15862]  ___sys_sendmsg+0x1ac/0x1f0
> [T15862]  __sys_sendmsg+0x110/0x1d0
> [T15862]  __arm64_sys_sendmsg+0x74/0xb0
> [T15862]  invoke_syscall+0x88/0x2e0
> [T15862]  el0_svc_common.constprop.0+0xe4/0x2a0
> [T15862]  do_el0_svc+0x44/0x60
> [T15862]  el0_svc+0x50/0x184
> [T15862]  el0t_64_sync_handler+0x120/0x12c
> [T15862]  el0t_64_sync+0x190/0x194
> [T15862]
> [T15862] Allocated by task 15857:
> [T15862]  kasan_save_stack+0x3c/0x70
> [T15862]  kasan_save_track+0x20/0x3c
> [T15862]  kasan_save_alloc_info+0x40/0x60
> [T15862]  __kasan_kmalloc+0xd4/0xe0
> [T15862]  __kmalloc_cache_noprof+0x194/0x334
> [T15862]  taprio_change+0x45c/0x2fe0
> [T15862]  tc_modify_qdisc+0x6a8/0x1838
> [T15862]  rtnetlink_rcv_msg+0x3c8/0xc20
> [T15862]  netlink_rcv_skb+0x1f8/0x3d4
> [T15862]  rtnetlink_rcv+0x28/0x40
> [T15862]  netlink_unicast+0x51c/0x790
> [T15862]  netlink_sendmsg+0x79c/0xc20
> [T15862]  __sock_sendmsg+0xe0/0x1a0
> [T15862]  ____sys_sendmsg+0x6c0/0x840
> [T15862]  ___sys_sendmsg+0x1ac/0x1f0
> [T15862]  __sys_sendmsg+0x110/0x1d0
> [T15862]  __arm64_sys_sendmsg+0x74/0xb0
> [T15862]  invoke_syscall+0x88/0x2e0
> [T15862]  el0_svc_common.constprop.0+0xe4/0x2a0
> [T15862]  do_el0_svc+0x44/0x60
> [T15862]  el0_svc+0x50/0x184
> [T15862]  el0t_64_sync_handler+0x120/0x12c
> [T15862]  el0t_64_sync+0x190/0x194
> [T15862]
> [T15862] Freed by task 6192:
> [T15862]  kasan_save_stack+0x3c/0x70
> [T15862]  kasan_save_track+0x20/0x3c
> [T15862]  kasan_save_free_info+0x4c/0x80
> [T15862]  poison_slab_object+0x110/0x160
> [T15862]  __kasan_slab_free+0x3c/0x74
> [T15862]  kfree+0x134/0x3c0
> [T15862]  taprio_free_sched_cb+0x18c/0x220
> [T15862]  rcu_core+0x920/0x1b7c
> [T15862]  rcu_core_si+0x10/0x1c
> [T15862]  handle_softirqs+0x2e8/0xd64
> [T15862]  __do_softirq+0x14/0x20
> 

Hi Dmitry,

As a fix for net, there should be a Fixes tag here,
indicating the first commit in which this bug manifested.
This can be the first commit in the tree, if the
bug has been there since then.

If the patch-set is not going to be resent, it is probably
sufficient to just reply with one here. And it may be a good
idea to do so regardless.

> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>