[PATCH v1 net] net: sched: sch_qfq: Fix use-after-free in qfq_reset_qdisc().

[PATCH v1 net] net: sched: sch_qfq: Fix use-after-free in qfq_reset_qdisc().

[Kuniyuki Iwashima]

syzbot reported use-after-free in qfq_reset_qdisc() while
accessing struct qfq_class hashed in qfq_sched.clhash.hash[]. [0]

When qfq_find_agg() returns NULL in qfq_change_class(),
struct qfq_aggregate is allocated.

If it fails, qfq_class is kfree()d at the destroy_class: label.

However, if it happens when qfq_change_class() is called for an
existing class, just freeing it is not sufficient, leaking the old
qfq_aggregate and leaving qfq_class in qfq_sched.clhash.hash[].

Let's call qfq_delete_class() in such a case.

We need to move up qfq_destroy_class() and qfq_delete_class().

[0]:
BUG: KASAN: slab-use-after-free in qfq_reset_qdisc+0xcc/0x208 net/sched/sch_qfq.c:1484
Read of size 8 at addr ffff0000ca2bfe50 by task syz.0.17/6716
CPU: 0 UID: 0 PID: 6716 Comm: syz.0.17 Not tainted syzkaller #0 PREEMPT
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 06/30/2025
Call trace:
 show_stack+0x2c/0x3c arch/arm64/kernel/stacktrace.c:499 (C)
 __dump_stack+0x30/0x40 lib/dump_stack.c:94
 dump_stack_lvl+0xd8/0x12c lib/dump_stack.c:120
 print_address_description+0xa8/0x238 mm/kasan/report.c:378
 print_report+0x68/0x84 mm/kasan/report.c:482
 kasan_report+0xb0/0x110 mm/kasan/report.c:595
 __asan_report_load8_noabort+0x20/0x2c mm/kasan/report_generic.c:381
 qfq_reset_qdisc+0xcc/0x208 net/sched/sch_qfq.c:1484
 qdisc_reset+0x128/0x598 net/sched/sch_generic.c:1038
 __qdisc_destroy+0x134/0x4bc net/sched/sch_generic.c:1077
 qdisc_put net/sched/sch_generic.c:1109 [inline]
 dev_shutdown+0x35c/0x47c net/sched/sch_generic.c:1497
 unregister_netdevice_many_notify+0xbb8/0x1de0 net/core/dev.c:12242
 unregister_netdevice_many net/core/dev.c:12317 [inline]
 unregister_netdevice_queue+0x2b4/0x300 net/core/dev.c:12161
 unregister_netdevice include/linux/netdevice.h:3389 [inline]
 __tun_detach+0x5d4/0x1304 drivers/net/tun.c:621
 tun_detach drivers/net/tun.c:637 [inline]
 tun_chr_close+0x118/0x1f8 drivers/net/tun.c:3436
 __fput+0x340/0x75c fs/file_table.c:468
 ____fput+0x20/0x58 fs/file_table.c:496
 task_work_run+0x1dc/0x260 kernel/task_work.c:227
 resume_user_mode_work include/linux/resume_user_mode.h:50 [inline]
 exit_to_user_mode_loop+0xfc/0x178 kernel/entry/common.c:43
 exit_to_user_mode_prepare include/linux/irq-entry-common.h:225 [inline]
 arm64_exit_to_user_mode arch/arm64/kernel/entry-common.c:103 [inline]
 el0_svc+0x170/0x254 arch/arm64/kernel/entry-common.c:747
 el0t_64_sync_handler+0x84/0x12c arch/arm64/kernel/entry-common.c:765
 el0t_64_sync+0x198/0x19c arch/arm64/kernel/entry.S:596

Allocated by task 6716:
 kasan_save_stack mm/kasan/common.c:56 [inline]
 kasan_save_track+0x40/0x78 mm/kasan/common.c:77
 kasan_save_alloc_info+0x44/0x54 mm/kasan/generic.c:573
 poison_kmalloc_redzone mm/kasan/common.c:400 [inline]
 __kasan_kmalloc+0x9c/0xb4 mm/kasan/common.c:417
 kasan_kmalloc include/linux/kasan.h:262 [inline]
 __kmalloc_cache_noprof+0x3a4/0x65c mm/slub.c:5748
 kmalloc_noprof include/linux/slab.h:957 [inline]
 kzalloc_noprof include/linux/slab.h:1094 [inline]
 qfq_change_class+0x498/0xbe8 net/sched/sch_qfq.c:479
 __tc_ctl_tclass net/sched/sch_api.c:2274 [inline]
 tc_ctl_tclass+0x988/0x10b0 net/sched/sch_api.c:2304
 rtnetlink_rcv_msg+0x624/0x97c net/core/rtnetlink.c:6963
 netlink_rcv_skb+0x220/0x3fc net/netlink/af_netlink.c:2552
 rtnetlink_rcv+0x28/0x38 net/core/rtnetlink.c:6981
 netlink_unicast_kernel net/netlink/af_netlink.c:1320 [inline]
 netlink_unicast+0x694/0x8c4 net/netlink/af_netlink.c:1346
 netlink_sendmsg+0x648/0x930 net/netlink/af_netlink.c:1896
 sock_sendmsg_nosec net/socket.c:727 [inline]
 __sock_sendmsg net/socket.c:742 [inline]
 ____sys_sendmsg+0x490/0x7b8 net/socket.c:2630
 ___sys_sendmsg+0x204/0x278 net/socket.c:2684
 __sys_sendmsg net/socket.c:2716 [inline]
 __do_sys_sendmsg net/socket.c:2721 [inline]
 __se_sys_sendmsg net/socket.c:2719 [inline]
 __arm64_sys_sendmsg+0x184/0x238 net/socket.c:2719
 __invoke_syscall arch/arm64/kernel/syscall.c:35 [inline]
 invoke_syscall+0x98/0x254 arch/arm64/kernel/syscall.c:49
 el0_svc_common+0x130/0x23c arch/arm64/kernel/syscall.c:132
 do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151
 el0_svc+0x5c/0x254 arch/arm64/kernel/entry-common.c:746
 el0t_64_sync_handler+0x84/0x12c arch/arm64/kernel/entry-common.c:765
 el0t_64_sync+0x198/0x19c arch/arm64/kernel/entry.S:596

Freed by task 6716:
 kasan_save_stack mm/kasan/common.c:56 [inline]
 kasan_save_track+0x40/0x78 mm/kasan/common.c:77
 __kasan_save_free_info+0x58/0x70 mm/kasan/generic.c:587
 kasan_save_free_info mm/kasan/kasan.h:406 [inline]
 poison_slab_object mm/kasan/common.c:252 [inline]
 __kasan_slab_free+0x74/0xa4 mm/kasan/common.c:284
 kasan_slab_free include/linux/kasan.h:234 [inline]
 slab_free_hook mm/slub.c:2523 [inline]
 slab_free mm/slub.c:6611 [inline]
 kfree+0x184/0x600 mm/slub.c:6818
 qfq_change_class+0x92c/0xbe8 net/sched/sch_qfq.c:533
 __tc_ctl_tclass net/sched/sch_api.c:2274 [inline]
 tc_ctl_tclass+0x988/0x10b0 net/sched/sch_api.c:2304
 rtnetlink_rcv_msg+0x624/0x97c net/core/rtnetlink.c:6963
 netlink_rcv_skb+0x220/0x3fc net/netlink/af_netlink.c:2552
 rtnetlink_rcv+0x28/0x38 net/core/rtnetlink.c:6981
 netlink_unicast_kernel net/netlink/af_netlink.c:1320 [inline]
 netlink_unicast+0x694/0x8c4 net/netlink/af_netlink.c:1346
 netlink_sendmsg+0x648/0x930 net/netlink/af_netlink.c:1896
 sock_sendmsg_nosec net/socket.c:727 [inline]
 __sock_sendmsg net/socket.c:742 [inline]
 ____sys_sendmsg+0x490/0x7b8 net/socket.c:2630
 ___sys_sendmsg+0x204/0x278 net/socket.c:2684
 __sys_sendmsg net/socket.c:2716 [inline]
 __do_sys_sendmsg net/socket.c:2721 [inline]
 __se_sys_sendmsg net/socket.c:2719 [inline]
 __arm64_sys_sendmsg+0x184/0x238 net/socket.c:2719
 __invoke_syscall arch/arm64/kernel/syscall.c:35 [inline]
 invoke_syscall+0x98/0x254 arch/arm64/kernel/syscall.c:49
 el0_svc_common+0x130/0x23c arch/arm64/kernel/syscall.c:132
 do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151
 el0_svc+0x5c/0x254 arch/arm64/kernel/entry-common.c:746
 el0t_64_sync_handler+0x84/0x12c arch/arm64/kernel/entry-common.c:765
 el0t_64_sync+0x198/0x19c arch/arm64/kernel/entry.S:596

Fixes: 462dbc9101ac ("pkt_sched: QFQ Plus: fair-queueing service at DRR cost")
Reported-by: syzbot+ec7176504e5bcc33ca4e@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/690c48f2.050a0220.baf87.0080.GAE@google.com/
Signed-off-by: Kuniyuki Iwashima <kuniyu@google.com>
---
 net/sched/sch_qfq.c | 68 +++++++++++++++++++++++++--------------------
 1 file changed, 38 insertions(+), 30 deletions(-)

diff --git a/net/sched/sch_qfq.c b/net/sched/sch_qfq.c
index 2255355e51d3..3d1522b1b88a 100644
--- a/net/sched/sch_qfq.c
+++ b/net/sched/sch_qfq.c
@@ -403,6 +403,36 @@ static int qfq_change_agg(struct Qdisc *sch, struct qfq_class *cl, u32 weight,
 	return 0;
 }
 
+static void qfq_destroy_class(struct Qdisc *sch, struct qfq_class *cl)
+{
+	gen_kill_estimator(&cl->rate_est);
+	qdisc_put(cl->qdisc);
+	kfree(cl);
+}
+
+static int qfq_delete_class(struct Qdisc *sch, unsigned long arg,
+			    struct netlink_ext_ack *extack)
+{
+	struct qfq_sched *q = qdisc_priv(sch);
+	struct qfq_class *cl = (struct qfq_class *)arg;
+
+	if (qdisc_class_in_use(&cl->common)) {
+		NL_SET_ERR_MSG_MOD(extack, "QFQ class in use");
+		return -EBUSY;
+	}
+
+	sch_tree_lock(sch);
+
+	qdisc_purge_queue(cl->qdisc);
+	qdisc_class_hash_remove(&q->clhash, &cl->common);
+	qfq_rm_from_agg(q, cl);
+
+	sch_tree_unlock(sch);
+
+	qfq_destroy_class(sch, cl);
+	return 0;
+}
+
 static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 			    struct nlattr **tca, unsigned long *arg,
 			    struct netlink_ext_ack *extack)
@@ -511,6 +541,10 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 		new_agg = kzalloc(sizeof(*new_agg), GFP_KERNEL);
 		if (new_agg == NULL) {
 			err = -ENOBUFS;
+
+			if (existing)
+				goto delete_class;
+
 			gen_kill_estimator(&cl->rate_est);
 			goto destroy_class;
 		}
@@ -528,40 +562,14 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
 	*arg = (unsigned long)cl;
 	return 0;
 
-destroy_class:
-	qdisc_put(cl->qdisc);
-	kfree(cl);
+delete_class:
+	qfq_delete_class(sch, (unsigned long)cl, extack);
 	return err;
-}
 
-static void qfq_destroy_class(struct Qdisc *sch, struct qfq_class *cl)
-{
-	gen_kill_estimator(&cl->rate_est);
+destroy_class:
 	qdisc_put(cl->qdisc);
 	kfree(cl);
-}
-
-static int qfq_delete_class(struct Qdisc *sch, unsigned long arg,
-			    struct netlink_ext_ack *extack)
-{
-	struct qfq_sched *q = qdisc_priv(sch);
-	struct qfq_class *cl = (struct qfq_class *)arg;
-
-	if (qdisc_class_in_use(&cl->common)) {
-		NL_SET_ERR_MSG_MOD(extack, "QFQ class in use");
-		return -EBUSY;
-	}
-
-	sch_tree_lock(sch);
-
-	qdisc_purge_queue(cl->qdisc);
-	qdisc_class_hash_remove(&q->clhash, &cl->common);
-	qfq_rm_from_agg(q, cl);
-
-	sch_tree_unlock(sch);
-
-	qfq_destroy_class(sch, cl);
-	return 0;
+	return err;
 }
 
 static unsigned long qfq_search_class(struct Qdisc *sch, u32 classid)
-- 
2.51.2.1026.g39e6a42477-goog

Re: [PATCH v1 net] net: sched: sch_qfq: Fix use-after-free in qfq_reset_qdisc().

[Cong Wang]

On Thu, Nov 06, 2025 at 07:10:49AM +0000, Kuniyuki Iwashima wrote:
>  static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
>  			    struct nlattr **tca, unsigned long *arg,
>  			    struct netlink_ext_ack *extack)
> @@ -511,6 +541,10 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
>  		new_agg = kzalloc(sizeof(*new_agg), GFP_KERNEL);
>  		if (new_agg == NULL) {
>  			err = -ENOBUFS;
> +
> +			if (existing)
> +				goto delete_class;
> +
>  			gen_kill_estimator(&cl->rate_est);
>  			goto destroy_class;
>  		}
> @@ -528,40 +562,14 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
>  	*arg = (unsigned long)cl;
>  	return 0;
>  
> -destroy_class:
> -	qdisc_put(cl->qdisc);
> -	kfree(cl);
> +delete_class:
> +	qfq_delete_class(sch, (unsigned long)cl, extack);
>  	return err;

Is it better to just call qfq_delete_class() directly? Two reasons:
1) It is only used by this code path
2) It reads odd to place a 'return' above 'destroy_class' label below.

And, what about the error patch of gen_new_estimator()? 'existing' could
be true for that case too, which I assume requires the same error
handling?

Thanks!

Re: [PATCH v1 net] net: sched: sch_qfq: Fix use-after-free in qfq_reset_qdisc().

[Kuniyuki Iwashima]

On Mon, Nov 10, 2025 at 12:22 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Thu, Nov 06, 2025 at 07:10:49AM +0000, Kuniyuki Iwashima wrote:
> >  static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
> >                           struct nlattr **tca, unsigned long *arg,
> >                           struct netlink_ext_ack *extack)
> > @@ -511,6 +541,10 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
> >               new_agg = kzalloc(sizeof(*new_agg), GFP_KERNEL);
> >               if (new_agg == NULL) {
> >                       err = -ENOBUFS;
> > +
> > +                     if (existing)
> > +                             goto delete_class;
> > +
> >                       gen_kill_estimator(&cl->rate_est);
> >                       goto destroy_class;
> >               }
> > @@ -528,40 +562,14 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
> >       *arg = (unsigned long)cl;
> >       return 0;
> >
> > -destroy_class:
> > -     qdisc_put(cl->qdisc);
> > -     kfree(cl);
> > +delete_class:
> > +     qfq_delete_class(sch, (unsigned long)cl, extack);
> >       return err;
>
> Is it better to just call qfq_delete_class() directly? Two reasons:
> 1) It is only used by this code path
> 2) It reads odd to place a 'return' above 'destroy_class' label below.
>
> And, what about the error patch of gen_new_estimator()? 'existing' could
> be true for that case too, which I assume requires the same error
> handling?

Ah right, the path also needs qfq_delete_class().
Then, the label will be needed anyway.

Thanks!

Re: [PATCH v1 net] net: sched: sch_qfq: Fix use-after-free in qfq_reset_qdisc().

[Kuniyuki Iwashima]

On Tue, Nov 11, 2025 at 7:07 PM Kuniyuki Iwashima <kuniyu@google.com> wrote:
>
> On Mon, Nov 10, 2025 at 12:22 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Thu, Nov 06, 2025 at 07:10:49AM +0000, Kuniyuki Iwashima wrote:
> > >  static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
> > >                           struct nlattr **tca, unsigned long *arg,
> > >                           struct netlink_ext_ack *extack)
> > > @@ -511,6 +541,10 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
> > >               new_agg = kzalloc(sizeof(*new_agg), GFP_KERNEL);
> > >               if (new_agg == NULL) {
> > >                       err = -ENOBUFS;
> > > +
> > > +                     if (existing)
> > > +                             goto delete_class;
> > > +
> > >                       gen_kill_estimator(&cl->rate_est);
> > >                       goto destroy_class;
> > >               }
> > > @@ -528,40 +562,14 @@ static int qfq_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
> > >       *arg = (unsigned long)cl;
> > >       return 0;
> > >
> > > -destroy_class:
> > > -     qdisc_put(cl->qdisc);
> > > -     kfree(cl);
> > > +delete_class:
> > > +     qfq_delete_class(sch, (unsigned long)cl, extack);
> > >       return err;
> >
> > Is it better to just call qfq_delete_class() directly? Two reasons:
> > 1) It is only used by this code path
> > 2) It reads odd to place a 'return' above 'destroy_class' label below.
> >
> > And, what about the error patch of gen_new_estimator()? 'existing' could
> > be true for that case too, which I assume requires the same error
> > handling?
>
> Ah right, the path also needs qfq_delete_class().

Oh no.  Just to be sure, I assume you meant
gen_replace_estimator() (and gen_new_estimator() in it), right ?
Otherwise 'existing' can't be true for the other gen_new_estimator().

In case gen_replace_estimator () fails, we just return an error
without destroying the existing one because we can still use it
without reverting gen_replace_estimator().

So I think we don't need extra error handling for that case.