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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAAVpQUDi8hLHa6nps9PyNbxqLUEm4znO8xF8M=rER5UAEN7ghw@mail.gmail.com>
Date: Tue, 11 Nov 2025 19:07:02 -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 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().
Then, the label will be needed anyway.

Thanks!