netdev - Re: [PATCH net] net/sched: Abort __tc_modify_qdisc if parent class does not exist

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aGhpq58BI8mFpCEs@pop-os.localdomain>
Date: Fri, 4 Jul 2025 16:54:19 -0700
From: Cong Wang <xiyou.wangcong@...il.com>
To: Victor Nogueira <victor@...atatu.com>
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
	edumazet@...gle.com, jhs@...atatu.com, jiri@...nulli.us,
	netdev@...r.kernel.org, pctammela@...atatu.com,
	syzbot+d8b58d7b0ad89a678a16@...kaller.appspotmail.com,
	syzbot+5eccb463fa89309d8bdc@...kaller.appspotmail.com,
	syzbot+1261670bbdefc5485a06@...kaller.appspotmail.com,
	syzbot+4dadc5aecf80324d5a51@...kaller.appspotmail.com,
	syzbot+15b96fc3aac35468fe77@...kaller.appspotmail.com
Subject: Re: [PATCH net] net/sched: Abort __tc_modify_qdisc if parent class
 does not exist

On Fri, Jul 04, 2025 at 01:34:22PM -0300, Victor Nogueira wrote:
> Lion's patch [1] revealed an ancient bug in the qdisc API.
> Whenever a user creates/modifies a qdisc specifying as a parent another
> qdisc, the qdisc API will, during grafting, detect that the user is
> not trying to attach to a class and reject. However grafting is
> performed after qdisc_create (and thus the qdiscs' init callback) is
> executed. In qdiscs that eventually call qdisc_tree_reduce_backlog
> during init or change (such as fq, hhf, choke, etc), an issue
> arises. For example, executing the following commands:
> 
> sudo tc qdisc add dev lo root handle a: htb default 2
> sudo tc qdisc add dev lo parent a: handle beef fq
> 
> Qdiscs such as fq, hhf, choke, etc unconditionally invoke
> qdisc_tree_reduce_backlog() in their control path init() or change() which
> then causes a failure to find the child class; however, that does not stop
> the unconditional invocation of the assumed child qdisc's qlen_notify with
> a null class. All these qdiscs make the assumption that class is non-null.
> 
> The solution is ensure that qdisc_leaf() which looks up the parent
> class, and is invoked prior to qdisc_create(), should return failure on
> not finding the class.
> In this patch, we leverage qdisc_leaf to return ERR_PTRs whenever the
> parentid doesn't correspond to a class, so that we can detect it
> earlier on and abort before qdisc_create is called.
> 

Thanks for your quick and excellent catch!

Reviewed-by: Cong Wang <xiyou.wangcong@...il.com>

Just one additional question, do we still need to safe-guard
qdisc_tree_reduce_backlog()? Below is actually what pops on my mind
immediately after reading your above description (before reading your
code):


diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index c5e3673aadbe..5a32af623aa4 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -817,6 +817,8 @@ void qdisc_tree_reduce_backlog(struct Qdisc *sch, int n, int len)
                cops = sch->ops->cl_ops;
                if (notify && cops->qlen_notify) {
                        cl = cops->find(sch, parentid);
+                       if (!cl)
+                               break;
                        cops->qlen_notify(sch, cl);
                }
                sch->q.qlen -= n;