| Message ID | CALk3=6u+PTcc2xhCx3YgWrx3_SzazpXTk1ndDmik+AOi==oq9Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Use-after-free from netem/hfsc interaction | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
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.
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
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
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.
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);
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 (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