netdev - Re: [PATCH v1 net] net: sched: sch_qfq: Fix use-after-free in qfq_reset

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAAVpQUDxg_BjyJ5ZPYAwCiWwZ=pgb4x+uymc=x_yPs8_hiSbzw@mail.gmail.com>
Date: Tue, 11 Nov 2025 19:48:56 -0800
From: Kuniyuki Iwashima <kuniyu@...gle.com>
To: Cong Wang <xiyou.wangcong@...il.com>
Cc: Jamal Hadi Salim <jhs@...atatu.com>, Jiri Pirko <jiri@...nulli.us>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, 
	Paolo Valente <paolo.valente@...more.it>, Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org, 
	syzbot+ec7176504e5bcc33ca4e@...kaller.appspotmail.com
Subject: Re: [PATCH v1 net] net: sched: sch_qfq: Fix use-after-free in qfq_reset_qdisc().

On Tue, Nov 11, 2025 at 7:07 PM Kuniyuki Iwashima <kuniyu@...gle.com> wrote:
>
> On Mon, Nov 10, 2025 at 12:22 PM Cong Wang <xiyou.wangcong@...il.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.