[Patch net 0/2] net_sched: Fix a UAF vulnerability in HFSC class handling

[Patch net 0/2] net_sched: Fix a UAF vulnerability in HFSC class handling

[Cong Wang]

This patchset contains a bug fix and a selftest for it, please check
each patch description for details.

---
Cong Wang (2):
  net_sched: hfsc: Fix a UAF vulnerability in class handling
  selftests/tc-testing: Add test for HFSC queue emptying during peek
    operation

 net/sched/sch_hfsc.c                          |  9 ++++-
 .../tc-testing/tc-tests/infra/qdiscs.json     | 39 +++++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

-- 
2.34.1

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

[Cong Wang]

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@starlabs.sg>
Cc: Konstantin Khlebnikov <koct9i@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.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);
-
 			if (cl->cl_flags & HFSC_RSC) {
 				if (old_flags & HFSC_RSC)
 					update_ed(cl, len);
-- 
2.34.1

[Patch net 2/2] selftests/tc-testing: Add test for HFSC queue emptying during peek operation

[Cong Wang]

Add a selftest to exercise the condition where qdisc implementations
like netem or codel might empty the queue during a peek operation.
This tests the defensive code path in HFSC that checks the queue length
again after peeking to handle this case.

Based on the reproducer from Gerrard, improved by Jamal.

Reported-by: Gerrard Tai <gerrard.tai@starlabs.sg>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 .../tc-testing/tc-tests/infra/qdiscs.json     | 39 +++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
index d4ea9cd845a3..e26bbc169783 100644
--- a/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
+++ b/tools/testing/selftests/tc-testing/tc-tests/infra/qdiscs.json
@@ -313,5 +313,44 @@
             "$TC qdisc del dev $DUMMY handle 1: root",
             "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
         ]
+    },
+    {
+        "id": "a4c3",
+        "name": "Test HFSC with netem/blackhole - queue emptying during peek operation",
+        "category": [
+            "qdisc",
+            "hfsc",
+            "netem",
+            "blackhole"
+        ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+        "setup": [
+            "$IP link set dev $DUMMY up || true",
+            "$IP addr add 10.10.10.10/24 dev $DUMMY || true",
+            "$TC qdisc add dev $DUMMY handle 1:0 root drr",
+            "$TC class add dev $DUMMY parent 1:0 classid 1:1 drr",
+            "$TC class add dev $DUMMY parent 1:0 classid 1:2 drr",
+            "$TC qdisc add dev $DUMMY parent 1:1 handle 2:0 plug limit 1024",
+            "$TC qdisc add dev $DUMMY parent 1:2 handle 3:0 hfsc default 1",
+            "$TC class add dev $DUMMY parent 3:0 classid 3:1 hfsc rt m1 5Mbit d 10ms m2 10Mbit",
+            "$TC qdisc add dev $DUMMY parent 3:1 handle 4:0 netem delay 1ms",
+            "$TC qdisc add dev $DUMMY parent 4:1 handle 5:0 blackhole",
+            "ping -c 3 -W 0.01 -i 0.001 -s 1 10.10.10.10 -I $DUMMY > /dev/null 2>&1 || true",
+            "$TC class change dev $DUMMY parent 3:0 classid 3:1 hfsc sc m1 5Mbit d 10ms m2 10Mbit",
+            "$TC class del dev $DUMMY parent 3:0 classid 3:1",
+            "$TC class add dev $DUMMY parent 3:0 classid 3:1 hfsc rt m1 5Mbit d 10ms m2 10Mbit",
+            "ping -c 3 -W 0.01 -i 0.001 -s 1 10.10.10.10 -I $DUMMY > /dev/null 2>&1 || true"
+        ],
+        "cmdUnderTest": "$TC class change dev $DUMMY parent 3:0 classid 3:1 hfsc sc m1 5Mbit d 10ms m2 10Mbit",
+        "expExitCode": "0",
+        "verifyCmd": "$TC -s qdisc show dev $DUMMY",
+        "matchPattern": "qdisc hfsc 3:.*parent 1:2.*default 1",
+        "matchCount": "1",
+        "teardown": [
+            "$TC qdisc del dev $DUMMY handle 1:0 root",
+            "$IP addr del 10.10.10.10/24 dev $DUMMY || true"
+        ]
     }
 ]
-- 
2.34.1

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

[Konstantin Khlebnikov]

On Mon, 14 Apr 2025 at 03:09, Cong Wang <xiyou.wangcong@gmail.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@starlabs.sg>
> Cc: Konstantin Khlebnikov <koct9i@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.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.

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

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

[Konstantin Khlebnikov]

On Mon, 14 Apr 2025 at 11:39, Konstantin Khlebnikov <koct9i@gmail.com> wrote:
>
> On Mon, 14 Apr 2025 at 03:09, Cong Wang <xiyou.wangcong@gmail.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@starlabs.sg>
> > Cc: Konstantin Khlebnikov <koct9i@gmail.com>
> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.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
> >

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

[Cong Wang]

On Mon, Apr 14, 2025 at 12:09:54PM +0200, Konstantin Khlebnikov wrote:
> On Mon, 14 Apr 2025 at 11:39, Konstantin Khlebnikov <koct9i@gmail.com> wrote:
> >
> > >                 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:

Actually the bug happened when ->peek() returns NULL (hence len = 0),
because otherwise implementation like qdisc_peek_dequeued() would just
add the qlen back:

1125         /* we can reuse ->gso_skb because peek isn't called for root qdiscs */
1126         if (!skb) {
1127                 skb = sch->dequeue(sch);
1128
1129                 if (skb) {
1130                         __skb_queue_head(&sch->gso_skb, skb);
1131                         /* it's still part of the queue */
1132                         qdisc_qstats_backlog_inc(sch, skb);
1133                         sch->q.qlen++;

To me, it is at least harder to interpret len!=0, qlen is more straight
forward and easier to understand. And more importantly update_vf() tests
qlen too, so it is at least more consistent if we just use qlen across
the entire HFSC code.

Thanks.

Re: [Patch net 0/2] net_sched: Fix a UAF vulnerability in HFSC class handling

[Cong Wang]

On Sun, Apr 13, 2025 at 06:09:10PM -0700, Cong Wang wrote:
> This patchset contains a bug fix and a selftest for it, please check
> each patch description for details.
> 

Gerrard reminded me hfsc_dequeue() needs a similar fix. So, I will
update this patchset to include that fix as well. I will post v2
tomorrow.

Thanks!

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

[Konstantin Khlebnikov]

On Mon, 14 Apr 2025 at 21:04, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Mon, Apr 14, 2025 at 12:09:54PM +0200, Konstantin Khlebnikov wrote:
> > On Mon, 14 Apr 2025 at 11:39, Konstantin Khlebnikov <koct9i@gmail.com> wrote:
> > >
> > > >                 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:
>
> Actually the bug happened when ->peek() returns NULL (hence len = 0),
> because otherwise implementation like qdisc_peek_dequeued() would just
> add the qlen back:
>
> 1125         /* we can reuse ->gso_skb because peek isn't called for root qdiscs */
> 1126         if (!skb) {
> 1127                 skb = sch->dequeue(sch);
> 1128
> 1129                 if (skb) {
> 1130                         __skb_queue_head(&sch->gso_skb, skb);
> 1131                         /* it's still part of the queue */
> 1132                         qdisc_qstats_backlog_inc(sch, skb);
> 1133                         sch->q.qlen++;
>
> To me, it is at least harder to interpret len!=0, qlen is more straight
> forward and easier to understand. And more importantly update_vf() tests
> qlen too, so it is at least more consistent if we just use qlen across
> the entire HFSC code.

Ok, that's a matter of taste.

> Thanks.

Reviewed-by: Konstantin Khlebnikov <koct9i@gmail.com>