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
| ||
|
Message-ID: <CAM0EoMkkQc+ZwEyY19oZMo_zuhTR-w2gCYdjhrvReNzcuQs9CQ@mail.gmail.com> Date: Sat, 5 Jul 2025 10:03:00 -0400 From: Jamal Hadi Salim <jhs@...atatu.com> To: Cong Wang <xiyou.wangcong@...il.com> Cc: Victor Nogueira <victor@...atatu.com>, davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.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 4, 2025 at 7:54 PM Cong Wang <xiyou.wangcong@...il.com> wrote: > > 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; > Sorry, meant to respond to this in last email: there's no need for this extra piece because with the patch posted by Victor this code will not be hit anymore. cheers, jamal
Powered by blists - more mailing lists