netdev - Re: 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>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <608e4396-69b2-4a04-9229-e6bff8de1fc3@redhat.com>
Date: Tue, 8 Oct 2024 10:23:31 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Budimir Markovic <markovicbudimir@...il.com>, netdev@...r.kernel.org
Subject: Re: Use-after-free from netem/hfsc interaction

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

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