[PATCH net] net/sched: clamp skb->queue_mapping before indexing the tx queue

[PATCH net] net/sched: clamp skb->queue_mapping before indexing the tx queue

[Jamal Hadi Salim]

This issue was caught by Sashiko (nipa).
A producer-side path cap is already merged (ea4d4b5dddb5); this
patch adds the matching consumer-side cap.

The tx queue is picked and capped in netdev_core_pick_tx(), before
__dev_xmit_skb() qdisc egress path. A tc BPF program running inside
egress qdisc (not clsact) can then change __sk_buff->queue_mapping;
that update is never re-checked. bpf_convert_ctx_access() bounds it
only by NO_QUEUE_MAPPING, not by the device's queue count.

On dequeue, qdisc_restart() we end up in netdev_get_tx_queue() which
indexes dev->_tx[] with that value. netdev_get_tx_queue() guards it with
just a DEBUG_NET_WARN_ON_ONCE, so sch_direct_xmit() takes the out-of-bounds
txq->_xmit_lock and writes txq->xmit_lock_owner past the end of the
allocation on non-lltx devices such as ifb. Drivers that index their rings
from skb_get_queue_mapping() (ex: ifb) go out of range the same way.

Cap the mapping at the consumer and write the capped value back, so both
the qdisc's _tx[] lookup and any later driver read stay in range. The cap
folds into the dequeue path instead of a separate walk over the returned
list: dequeue_skb() caps the head, and each bulk helper caps the element it
appends. netdev_cap_txqueue() already logs a ratelimited notice and selects
queue 0. The single-packet case pays one extra compare against
real_num_tx_queues.

To recreate the bug: load a SCHED_CLS or SCHED_ACT program that stores
__sk_buff->queue_mapping (a tc BPF action is enough), attach it to a
transmit qdisc (for example a matchall filter on a prio root), and send a
packet. With a 1-queue device a store of 1 indexes one past dev->_tx[].
Under KASAN with panic_on_warn=1 the unfixed kernel faults with
"BUG: KASAN: slab-out-of-bounds in sch_direct_xmit". Loading the program
needs CAP_BPF/CAP_SYS_ADMIN, so the trigger is root.

Fixes: 74e31ca850c1 ("bpf: add skb->queue_mapping write access from tc clsact")
Reported-by: Sashiko (nipa) <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/netdev/179033713973.2160803.4914570693994398206@kernel.org/
Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-9R8V.v2.20260924072708%40mojatatu.com
Reviewed-by: Victor Nogueira <victor@mojatatu.com>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 net/sched/sch_generic.c | 45 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 6f6a6f0d5eb0..e3e76e794aae 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -96,9 +96,33 @@ static void qdisc_maybe_clear_missed(struct Qdisc *q,

 #define SKB_XOFF_MAGIC ((struct sk_buff *)1UL)

+/* A tc BPF program attached to a transmit qdisc runs inside q->enqueue(),
+ * after the tx queue has been picked and capped, so its store to
+ * __sk_buff->queue_mapping is not re-capped. Bound it against the device at
+ * the consumer and write the capped value back, so a driver that re-reads
+ * skb_get_queue_mapping() in ndo_start_xmit() also indexes in range.
+ */
+static u16 qdisc_cap_skb_tx_queue(struct net_device *dev, struct sk_buff *skb)
+{
+	u16 queue = skb_get_queue_mapping(skb);
+	u16 capped;
+
+	capped = netdev_cap_txqueue(dev, queue);
+	if (unlikely(capped != queue))
+		skb_set_queue_mapping(skb, capped);
+
+	return capped;
+}
+
+static struct netdev_queue *skb_cap_tx_queue(struct net_device *dev,
+					     struct sk_buff *skb)
+{
+	return netdev_get_tx_queue(dev, qdisc_cap_skb_tx_queue(dev, skb));
+}
+
 static inline struct sk_buff *__skb_dequeue_bad_txq(struct Qdisc *q)
 {
-	const struct netdev_queue *txq = q->dev_queue;
+	const struct netdev_queue *txq;
 	spinlock_t *lock = NULL;
 	struct sk_buff *skb;

@@ -110,7 +134,7 @@ static inline struct sk_buff *__skb_dequeue_bad_txq(struct Qdisc *q)
 	skb = skb_peek(&q->skb_bad_txq);
 	if (skb) {
 		/* check the reason of requeuing without tx lock first */
-		txq = skb_get_tx_queue(txq->dev, skb);
+		txq = skb_cap_tx_queue(qdisc_dev(q), skb);
 		if (!netif_xmit_frozen_or_stopped(txq)) {
 			skb = __skb_dequeue(&q->skb_bad_txq);
 			if (qdisc_is_percpu_stats(q)) {
@@ -208,6 +232,7 @@ static void try_bulk_dequeue_skb(struct Qdisc *q,
 				 int *packets, int budget)
 {
 	int bytelimit = qdisc_avail_bulklimit(txq) - skb->len;
+	struct net_device *dev = qdisc_dev(q);
 	int cnt = 0;

 	while (bytelimit > 0) {
@@ -216,6 +241,10 @@ static void try_bulk_dequeue_skb(struct Qdisc *q,
 		if (!nskb)
 			break;

+		/* A tc BPF store may have poisoned the mapping after pick;
+		 * cap every element before it reaches the driver.
+		 */
+		qdisc_cap_skb_tx_queue(dev, nskb);
 		bytelimit -= nskb->len; /* covers GSO len */
 		skb->next = nskb;
 		skb = nskb;
@@ -233,7 +262,7 @@ static void try_bulk_dequeue_skb_slow(struct Qdisc *q,
 				      struct sk_buff *skb,
 				      int *packets)
 {
-	int mapping = skb_get_queue_mapping(skb);
+	struct net_device *dev = qdisc_dev(q);
 	struct sk_buff *nskb;
 	int cnt = 0;

@@ -241,7 +270,11 @@ static void try_bulk_dequeue_skb_slow(struct Qdisc *q,
 		nskb = q->dequeue(q);
 		if (!nskb)
 			break;
-		if (unlikely(skb_get_queue_mapping(nskb) != mapping)) {
+		/* cap every element; the head was already capped, so a
+		 * poisoned follower now compares equal to a poisoned head
+		 */
+		if (unlikely(qdisc_cap_skb_tx_queue(dev, nskb) !=
+			     skb_get_queue_mapping(skb))) {
 			qdisc_enqueue_skb_bad_txq(q, nskb);
 			break;
 		}
@@ -286,7 +319,7 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
 		if (xfrm_offload(skb))
 			*validate = true;
 		/* check the reason of requeuing without tx lock first */
-		txq = skb_get_tx_queue(txq->dev, skb);
+		txq = skb_cap_tx_queue(qdisc_dev(q), skb);
 		if (!netif_xmit_frozen_or_stopped(txq)) {
 			skb = __skb_dequeue(&q->gso_skb);
 			if (qdisc_is_percpu_stats(q)) {
@@ -322,6 +355,8 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
 	skb = q->dequeue(q);
 	if (skb) {
 bulk:
+		/* cap the head; the bulk helpers cap every appended element */
+		qdisc_cap_skb_tx_queue(qdisc_dev(q), skb);
 		if (qdisc_may_bulk(q))
 			try_bulk_dequeue_skb(q, skb, txq, packets, budget);
 		else
--
2.43.0

Re: [PATCH net] net/sched: clamp skb->queue_mapping before indexing the tx queue

[netdev-bot+sashiko]

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

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

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

- [High] The patch fixes the out-of-range index, but on the
  TCQ_F_ONETXQUEUE fast bulk path a packet whose queue_mapping does not…

--

net/sched: clamp skb->queue_mapping before indexing the tx queue

A tc BPF program running inside an egress qdisc can store an out-of-range
skb->queue_mapping after netdev_core_pick_tx() has already capped it. This
patch adds the cap on the qdisc dequeue side. dequeue_skb() caps the head
skb and the bulk helpers cap each skb they append. The capped value is
written back to the skb, and an out-of-range mapping becomes 0.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-9R8V.v2.20260924072708%40mojatatu.com

> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 6f6a6f0d5eb0d..e3e76e794aae2 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -96,9 +96,33 @@ static void qdisc_maybe_clear_missed(struct Qdisc *q,
>  
>  #define SKB_XOFF_MAGIC ((struct sk_buff *)1UL)
>  
> +/* A tc BPF program attached to a transmit qdisc runs inside q->enqueue(),
> + * after the tx queue has been picked and capped, so its store to
> + * __sk_buff->queue_mapping is not re-capped. Bound it against the device at
> + * the consumer and write the capped value back, so a driver that re-reads
> + * skb_get_queue_mapping() in ndo_start_xmit() also indexes in range.
> + */

[ ... ]

> @@ -208,6 +232,7 @@ static void try_bulk_dequeue_skb(struct Qdisc *q,
>  				 int *packets, int budget)
>  {
>  	int bytelimit = qdisc_avail_bulklimit(txq) - skb->len;
> +	struct net_device *dev = qdisc_dev(q);
>  	int cnt = 0;
>  
>  	while (bytelimit > 0) {
> @@ -216,6 +241,10 @@ static void try_bulk_dequeue_skb(struct Qdisc *q,
>  		if (!nskb)
>  			break;
>  
> +		/* A tc BPF store may have poisoned the mapping after pick;
> +		 * cap every element before it reaches the driver.
> +		 */
> +		qdisc_cap_skb_tx_queue(dev, nskb);
>  		bytelimit -= nskb->len; /* covers GSO len */
>  		skb->next = nskb;
>  		skb = nskb;

[Severity: High]
Can a follower whose queue_mapping differs from the head's still reach the
driver under the wrong tx queue lock here?

The return value of qdisc_cap_skb_tx_queue() is ignored, and nskb is
appended no matter what. try_bulk_dequeue_skb_slow() compares each
follower with the head and diverts mismatches:

    if (unlikely(qdisc_cap_skb_tx_queue(dev, nskb) !=
                 skb_get_queue_mapping(skb))) {
        qdisc_enqueue_skb_bad_txq(q, nskb);
        break;
    }

This fast path has no such check. It is the path taken on BQL devices by
TCQ_F_ONETXQUEUE qdiscs. That includes the per-queue children of mq,
mqprio, taprio and htb offload, which get the flag in mq_init_common()
and mq_graft().

The whole list is then sent under the head's txq only:

qdisc_restart()
    txq = skb_get_tx_queue(dev, skb);
  sch_direct_xmit()
    HARD_TX_LOCK(dev, txq, smp_processor_id());
    dev_hard_start_xmit(skb, dev, txq, &ret);

Capping to 0 can create a mismatch in two ways:

  - In an mq child bound to txq N, a tc BPF program sets a follower's
    mapping out of range. The follower is rewritten to 0, but it is still
    sent under txq N's lock.

  - A poisoned head is capped to 0 in dequeue_skb(). The whole list then
    goes out under txq 0's lock, while unpoisoned followers keep mapping
    N.

Drivers that pick their ring from the skb mapping would then touch
another queue's ring and BQL state without holding that queue's
_xmit_lock. virtio_net start_xmit() is one example:

    int qnum = skb_get_queue_mapping(skb);
    struct send_queue *sq = &vi->sq[qnum];

igb is another. igb_tx_queue_mapping() picks the ring, and
netdev_tx_sent_queue(txring_txq(tx_ring)) updates BQL.

Meanwhile another CPU can legitimately hold the right lock: the mq child
that owns that txq, PACKET_QDISC_BYPASS, or netpoll. Could that race on
the descriptor ring and the dql counters? It would also let packets
bypass a stopped txq 0.

igb also changes behaviour in one case. Take a 2-ring device and a
follower stored as 3 inside mq child 1. igb's modulo used to map it to
ring 1, which matched the held lock. With this patch it goes to ring 0
while txq 1's lock is held.

There is also a possible stall. dequeue_skb() still runs the ONETXQUEUE
frozen pre-check and computes the bulk byte limit against q->dev_queue
(txq N). A capped head that is requeued into child N's gso_skb now
depends on txq 0 instead. When txq 0 wakes, netif_tx_wake_queue()
reschedules txq 0's qdisc, not child N. Could child N stall until
something else is enqueued to it?

The new comment says a driver that re-reads skb_get_queue_mapping()
"also indexes in range". Staying in range doesn't look sufficient,
because per-queue driver state is protected by the matching txq lock.

ifb is not affected by this fast path. It doesn't use BQL, so
qdisc_avail_bulklimit() returns 0 and no followers are appended. The
drivers exposed are BQL multi-queue ones such as virtio_net, igb and
ixgbe.

Would it work to compare each follower's capped mapping with the head's
here, and push mismatches to skb_bad_txq the way the slow path does?
Another option for ONETXQUEUE qdiscs would be to cap to the index of
q->dev_queue instead of 0.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-MD1X.v1.20261005193231%40mojatatu.com