netdev - Re: Use-after-free in hfsc

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>] [day] [month] [year] [list]

Message-ID: <CAM0EoMmA1WLUtamjYNFVZ75NYKznL3K2h8HSv=2z4D3=ZDS83Q@mail.gmail.com>
Date: Sat, 28 Jun 2025 17:19:09 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Budimir Markovic <markovicbudimir@...il.com>
Cc: security@...nel.org, xiyou.wangcong@...il.com, 
	Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: Use-after-free in hfsc_enqueue()

On Fri, Jun 27, 2025 at 12:23 AM Budimir Markovic
<markovicbudimir@...il.com> wrote:
>
> The use-after-free referenced in commit 3f981138109f ("sch_hfsc: Fix qlen
> accounting bug when using peek in hfsc_enqueue()") is still possible.  That
> commit ensured that qlen_notify is called on sch's parent class during peek(),
> but that class has not yet been added to its active list at this point. It is
> only added after hfsc_enqueue() returns, and since no packets are enqueued it
> will never be removed. The same applies to any classes further up the
> hierarchy.
>
> These commands trigger the bug causing a use-after-free and often a kernel
> crash:
>
> ip link set dev lo up
> tc qdisc add dev lo root handle 1: drr
> tc filter add dev lo parent 1: basic classid 1:1
> tc class add dev lo parent 1: classid 1:1 drr
> tc qdisc add dev lo parent 1:1 handle 2: hfsc def 1
> tc class add dev lo parent 2: classid 2:1 hfsc rt m1 8 d 1 m2 0
> tc qdisc add dev lo parent 2:1 handle 3: netem
> tc qdisc add dev lo parent 3:1 handle 4: blackhole
> ping -c1 -W0.01 localhost
> tc class del dev lo classid 1:1
> tc filter add dev lo parent 1: basic classid 1:1
> ping -c1 -W0.01 localhost
>

There are several approaches being discussed to do this on the list.
Please come there, see this thread:

https://lore.kernel.org/netdev/aF847kk6H+kr5kIV@pop-os.localdomain/

cheers,
jamal

> It can be fixed with the following change:
>
> diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
> index 5a7745170..8e25fae48 100644
> --- a/net/sched/sch_hfsc.c
> +++ b/net/sched/sch_hfsc.c
> @@ -1589,8 +1589,13 @@ hfsc_enqueue(struct sk_buff *skb, struct Qdisc *sch, struct sk_buff **to_free)
>                  * head drop before the first dequeue operation has no chance
>                  * to invalidate the deadline.
>                  */
> -               if (cl->cl_flags & HFSC_RSC)
> +               if (cl->cl_flags & HFSC_RSC) {
>                         cl->qdisc->ops->peek(cl->qdisc);
> +                       if (!cl->qdisc->q.qlen) {
> +                               qdisc_tree_reduce_backlog(sch, -1, -len);
> +                               return NET_XMIT_DROP;
> +                       }
> +               }
>
>         }
>
> Returning __NET_XMIT_BYPASS prevents sch's parent class from being added to its
> active list when no packets have been enqueued. It also prevents it from
> updating its queue length, so we need to do this manually with
> qdisc_tree_reduce_backlog(). This is necessary because netem_dequeue() assumes
> the packet has already been enqueued to its ancestor qdiscs and decreases their
> queue lengths when dropping the packet.
>
> qdisc_tree_reduce_backlog() will end up calling qlen_notify on non-active
> classes, but after the recent hardening of the qlen_notify methods this should
> be safe.