Re: [PATCH net v3] net/sched: taprio: fix NULL pointer dereference in class dump

[Weiming Shi <bestswngs@gmail.com>]

On 26-04-16 11:50, Jamal Hadi Salim wrote:
> On Tue, Apr 14, 2026 at 6:43 AM Weiming Shi <bestswngs@gmail.com> wrote:
> >
> > When a TAPRIO child qdisc is deleted via RTM_DELQDISC, taprio_graft()
> > is called with new == NULL and stores NULL into q->qdiscs[cl - 1].
> > Subsequent RTM_GETTCLASS dump operations walk all classes via
> > taprio_walk() and call taprio_dump_class(), which calls taprio_leaf()
> > returning the NULL pointer, then dereferences it to read child->handle,
> > causing a kernel NULL pointer dereference.
> >
> > The bug is reachable with namespace-scoped CAP_NET_ADMIN on any kernel
> > with CONFIG_NET_SCH_TAPRIO enabled. On systems with unprivileged user
> > namespaces enabled, an unprivileged local user can trigger a kernel
> > panic by creating a taprio qdisc inside a new network namespace,
> > grafting an explicit child qdisc, deleting it, and requesting a class
> > dump. The RTM_GETTCLASS dump itself requires no capability.
> >
> 
> While i would say this looks good to me, I hate to sound like a broken
> record but:
> 
> Do you have a reproducer?
> And it would be nice to get a tdc test from you.
> 
> Also, please use assisted-by: or even suggested-by if this patch was
> suggested by AI.
> 
> cheers,
> jamal

Hi Jamal,

Thanks for the review. Reproducer below -- tested on a KASAN kernel
under vng:

```
unshare -Urn
ip link add veth0 numtxqueues 8 numrxqueues 8 type veth \
    peer name veth1 numtxqueues 8 numrxqueues 8
ip link set veth0 up
tc qdisc replace dev veth0 root handle 100: taprio       \
    num_tc 8 map 0 1 2 3 4 5 6 7                         \
    queues 1@0 1@1 1@2 1@3 1@4 1@5 1@6 1@7               \
    base-time 0 sched-entry S ff 20000000 clockid CLOCK_TAI
tc qdisc add dev veth0 parent 100:1 handle 200: pfifo
tc qdisc del dev veth0 parent 100:1 handle 200:
tc class show dev veth0
```
A C PoC is also available if needed.

I'll test and send a v4 with the tdc test case and an Assisted-by tag.

Thanks,
Weiming Shi
> 
> 
> >  Oops: general protection fault, probably for non-canonical address 0xdffffc0000000007: 0000 [#1] SMP KASAN NOPTI
> >  KASAN: null-ptr-deref in range [0x0000000000000038-0x000000000000003f]
> >  RIP: 0010:taprio_dump_class (net/sched/sch_taprio.c:2475)
> >  Call Trace:
> >   <TASK>
> >   tc_fill_tclass (net/sched/sch_api.c:1966)
> >   qdisc_class_dump (net/sched/sch_api.c:2329)
> >   taprio_walk (net/sched/sch_taprio.c:2510)
> >   tc_dump_tclass_qdisc (net/sched/sch_api.c:2353)
> >   tc_dump_tclass_root (net/sched/sch_api.c:2370)
> >   tc_dump_tclass (net/sched/sch_api.c:2431)
> >   rtnl_dumpit (net/core/rtnetlink.c:6827)
> >   netlink_dump (net/netlink/af_netlink.c:2325)
> >   rtnetlink_rcv_msg (net/core/rtnetlink.c:6927)
> >   netlink_rcv_skb (net/netlink/af_netlink.c:2550)
> >   </TASK>
> >
> > Fix this by substituting &noop_qdisc when new is NULL in
> > taprio_graft(), following the same pattern used by multiq_graft() and
> > prio_graft(). This ensures q->qdiscs[] slots are never NULL, making
> > control-plane dump paths safe without requiring individual NULL checks.
> >
> > Since the data-plane paths (taprio_enqueue and taprio_dequeue_from_txq)
> > previously had explicit NULL guards that would drop/skip the packet
> > cleanly, update those checks to test for &noop_qdisc instead. Without
> > this, packets would reach taprio_enqueue_one() which increments the root
> > qdisc's qlen and backlog before calling the child's enqueue; noop_qdisc
> > drops the packet but those counters are never rolled back, permanently
> > inflating the root qdisc's statistics.
> >
> > Fixes: 665338b2a7a0 ("net/sched: taprio: dump class stats for the actual q->qdiscs[]")
> > Reported-by: Xiang Mei <xmei5@asu.edu>
> > Signed-off-by: Weiming Shi <bestswngs@gmail.com>
> > ---
> > v3: fix broken patch
> > v2: Also update NULL guards in taprio_enqueue() and
> >     taprio_dequeue_from_txq() to avoid qlen/backlog inflation (Paolo).
> > ---
> >  net/sched/sch_taprio.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> > index f721c03514f60..07723b156c5b3 100644
> > --- a/net/sched/sch_taprio.c
> > +++ b/net/sched/sch_taprio.c
> > @@ -634,7 +634,7 @@ static int taprio_enqueue(struct sk_buff *skb, struct Qdisc *sch,
> >         queue = skb_get_queue_mapping(skb);
> >
> >         child = q->qdiscs[queue];
> > -       if (unlikely(!child))
> > +       if (unlikely(child == &noop_qdisc))
> >                 return qdisc_drop(skb, sch, to_free);
> >
> >         if (taprio_skb_exceeds_queue_max_sdu(sch, skb)) {
> > @@ -717,7 +717,7 @@ static struct sk_buff *taprio_dequeue_from_txq(struct Qdisc *sch, int txq,
> >         int len;
> >         u8 tc;
> >
> > -       if (unlikely(!child))
> > +       if (unlikely(child == &noop_qdisc))
> >                 return NULL;
> >
> >         if (TXTIME_ASSIST_IS_ENABLED(q->flags))
> > @@ -2183,6 +2183,9 @@ static int taprio_graft(struct Qdisc *sch, unsigned long cl,
> >         if (!dev_queue)
> >                 return -EINVAL;
> >
> > +       if (!new)
> > +               new = &noop_qdisc;
> > +
> >         if (dev->flags & IFF_UP)
> >                 dev_deactivate(dev);
> >
> > @@ -2196,14 +2199,14 @@ static int taprio_graft(struct Qdisc *sch, unsigned long cl,
> >         *old = q->qdiscs[cl - 1];
> >         if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
> >                 WARN_ON_ONCE(dev_graft_qdisc(dev_queue, new) != *old);
> > -               if (new)
> > +               if (new != &noop_qdisc)
> >                         qdisc_refcount_inc(new);
> >                 if (*old)
> >                         qdisc_put(*old);
> >         }
> >
> >         q->qdiscs[cl - 1] = new;
> > -       if (new)
> > +       if (new != &noop_qdisc)
> >                 new->flags |= TCQ_F_ONETXQUEUE | TCQ_F_NOPARENT;
> >
> >         if (dev->flags & IFF_UP)
> > --
> > 2.43.0
> >