netdev - Use-after-free from netem/hfsc interaction

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: <CALk3=6u+PTcc2xhCx3YgWrx3_SzazpXTk1ndDmik+AOi==oq9Q@mail.gmail.com>
Date: Tue, 1 Oct 2024 16:53:15 +0200
From: Budimir Markovic <markovicbudimir@...il.com>
To: netdev@...r.kernel.org
Subject: Use-after-free from netem/hfsc interaction

There is a bug leading to a use-after-free in an interaction between
netem_dequeue() and hfsc_enqueue() (I originally sent this to
security@...nel.org and was told to send it here for further discussion).

If an HFSC RSC class has a netem child qdisc, the peek() in hfsc_enqueue() will
call netem_dequeue() which may drop the packet. When netem_dequeue() drops
a packet, it uses qdisc_tree_reduce_backlog() to decrement its ancestor qdisc's
q.qlens. The problem is that the ancestor qdiscs have not yet accounted for
the packet at this point.

In this case hfsc_enqueue() still returns NET_XMIT_SUCCESS, so the q.qlens have
the correct values at the end. However since they are decremented and
incremented in the wrong order, the ancestor classes may be added to active
lists after qlen_notify() has tried to remove them, leaving dangling pointers.

Commit 50612537e9ab ("netem: fix classful handling") added qdisc_enqueue() to
netem_dequeue(), making it possible for it to drop a packet. Later, commit
12d0ad3be9c3 ("net/sched/sch_hfsc.c: handle corner cases where head may change
invalidating calculated deadline") added a call to peek() to hfsc_enqueue().

The QFQ qdisc also calls peek() from qfq_enqueue(). It cannot be used to create
a dangling pointer in the same way, but may still be exploitable.  I will look
into it more if the patch for this bug does not address it.

A quick fix is to prevent netem_dequeue() from calling qdisc_enqueue() when it
is called from an enqueue function.  I believe qdisc_is_running() can be used
to determine this:

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 39382ee1e..6150a2605 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -698,6 +698,9 @@ static struct sk_buff *netem_dequeue(struct Qdisc *sch)
        struct netem_sched_data *q = qdisc_priv(sch);
        struct sk_buff *skb;

+       if (q->qdisc && !qdisc_is_running(qdisc_root_b
h(sch)))
+               return NULL;
+
 tfifo_dequeue:
        skb = __qdisc_dequeue_head(&sch->q);
        if (skb) {

I do not see a better way to fix the bug without larger changes, such as moving
qdisc_enqueue() out of netem_dequeue() and into netem_enqueue().

Commands to trigger KASAN UaF detection on a drr_class:

ip link set lo up
tc qdisc add dev lo parent 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: handle 4: drr
tc filter add dev lo parent 4: basic action drop
ping -c1 -W0.01 localhost
tc class del dev lo classid 1:1
tc class add dev lo parent 1: classid 1:1 drr
ping -c1 -W0.01 localhost