* Re: [PATCH net] net/sched: re-cap skb->queue_mapping before indexing dev->_tx[]
2026-10-21 15:54 [PATCH net] net/sched: re-cap skb->queue_mapping before indexing dev->_tx[] Dudu Lu
@ 2026-10-23 18:01 ` Jamal Hadi Salim
2026-10-23 18:03 ` Jamal Hadi Salim
2026-10-25 16:54 ` netdev-bot+sashiko
1 sibling, 1 reply; 4+ messages in thread
From: Jamal Hadi Salim @ 2026-10-23 18:01 UTC (permalink / raw)
To: Dudu Lu; +Cc: jiri, netdev
On Wed, Oct 21, 2026 at 11:54 AM Dudu Lu <phx0fer@gmail.com> wrote:
>
> qdisc_restart() computes txq = skb_get_tx_queue(dev, skb), which indexes
> dev->_tx[] with skb->queue_mapping and no bound. A tc BPF program
> attached to a transmit qdisc can store an arbitrary __sk_buff.queue_mapping
> after the tx queue was picked; that store is not re-capped on this path
> (the existing cap in netdev_tx_queue_mapping() only covers the
> skip_txqueue path), so an out-of-range mapping indexes past dev->_tx[]:
>
> BUG: KASAN: slab-out-of-bounds in sch_direct_xmit+0x744/0x89c
> Read of size 8 at addr ffff00000e0802d0 by task A-254-2_S4/100
> sch_direct_xmit+0x744/0x89c
> __qdisc_run+0x34c/0x1d04
> __dev_queue_xmit+0x1aec/0x306c
> Allocated by task 1: alloc_netdev_mqs+0x758/0xfa0
>
> The same stale value is then handed to dev_hard_start_xmit(), which
> reports the same fault. Re-cap the mapping with netdev_cap_txqueue()
> before it is used to pick the tx queue.
>
> Fixes: 10c51b56232d ("net: add skb_get_tx_queue() helper")
> Assisted-by: LLM
> Signed-off-by: Dudu Lu <phx0fer@gmail.com>
> ---
I did not look closely at the rest of your posting, I suspect there
are similar issues in some of them:
please please since you are using AI ask it to check if there is
already a submission for what you are trying to fix on the list.
See:
https://lore.kernel.org/netdev/QDISC-MD1X.v4.r0.20261021042722-1@mojatatu.com/
I got my report from sashiko after submitting a fix for a related
area; hopefully your guy is not just scanning the mailing list...
cheers,
jamal
> Tested base
> -----------
> Detected on Linux 7.3-rc4 (commit 93f51579e7df), arm64 QEMU guest.
> Re-checked against mainline f47d223627f8 (v7.3-341): the affected code
> is unchanged between the two, so the bug is still present there.
> Checked against linux-next next-20261021 (commit 9a482184d910): the
> affected code is unchanged there as well, so the bug is still present in
> linux-next too.
>
> Reproducer status
> -----------------
> A reproducer was written and executed on an arm64 QEMU guest (KASAN,
> CONFIG_NET_SCH_PRIO=y, cls_bpf); it fires at the claimed sink, and still fires on
> a kernel built with ea4d4b5dddb5 applied. The reproducer source is not posted
> here per Documentation/process/security-bugs.rst; it is available on request.
>
> Triggering conditions
> ---------------------
> CAP_NET_ADMIN (attach the classifier and qdisc). The residual is the
> store made by a tc BPF program attached to a transmit qdisc, which ea4d4b5dddb5
> explicitly scopes out.
>
> Notes
> -----
> Built and checkpatch-clean: this patch was applied on top of 7.3-rc4
> (commit 93f51579e7df) and a full arm64 kernel (defconfig) was built with
> it -- no new warnings or errors. scripts/checkpatch.pl reports 0 errors,
> 0 warnings.
>
> net/sched/sch_generic.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 6f6a6f0d5eb0..c326d12942e5 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -432,6 +432,13 @@ static inline bool qdisc_restart(struct Qdisc *q, int *packets, int budget)
> root_lock = qdisc_lock(q);
>
> dev = qdisc_dev(q);
> + /* A tc BPF program attached to a transmit qdisc can store an
> + * arbitrary skb->queue_mapping after the tx queue was picked, so
> + * the value reaching here is not necessarily one the device can
> + * serve. Re-cap it before it indexes dev->_tx[].
> + */
> + skb_set_queue_mapping(skb,
> + netdev_cap_txqueue(dev, skb_get_queue_mapping(skb)));
> txq = skb_get_tx_queue(dev, skb);
>
> return sch_direct_xmit(skb, q, dev, txq, root_lock, validate);
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH net] net/sched: re-cap skb->queue_mapping before indexing dev->_tx[]
2026-10-21 15:54 [PATCH net] net/sched: re-cap skb->queue_mapping before indexing dev->_tx[] Dudu Lu
2026-10-23 18:01 ` Jamal Hadi Salim
@ 2026-10-25 16:54 ` netdev-bot+sashiko
1 sibling, 0 replies; 4+ messages in thread
From: netdev-bot+sashiko @ 2026-10-25 16:54 UTC (permalink / raw)
To: phx0fer; +Cc: jhs, jiri, netdev, kuba
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 2 · 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 fix is incomplete.
- [High] qdisc_restart() caps queue_mapping only on the head skb that
dequeue_skb() returns, but dequeue_skb() can return a list linked…
--
net/sched: re-cap skb->queue_mapping before indexing dev->_tx[]
This commit adds a netdev_cap_txqueue() call in qdisc_restart(). It
clamps skb->queue_mapping before skb_get_tx_queue() indexes dev->_tx[],
because a tc BPF program on a transmit qdisc can store an out-of-range
queue_mapping.
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 6f6a6f0d5eb0d..c326d12942e52 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -432,6 +432,13 @@ static inline bool qdisc_restart(struct Qdisc *q, int *packets, int budget)
> root_lock = qdisc_lock(q);
>
> dev = qdisc_dev(q);
> + /* A tc BPF program attached to a transmit qdisc can store an
> + * arbitrary skb->queue_mapping after the tx queue was picked, so
> + * the value reaching here is not necessarily one the device can
> + * serve. Re-cap it before it indexes dev->_tx[].
> + */
> + skb_set_queue_mapping(skb,
> + netdev_cap_txqueue(dev, skb_get_queue_mapping(skb)));
[Severity: High]
Is this cap early enough? dequeue_skb() runs before it, and it can index
dev->_tx[] with a BPF-written mapping in two places in this file.
First, take a qdisc without TCQ_F_ONETXQUEUE, such as prio as root of a
multiqueue device. There, try_bulk_dequeue_skb_slow() parks an skb whose
mapping differs from the head's on skb_bad_txq, and leaves the mapping
unchanged:
net/sched/sch_generic.c:try_bulk_dequeue_skb_slow() {
...
if (unlikely(skb_get_queue_mapping(nskb) != mapping)) {
qdisc_enqueue_skb_bad_txq(q, nskb);
break;
}
...
}
On the next qdisc_restart(), the path
dequeue_skb()->qdisc_dequeue_skb_bad_txq()->__skb_dequeue_bad_txq()
does:
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);
if (!netif_xmit_frozen_or_stopped(txq)) {
Say the classifier writes 0 on one packet and 1000 on the next. Wouldn't
this read &dev->_tx[1000].state before the new cap runs?
Second, dev_requeue_skb() puts every unsent skb of a returned list on
q->gso_skb without changing it. Only the head of a bulk list is capped
here. So on NETDEV_TX_BUSY, on a stopped queue partway through the list,
or on the XFRM "again" path, the tail skbs are requeued with their
original mapping. The gso_skb branch of dequeue_skb() then does:
/* check the reason of requeuing without tx lock first */
txq = skb_get_tx_queue(txq->dev, skb);
if (!netif_xmit_frozen_or_stopped(txq)) {
using that uncapped value.
The queue_mapping store rewrite in bpf_convert_ctx_access() only drops
stores >= NO_QUEUE_MAPPING. Any value below 0xffff can therefore reach
both paths, so the new comment's "Re-cap it before it indexes dev->_tx[]"
doesn't hold for them.
The existing cap in netdev_tx_queue_mapping() in net/core/dev.c is
applied where the value is produced, not at one consumer. Would capping
after the egress qdisc's classifier runs during enqueue, or in the ctx
store rewrite, cover all of these consumers instead of adding a cap in
qdisc_restart()?
> txq = skb_get_tx_queue(dev, skb);
>
> return sch_direct_xmit(skb, q, dev, txq, root_lock, validate);
[Severity: High]
This caps only the head skb, but dequeue_skb() can return a list linked
through skb->next. try_bulk_dequeue_skb_slow() chains up to 8 skbs that
share the head's original mapping:
int mapping = skb_get_queue_mapping(skb);
...
skb->next = nskb;
skb = nskb;
} while (++cnt < 8);
try_bulk_dequeue_skb() is used for TCQ_F_ONETXQUEUE qdiscs such as
mq/mqprio children. It chains skbs without checking their mapping at all.
Suppose a cls_bpf program writes 100 on every packet. After this change
the head skb maps to 0, but every other skb in the list still carries
100. sch_direct_xmit()->dev_hard_start_xmit() then walks the whole list:
while (skb) {
struct sk_buff *next = skb->next;
skb_mark_not_on_list(skb);
rc = xmit_one(skb, dev, txq, next != NULL);
xmit_one() passes each skb to ndo_start_xmit. Drivers such as virtio_net
index their per-queue arrays with the raw mapping:
drivers/net/virtio_net.c:start_xmit() {
int qnum = skb_get_queue_mapping(skb);
struct send_queue *sq = &vi->sq[qnum];
int err;
struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
...
}
Can this still overrun vi->sq[] and dev->_tx[] for every packet after the
first in a bulk batch?
The commit message says:
The same stale value is then handed to dev_hard_start_xmit(), which
reports the same fault.
That path still looks reachable for the tail skbs. These tail skbs can
also be requeued to gso_skb, where they hit the uncapped lookup in
dequeue_skb() described above.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261021155450.66934-1-phx0fer%40gmail.com
^ permalink raw reply [flat|nested] 4+ messages in thread