Re: [PATCH net v3 3/4] net/sched: sch_teql: reject qdiscs from another network namespace

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 2

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The fix checks that the slave device and the master are in the
  same network namespace only once, in teql_qdisc_init()…
- [Low] The commit message says only that a qdisc from another netns
  could attach to the init-net master "and change its MTU and flags".
- [Low] Moving skb_queue_head_init(&q->q) to the top of teql_qdisc_init()
  (sch_teql.c:203) is presented as groundwork for the new netns failure…

--

net/sched: sch_teql: reject qdiscs from another network namespace

This patch makes teql_qdisc_init() reject a teql qdisc whose device is
in a different network namespace than the teql master. It returns
-EOPNOTSUPP with an extack message. It also moves skb_queue_head_init()
of the private queue to the top of teql_qdisc_init(), so every
constructor failure path sees an initialized queue.

Link: https://lore.kernel.org/netdev/20261022144830.67948-1-shawnq1@163.com/
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v1.r2.20261031160050-3%40mojatatu.com
Link: https://lore.kernel.org/netdev/179366641863.488856.17368571591466000274@kernel.org/

> The issue:
> The teql master devices are created once in the initial network
> namespace and their Qdisc_ops are global. teql_qdisc_init() takes the
> teql_master embedded in the registered sch->ops and uses it without
> checking whether the new qdisc's device lives in the same network
> namespace as the master. A qdisc created in another netns can therefore
> attach to the init-net master and change its MTU and flags.

[Severity: Low]
Does this description understate the impact of the bug?

Besides changing the MTU and flags, teql_qdisc_init() links the foreign
qdisc into the master's slave ring, or makes it the first slave:

    rcu_assign_pointer(NEXT_SLAVE(first), sch);

>From then on, teql_master_xmit() round-robins init_net traffic sent
through teqlN onto the device from the other netns. There is no netns
check and no skb scrubbing:

    skb->dev = slave;
    if (!netif_xmit_frozen_or_stopped(slave_txq))
        rc = netdev_start_xmit(skb, slave, slave_txq, false);

If teql0 is already up with slaves, the attach still succeeds as long as
the foreign device has compatible flags and an MTU at least as large as
the master's. A CAP_NET_ADMIN holder in a user namespace can set both.

With a veth slave, the netns owner could capture or drop part of the
host's teql traffic. It could also answer ARP that __teql_resolve()
sends on that device.

The code change closes this path as well. Since stable and CVE triage
depend on the stated impact, could the commit message describe the
cross-netns traffic interception?

> Initialize the private skb queue before any validation so every
> constructor failure path, including qdisc_create_dflt(), which resets the
> qdisc before destroying it, sees an initialized empty queue.

[Severity: Low]
This isn't a bug, but should the commit message say that this reorder
also fixes an existing NULL pointer dereference?

Before this patch, three returns happened before skb_queue_head_init():
-EINVAL (hard_header_len), -ELOOP (m->dev == dev) and -EOPNOTSUPP
(non-root parent). On those paths q->q was still zeroed. When init
fails, qdisc_create_dflt() calls qdisc_put(), which leads to:

    qdisc_put()
      __qdisc_destroy()
        qdisc_reset()
          teql_reset()
            skb_queue_purge(&dat->q)

skb_queue_purge_reason() sees list->next == NULL. That is not equal to
list, so it treats the queue as non-empty, and skb_queue_splice_init()
dereferences a NULL first pointer.

This looks reachable with net.core.default_qdisc=teql0. mq, mqprio and
taprio create children with a non-root parent, and
attach_one_default_qdisc() on teql0 itself returns -ELOOP. Setting
default_qdisc needs CAP_NET_ADMIN in init_net.

Would it be worth calling out the crash fix explicitly, for backports
and Fixes: attribution?

> diff --git a/net/sched/sch_teql.c b/net/sched/sch_teql.c
> index 70c948a26f809..dc21cd9704a25 100644
> --- a/net/sched/sch_teql.c
> +++ b/net/sched/sch_teql.c

[ ... ]

> @@ -211,9 +213,12 @@ static int teql_qdisc_init(struct Qdisc *sch, struct nlattr *opt,
>  		return -EOPNOTSUPP;
>  	}
>  
> -	q->m = m;
> +	if (dev_net(dev) != dev_net(m->dev)) {
> +		NL_SET_ERR_MSG_MOD(extack, "teql master is in a different network namespace");
> +		return -EOPNOTSUPP;
> +	}

[Severity: Medium]
Is a netns check at attach time enough to keep the master and its
slaves in the same namespace?

teql_master_setup() never sets dev->netns_immutable. That means
__dev_change_net_namespace() lets teqlN move to another netns while
slaves are attached, because this is the only guard:

    if (dev->netns_immutable) {
        NL_SET_ERR_MSG(extack, "The interface netns is immutable");
        goto out;
    }

The move runs dev_shutdown() only on teqlN's own qdisc. The teql slave
qdiscs on the init-net devices stay linked in m->slaves:
teql_master_close() only stops the queue, and teql registers no netdev
notifier.

Once the master is in netns B, three things can happen:

The owner of B, who may be root in an unprivileged user namespace,
passes the new dev_net(dev) == dev_net(m->dev) check. It can then add
its own devices to a ring that still holds init-net slaves.

teql_master_open() takes the MTU and flags from the init-net slaves.

Traffic sent through teqlN in B reaches teql_master_xmit(), which sets
skb->dev = slave and calls netdev_start_xmit() on the init-net slave.
Through __teql_resolve(), it also calls ipv4_neigh_create() on the
init-net slave.

Moving teqlN out of init_net needs CAP_NET_ADMIN in init_net, so the
unprivileged user cannot do this alone. After this patch, though,
moving the master seems to be the only way a container can use teql.

Should the master be marked netns_immutable? Alternatively, should a
netns change of the master be refused, or detach the existing slaves?

> -	skb_queue_head_init(&q->q);
> +	q->m = m;
>  
>  	raw_spin_lock_irqsave(&m->slaves_lock, flags);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/SER-NSBD.v3.r0.20261103064310%40mojatatu.com