netdev - Re: [Patch net 1/2] net_sched: hfsc: Fix a UAF vulnerability in class handling

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: <CALYGNiMtTKpn-BBPpnY+r9WxTPzgMB3rUS5ePO4HQNH1QgZEFQ@mail.gmail.com>
Date: Mon, 14 Apr 2025 12:09:54 +0200
From: Konstantin Khlebnikov <koct9i@...il.com>
To: Cong Wang <xiyou.wangcong@...il.com>
Cc: netdev@...r.kernel.org, jhs@...atatu.com, jiri@...nulli.us, 
	gerrard.tai@...rlabs.sg
Subject: Re: [Patch net 1/2] net_sched: hfsc: Fix a UAF vulnerability in class handling

On Mon, 14 Apr 2025 at 11:39, Konstantin Khlebnikov <koct9i@...il.com> wrote:
>
> On Mon, 14 Apr 2025 at 03:09, Cong Wang <xiyou.wangcong@...il.com> wrote:
> >
> > This patch fixes a Use-After-Free vulnerability in the HFSC qdisc class
> > handling. The issue occurs due to a time-of-check/time-of-use condition
> > in hfsc_change_class() when working with certain child qdiscs like netem
> > or codel.
> >
> > The vulnerability works as follows:
> > 1. hfsc_change_class() checks if a class has packets (q.qlen != 0)
> > 2. It then calls qdisc_peek_len(), which for certain qdiscs (e.g.,
> >    codel, netem) might drop packets and empty the queue
> > 3. The code continues assuming the queue is still non-empty, adding
> >    the class to vttree
> > 4. This breaks HFSC scheduler assumptions that only non-empty classes
> >    are in vttree
> > 5. Later, when the class is destroyed, this can lead to a Use-After-Free
> >
> > The fix adds a second queue length check after qdisc_peek_len() to verify
> > the queue wasn't emptied.
> >
> > Fixes: 21f4d5cc25ec ("net_sched/hfsc: fix curve activation in hfsc_change_class()")
> > Reported-by: Gerrard Tai <gerrard.tai@...rlabs.sg>
> > Cc: Konstantin Khlebnikov <koct9i@...il.com>
> > Signed-off-by: Cong Wang <xiyou.wangcong@...il.com>
> > ---
> >  net/sched/sch_hfsc.c | 9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
> > index ce5045eea065..b368ac0595d5 100644
> > --- a/net/sched/sch_hfsc.c
> > +++ b/net/sched/sch_hfsc.c
> > @@ -961,6 +961,7 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
> >
> >         if (cl != NULL) {
> >                 int old_flags;
> > +               int len = 0;
> >
> >                 if (parentid) {
> >                         if (cl->cl_parent &&
> > @@ -991,9 +992,13 @@ hfsc_change_class(struct Qdisc *sch, u32 classid, u32 parentid,
> >                 if (usc != NULL)
> >                         hfsc_change_usc(cl, usc, cur_time);
> >
> > +               if (cl->qdisc->q.qlen != 0)
> > +                       len = qdisc_peek_len(cl->qdisc);
> > +               /* Check queue length again since some qdisc implementations
> > +                * (e.g., netem/codel) might empty the queue during the peek
> > +                * operation.
> > +                */
> >                 if (cl->qdisc->q.qlen != 0) {
> > -                       int len = qdisc_peek_len(cl->qdisc);
> > -
>
> I don't see any functional changes in the code.

Oh, I see. "peek" indeed can drop some packets.
But it is supposed to return skb, which should still be still part of the queue.
I guess you are also seeing the warning "%s: %s qdisc %X: is
non-work-conserving?".

Actually, following code cares about size of next packet so it could
be clearer to rewrite it as:

if (cl->qdisc->q.qlen != 0) {
    int len = qdisc_peek_len(cl->qdisc);
    if (len != 0) {
        <insert class>
    }
}

>
> >                         if (cl->cl_flags & HFSC_RSC) {
> >                                 if (old_flags & HFSC_RSC)
> >                                         update_ed(cl, len);
> > --
> > 2.34.1
> >