Use-after-free from netem/hfsc interaction

Use-after-free from netem/hfsc interaction

[Budimir Markovic]

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@kernel.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

Re: Use-after-free from netem/hfsc interaction

[Jakub Kicinski]

On Tue, 1 Oct 2024 16:53:15 +0200 Budimir Markovic wrote:
> Commands to trigger KASAN UaF detection on a drr_class:

Just to be sure - does it repro on latest Linus's tree? there had been
a couple of fixes recently for similar issues.

Re: Use-after-free from netem/hfsc interaction

[Budimir Markovic]

On Wed, Oct 2, 2024 at 2:43 PM Jakub Kicinski <kuba@kernel.org> wrote:
> Just to be sure - does it repro on latest Linus's tree?

Yes

Re: Use-after-free from netem/hfsc interaction

[Paolo Abeni]

On 10/1/24 16:53, Budimir Markovic wrote:
> 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@kernel.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:

If I read correctly, that could happen only via netem peek, right? If 
so, what about constraining the fix into the netem peek callback?

> 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)))

Note that your email client corrupted the patch here. Please fix that 
for a formal patch submission, thanks!

Paolo

Re: Use-after-free from netem/hfsc interaction

[Budimir Markovic]

On Tue, Oct 8, 2024 at 10:23 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> If I read correctly, that could happen only via netem peek, right?

Yes

> what about constraining the fix into the netem peek callback?

I'm not sure what a good way to do this is.

One solution is to try to detect when peek is being called from an enqueue
function. My patch attempted to do that, but I've realized it is possible to
bypass it by calling qdisc_enqueue() from a netem parent during netem_dequeue()
(Eric also pointed out that qdisc_is_running() should not be called from a
qdisc).

Another option would be to move qdisc_enqueue() from netem_dequeue() to
netem_enqueue(), but then there needs to be an alternate way to keep track of
each packet's delay.