Re: Use-after-free in hfsc_enqueue()

Re: Use-after-free in hfsc_enqueue()

[Jamal Hadi Salim]

On Fri, Jun 27, 2025 at 12:23 AM Budimir Markovic
<markovicbudimir@gmail.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.

Re: Use-after-free in hfsc_enqueue()

[Cong Wang]

Hi Budimir,

On Thu, Jun 26, 2025 at 9:23 PM Budimir Markovic
<markovicbudimir@gmail.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
>
> 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.

I think this is probably the same as the one reported by Mingi a few
weeks ago. Your above patch looks reasonable from my glance.

Do you mind submitting a formal patch? Ideally, send it together
with a selftest (in the same patchset).

Thanks!

Re: Use-after-free in hfsc_enqueue()

[Cong Wang]

Hi Budimir and Mingi,

I just re-tested this. This bug has been gone after Lion's fix:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=103406b38c600fec1fe375a77b27d87e314aea09

Thanks!