[PATCH 1/2] net: fix OOB read in qdisc_pkt_len_segs_init() on negative transport offset

[PATCH 1/2] net: fix OOB read in qdisc_pkt_len_segs_init() on negative transport offset

[zjamg]

In qdisc_pkt_len_segs_init(), the header length up to the transport layer
is computed via skb_transport_offset() or skb_inner_transport_offset():

	/* mac layer + network layer */
	if (!skb->encapsulation) {
		if (unlikely(!skb_transport_header_was_set(skb)))
			return SKB_NOT_DROPPED_YET;
		hdr_len = skb_transport_offset(skb);
	} else {
		hdr_len = skb_inner_transport_offset(skb);
	}

Both offset accessors return a signed int, but hdr_len is declared as
unsigned int.

If a packet undergoes header operations (such as skb_vlan_untag() pulling
stacked VLAN tags without updating inner_transport_header, or other header
stripping that advances skb->data past the transport header), the offset
becomes negative.

When stored into unsigned int hdr_len, negative values are converted to
large positive numbers near UINT_MAX (e.g. (unsigned int)-16 is
0xFFFFFFF0). Subsequently:

1. pskb_may_pull(skb, hdr_len + sizeof(struct tcphdr)) evaluates
   0xFFFFFFF0 + 20, which overflows 32-bit unsigned arithmetic to 4.
   Since the packet length exceeds 4 bytes, pskb_may_pull() returns true,
   bypassing the bounds check.

2. th = (const struct tcphdr *)(skb->data + hdr_len) zero-extends hdr_len
   on 64-bit architectures, creating a wild pointer pointing ~4 GiB past
   skb->data into unmapped memory.

3. __tcp_hdrlen(th) dereferences th->doff, triggering an immediate kernel
   crash (page fault / KASAN wild-memory-access panic).

A similar unsigned overflow occurs for UDP (SKB_GSO_UDP_L4).

Fix this by declaring hdr_len as int to match the return types of
skb_transport_offset() and skb_inner_transport_offset(), and explicitly
rejecting negative offsets by returning SKB_DROP_REASON_SKB_BAD_GSO.

Fixes: 7fb4c1967011 ("net: pull headers in qdisc_pkt_len_segs_init()")
Cc: stable@vger.kernel.org
Signed-off-by: zjamg <ndaugoing@gmail.com>
---
 net/core/dev.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index ecfbd72d5d1a..f2dbf99181ec 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4159,8 +4159,9 @@ EXPORT_SYMBOL_GPL(validate_xmit_skb_list);
 static enum skb_drop_reason qdisc_pkt_len_segs_init(struct sk_buff *skb)
 {
 	struct skb_shared_info *shinfo = skb_shinfo(skb);
-	unsigned int hdr_len, tlen;
+	unsigned int tlen;
 	u16 gso_segs;
+	int hdr_len;
 
 	qdisc_skb_cb(skb)->pkt_len = skb->len;
 	if (!shinfo->gso_size) {
@@ -4182,6 +4183,8 @@ static enum skb_drop_reason qdisc_pkt_len_segs_init(struct sk_buff *skb)
 	} else {
 		hdr_len = skb_inner_transport_offset(skb);
 	}
+	if (unlikely(hdr_len < 0))
+		return SKB_DROP_REASON_SKB_BAD_GSO;
 	/* + transport layer */
 	if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) {
 		const struct tcphdr *th;
-- 
2.53.0

[PATCH 2/2] net/sched: sch_cake: check negative transport offset in cake_overhead()

[zjamg]

In cake_overhead(), the header length up to the transport layer is
computed using logic borrowed from qdisc_pkt_len_segs_init():

	/* borrowed from qdisc_pkt_len_segs_init() */
	if (!skb->encapsulation)
		hdr_len = skb_transport_offset(skb);
	else
		hdr_len = skb_inner_transport_offset(skb);

Both skb_transport_offset() and skb_inner_transport_offset() return a
signed int, but hdr_len is declared as unsigned int.

If an encapsulated or stripped packet has a negative transport offset,
hdr_len is converted to a huge positive integer near UINT_MAX. When
passed to skb_header_pointer(skb, hdr_len, ...), the signed offset argument
in skb_header_pointer() becomes negative, which can lead to an
out-of-bounds pointer (skb->data + negative_offset) and an OOB read.

Fix this by declaring hdr_len as int and falling back to the standard
cake_calc_overhead() calculation if hdr_len is negative.

Fixes: a41851bea7bf ("net: account for encap headers in qdisc pkt len")
Cc: stable@vger.kernel.org
Signed-off-by: zjamg <ndaugoing@gmail.com>
---
 net/sched/sch_cake.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index dc93267029e7..ce9e85f9571e 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -1413,10 +1413,11 @@ static u32 cake_calc_overhead(struct cake_sched_data *qd, u32 len, u32 off)
 static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
 {
 	const struct skb_shared_info *shinfo = skb_shinfo(skb);
-	unsigned int hdr_len, last_len = 0;
+	unsigned int last_len = 0;
 	u32 off = skb_network_offset(skb);
 	u16 segs = qdisc_pkt_segs(skb);
 	u32 len = qdisc_pkt_len(skb);
+	int hdr_len;
 
 	WRITE_ONCE(q->avg_netoff, cake_ewma(q->avg_netoff, off << 16, 8));
 
@@ -1429,6 +1430,9 @@ static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
 	else
 		hdr_len = skb_inner_transport_offset(skb);
 
+	if (unlikely(hdr_len < 0))
+		return cake_calc_overhead(q, len, off);
+
 	/* + transport layer */
 	if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 |
 						SKB_GSO_TCPV6))) {
-- 
2.53.0

Re: [PATCH 2/2] net/sched: sch_cake: check negative transport offset in cake_overhead()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 2 · 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).

- [Medium] Commit message asserts a memory-safety bug that the cited code
  path cannot produce: "can lead to an out-of-bounds pointer (skb->data…
- [Medium] cake_overhead() (net/sched/sch_cake.c:1427-1434) copies the
  header-length computation from qdisc_pkt_len_segs_init() but imports…

Pre-existing issues:
- [High] cake_overhead() only special-cases `segs == 1`
  (net/sched/sch_cake.c:1424); `segs == 0` falls into the multi-segment…

--

net/sched: sch_cake: check negative transport offset in cake_overhead()

The patch changes hdr_len in cake_overhead() from unsigned int to int and
returns the plain cake_calc_overhead() result when the computed transport
offset is negative.  A couple of questions about the reasoning in the
changelog and about the completeness of the new guard.

Quoting from the changelog:

>     If an encapsulated or stripped packet has a negative transport offset,
>     hdr_len is converted to a huge positive integer near UINT_MAX. When
>     passed to skb_header_pointer(skb, hdr_len, ...), the signed offset argument
>     in skb_header_pointer() becomes negative, which can lead to an
>     out-of-bounds pointer (skb->data + negative_offset) and an OOB read.

[Severity: Medium]
Can this path really produce a read outside the allocation?

The offset ends up in __skb_header_pointer(), whose offset parameter is
already an int, so the unsigned conversion is undone:

include/linux/skbuff.h:__skb_header_pointer() {
	if (likely(hlen - offset >= len))
		return (void *)data + offset;
	...
}

The negative value comes from skb_transport_offset() /
skb_inner_transport_offset(), which are head-relative:

	return skb->head + skb->transport_header - skb->data;

So skb->data + hdr_len == skb->head + skb->transport_header, which is at
or after skb->head and still inside the skb head buffer.  The
skb_copy_bits() fallback reads from the same allocation.  Is the actual
pre-patch consequence garbage header-length accounting rather than an OOB
read?

Relatedly, could the changelog say how a negative offset reaches
cake_overhead() at all?  __dev_queue_xmit() already rejects such GSO skbs
before any ->enqueue() runs:

net/core/dev.c:__dev_queue_xmit() {
	reason = qdisc_pkt_len_segs_init(skb);
	if (unlikely(reason)) {
		dev_core_stats_tx_dropped_inc(dev);
		kfree_skb_reason(skb, reason);
		return -EINVAL;
	}
	...
}

which leaves header mangling done after that point (nf_hook_egress(),
sch_handle_egress(), e.g. bpf_skb_adjust_room() ->
bpf_skb_net_shrink()) as the remaining route.  With Fixes: and
Cc: stable on the patch, spelling out the reachable trigger would help
backporters decide whether this is a memory-safety fix or accounting
hardening.

> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index dc93267029e70..ce9e85f9571e6 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -1413,10 +1413,11 @@ static u32 cake_calc_overhead(struct cake_sched_data *qd, u32 len, u32 off)
>  static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
>  {
>  	const struct skb_shared_info *shinfo = skb_shinfo(skb);
> -	unsigned int hdr_len, last_len = 0;
> +	unsigned int last_len = 0;
>  	u32 off = skb_network_offset(skb);
>  	u16 segs = qdisc_pkt_segs(skb);
>  	u32 len = qdisc_pkt_len(skb);
> +	int hdr_len;
>  
>  	WRITE_ONCE(q->avg_netoff, cake_ewma(q->avg_netoff, off << 16, 8));
>  
> @@ -1429,6 +1430,9 @@ static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
>  	else
>  		hdr_len = skb_inner_transport_offset(skb);
>  
> +	if (unlikely(hdr_len < 0))
> +		return cake_calc_overhead(q, len, off);
> +

[Severity: Medium]
Should the companion guard from qdisc_pkt_len_segs_init() be imported as
well?  That function has two tests, not one:

net/core/dev.c:qdisc_pkt_len_segs_init() {
	if (!skb->encapsulation) {
		if (unlikely(!skb_transport_header_was_set(skb)))
			return SKB_NOT_DROPPED_YET;
		hdr_len = skb_transport_offset(skb);
	} else {
		hdr_len = skb_inner_transport_offset(skb);
	}
	if (unlikely(hdr_len < 0))
		return SKB_DROP_REASON_SKB_BAD_GSO;
	...
}

When the transport header was never set it holds the sentinel installed by
__finalize_skb_around():

	skb->transport_header = (typeof(skb->transport_header))~0U;

so skb_transport_offset() returns 65535 - (skb->data - skb->head), a large
positive number that the new hdr_len < 0 test does not catch.
skb_header_pointer() then returns NULL for that offset, hdr_len stays at
roughly 65471, and cake_overhead() computes len = shinfo->gso_size +
hdr_len (about 66 KB) for each of segs - 1 segments.

That value is stored by cake_enqueue():

	get_cobalt_cb(skb)->adjusted_len = cake_overhead(q, skb);

and later charged in cake_advance_shaper():

	u32 len = get_cobalt_cb(skb)->adjusted_len;
	...
		u64 tin_dur = (len * b->tin_rate_ns) >> b->tin_rate_shft;

so one GSO skb can be charged many times its real size, and the u16
max_netlen/max_adjlen statistics are truncated.  On CONFIG_DEBUG_NET=y
skb_transport_header() also has a DEBUG_NET_WARN_ON_ONCE() for the unset
case.

Is the sentinel state reachable at enqueue?  qdisc_pkt_len_segs_init()
returns SKB_NOT_DROPPED_YET for it rather than dropping, after having
already stored pkt_segs, and the rx path only papers it over when
CONFIG_DEBUG_NET is unset:

net/core/dev.c:__netif_receive_skb_core() {
#if !defined(CONFIG_DEBUG_NET)
	/* We plan to no longer reset the transport header here. */
	if (!skb_transport_header_was_set(skb))
		skb_reset_transport_header(skb);
#endif
	...
}

Producers of that state include gro_cells_receive(), which calls
skb_unset_transport_header(skb), and the non-NEEDS_CSUM
else if (gso_type) branch of __virtio_net_hdr_to_skb(), which sets
gso_size without touching the transport header.  On a CONFIG_DEBUG_NET=n
kernel the rx-path reset above closes the route that could be verified,
so this may be limited to debug kernels.

[Severity: High]
This is a pre-existing issue and not something this patch introduces, but
while adding a guard here, would a segs <= 1 test be worth folding in?
cake_overhead() only special-cases one segment:

	if (segs == 1)
		return cake_calc_overhead(q, len, off);

With segs == 0, execution falls into the multi-segment arithmetic where
segs - 1 is the int -1:

	last_len = skb->len - shinfo->gso_size * (segs - 1);

	return (cake_calc_overhead(q, len, off) * (segs - 1) +
		cake_calc_overhead(q, last_len, off));

Does the u32 multiplication by -1 wrap to roughly 4.29e9 here, with
last_len becoming skb->len + gso_size?  That value reaches
cake_advance_shaper() through get_cobalt_cb(skb)->adjusted_len:

		u64 tin_dur = (len * b->tin_rate_ns) >> b->tin_rate_shft;
	...
		q->time_next_packet = ktime_add_ns(q->time_next_packet, global_dur);

which would push time_next_packet far into the future and stall dequeue
for the whole qdisc.

qdisc_pkt_segs() returns qdisc_skb_cb(skb)->pkt_segs verbatim, and
qdisc_pkt_len_segs_init() stores it before any fixup:

	qdisc_skb_cb(skb)->pkt_segs = gso_segs = shinfo->gso_segs;

gso_segs == 0 is a legitimate DODGY state, for example:

include/linux/virtio_net.h:__virtio_net_hdr_to_skb() {
			shinfo->gso_type |= SKB_GSO_DODGY;
			shinfo->gso_segs = 0;
	...
}

and the DODGY recomputation at the end of qdisc_pkt_len_segs_init() is
skipped by the unset-transport-header early return without dropping the
skb.  Note that the same virtio branch that sets gso_segs = 0 is also the
one that never sets the transport header, so both preconditions can arrive
together; as above, the CONFIG_DEBUG_NET=n rx-path reset closes the route
that could be verified, so reachability on non-debug kernels is not
established.

>  	/* + transport layer */
>  	if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 |
>  						SKB_GSO_TCPV6))) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917122153.62722-1-ndaugoing%40gmail.com

Re: [PATCH 2/2] net/sched: sch_cake: check negative transport offset in cake_overhead()

[Yuchao Zhang]

Hi,

Thank you for the detailed review. All three points raised are valid
and insightful.

1. Regarding OOB read vs shaper accounting corruption (Severity: Medium):
You are completely right. While the negative offset in [PATCH 1/2]
(net/core/dev.c) directly causes a wild pointer dereference via
raw pointer arithmetic (skb->data + hdr_len), in [PATCH 2/2]
cake_overhead() passes hdr_len to skb_header_pointer() which undoes
the unsigned conversion. The real impact is corrupted header length
accounting and inflated shaper durations, not an out-of-bounds read past
the slab allocation. I have revised the commit message to accurately
describe the issue.

2. Regarding companion guard from qdisc_pkt_len_segs_init() (Severity: Medium):
Agreed. When the transport header was never set, skb_transport_offset()
returns ~65535, which bypasses a negative offset check and inflates the
shaper duration to ~66 KB per segment. In v2, I have imported the
companion !skb_transport_header_was_set(skb) check.

3. Regarding segs == 0 underflow in multi-segment arithmetic (Severity: High):
Agreed. When segs == 0 (e.g. from DODGY GSO frames), (segs - 1) underflows
to 4294967295, causing an astronomical duration to be charged in
cake_advance_shaper() and permanently stalling dequeues. In v2, I have
updated the check to `segs <= 1`.

v2 patch has been sent in reply to this thread.

pw-bot: cr

[PATCH v2] net/sched: sch_cake: prevent shaper corruption and stall in cake_overhead()

[Yuchao Zhang]

In cake_overhead(), the header length up to the transport layer is
computed using logic borrowed from qdisc_pkt_len_segs_init():

        /* borrowed from qdisc_pkt_len_segs_init() */
        if (!skb->encapsulation)
                hdr_len = skb_transport_offset(skb);
        else
                hdr_len = skb_inner_transport_offset(skb);

However, cake_overhead() suffers from several issues:

1. When segs == 0 (e.g. from dodgy GSO packets where gso_segs is not
   recomputed), the multi-segment arithmetic underflows: (segs - 1) wraps
   to 4294967295, causing cake_calc_overhead() * (segs - 1) to produce
   an enormous length. When charged in cake_advance_shaper(),
   time_next_packet is pushed decades into the future, permanently
   stalling the CAKE dequeue queue.
   Fix this by returning cake_calc_overhead(q, len, off) for segs <= 1.

2. When the transport header was never set, skb->transport_header holds
   the sentinel value ~0U. skb_transport_offset() returns ~65535, which
   is not caught by a negative offset check. skb_header_pointer() fails,
   and hdr_len stays ~65535, charging ~66 KB per segment to the shaper.
   Fix this by mirroring qdisc_pkt_len_segs_init() and checking
   unlikely(!skb_transport_header_was_set(skb)).

3. Both skb_transport_offset() and skb_inner_transport_offset() return
   signed int, but hdr_len is declared as unsigned int. If post-enqueue
   mangling (such as BPF packet trimming via bpf_skb_net_shrink())
   produces a negative offset, hdr_len wraps to near UINT_MAX, causing
   corrupted header length accounting.
   Fix this by declaring hdr_len as int and falling back to
   cake_calc_overhead(q, len, off) if hdr_len is negative.

Fixes: a41851bea7bf ("net: account for encap headers in qdisc pkt len")
Cc: stable@vger.kernel.org
Signed-off-by: Yuchao Zhang <ndaugoing@gmail.com>
---
v2:
 - Accurately describe the impact as shaper accounting corruption / stall
   rather than OOB read past the allocation per Sashiko review.
 - Fix pre-existing high-severity integer underflow when segs == 0 by
   checking segs <= 1.
 - Import companion check !skb_transport_header_was_set(skb) from
   qdisc_pkt_len_segs_init() to prevent unset transport header sentinel
   (~0U) from inflating packet length to ~66 KB.

 net/sched/sch_cake.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index dc93267029e7..45969c1b95fc 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -1413,21 +1413,28 @@ static u32 cake_calc_overhead(struct cake_sched_data *qd, u32 len, u32 off)
 static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
 {
 	const struct skb_shared_info *shinfo = skb_shinfo(skb);
-	unsigned int hdr_len, last_len = 0;
+	unsigned int last_len = 0;
 	u32 off = skb_network_offset(skb);
 	u16 segs = qdisc_pkt_segs(skb);
 	u32 len = qdisc_pkt_len(skb);
+	int hdr_len;
 
 	WRITE_ONCE(q->avg_netoff, cake_ewma(q->avg_netoff, off << 16, 8));
 
-	if (segs == 1)
+	if (segs <= 1)
 		return cake_calc_overhead(q, len, off);
 
 	/* borrowed from qdisc_pkt_len_segs_init() */
-	if (!skb->encapsulation)
+	if (!skb->encapsulation) {
+		if (unlikely(!skb_transport_header_was_set(skb)))
+			return cake_calc_overhead(q, len, off);
 		hdr_len = skb_transport_offset(skb);
-	else
+	} else {
 		hdr_len = skb_inner_transport_offset(skb);
+	}
+
+	if (unlikely(hdr_len < 0))
+		return cake_calc_overhead(q, len, off);
 
 	/* + transport layer */
 	if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 |
-- 
2.53.0

Re: [PATCH v2] net/sched: sch_cake: prevent shaper corruption and stall in cake_overhead()

[netdev-bot+sashiko]

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

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

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] Single `Fixes: a41851bea7bf` tag plus `Cc: stable` for three
  independent defects, none of which was introduced by the tagged commit.
- [Low] The changelog justifies change 3 with "post-enqueue mangling
  (such as BPF packet trimming via bpf_skb_net_shrink())", but that…

--

net/sched: sch_cake: prevent shaper corruption and stall in cake_overhead()

This changes cake_overhead() to bail out to cake_calc_overhead() for
segs <= 1, to skip the multi-segment path when the transport header was
never set, and to treat a negative header offset as a fallback case by
making hdr_len signed. The stated goal is to avoid inflated lengths being
charged to the CAKE shaper.

> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index dc93267029e70..45969c1b95fc9 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -1413,21 +1413,28 @@ static u32 cake_calc_overhead(struct cake_sched_data *qd, u32 len, u32 off)
>  static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
>  {
>  	const struct skb_shared_info *shinfo = skb_shinfo(skb);
> -	unsigned int hdr_len, last_len = 0;
> +	unsigned int last_len = 0;
>  	u32 off = skb_network_offset(skb);
>  	u16 segs = qdisc_pkt_segs(skb);
>  	u32 len = qdisc_pkt_len(skb);
> +	int hdr_len;
>  
>  	WRITE_ONCE(q->avg_netoff, cake_ewma(q->avg_netoff, off << 16, 8));
>  
> -	if (segs == 1)
> +	if (segs <= 1)
>  		return cake_calc_overhead(q, len, off);
>  
>  	/* borrowed from qdisc_pkt_len_segs_init() */
> -	if (!skb->encapsulation)
> +	if (!skb->encapsulation) {
> +		if (unlikely(!skb_transport_header_was_set(skb)))
> +			return cake_calc_overhead(q, len, off);
>  		hdr_len = skb_transport_offset(skb);
> -	else
> +	} else {
>  		hdr_len = skb_inner_transport_offset(skb);
> +	}

[Severity: Medium]
Should the three changes here carry their own Fixes tags rather than a
single Fixes: a41851bea7bf plus Cc: stable?

None of the three defects look like they were introduced by
a41851bea7bf. git show a41851bea7bf only replaces

	hdr_len = skb_transport_offset(skb);

with the encapsulation if/else, so it neither added the segs handling nor
the missing sentinel check.

The segs early return that changes from segs == 1 to segs <= 1 came in
with c5d34f4583ea88 ("net_sched: cake: use qdisc_pkt_segs()"). Before
that commit cake_overhead() recomputed the count itself for dodgy GSO
skbs:

	segs = DIV_ROUND_UP(skb->len - hdr_len, shinfo->gso_size);

so a cached segs == 0 could not be observed in older trees.

The missing skb_transport_header_was_set() guard, on the other hand,
dates back to a729b7f0bd5bf4, where the same
hdr_len = skb_transport_offset(skb); had the identical ~0U sentinel
behaviour.

With only the a41851bea7bf tag, doesn't the segs <= 1 hunk get applied to
trees that never had the cached count, while the sentinel fix never
reaches the trees between a729b7f0bd5bf4 and a41851bea7bf that are
exposed to the ~66 KB per segment accounting the changelog describes?

Would splitting this into one logical fix per patch, each with its own
Fixes tag, make the stable ranges match the defects?

> +
> +	if (unlikely(hdr_len < 0))
> +		return cake_calc_overhead(q, len, off);
>  
>  	/* + transport layer */
>  	if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 |

[Severity: Low]
The changelog justifies this guard with:

    If post-enqueue mangling (such as BPF packet trimming via
    bpf_skb_net_shrink()) produces a negative offset, hdr_len wraps to
    near UINT_MAX

Is that ordering possible? __dev_queue_xmit() calls

	reason = qdisc_pkt_len_segs_init(skb);

near its top, and only afterwards runs sch_handle_egress() and
__dev_xmit_skb() -> cake_enqueue() -> cake_overhead(), which is the only
caller. So cake_overhead() runs during enqueue, not after it.

The two windows that do exist look like:

  - between qdisc_pkt_len_segs_init() and enqueue, i.e. tc/BPF egress in
    sch_handle_egress()
  - inside cake_enqueue() itself, since cake_classify() -> tcf_classify()
    (act_bpf, act_pedit, act_mpls, act_vlan) runs before
    get_cobalt_cb(skb)->adjusted_len = cake_overhead(q, skb)

The underlying mechanism still holds: bpf_skb_adjust_room() with
BPF_ADJ_ROOM_MAC ends up in bpf_skb_net_hdr_pop(), which adjusts only
mac_header/network_header and leaves transport_header stale, so
skb_transport_offset() becomes old_offset - len_diff and can go negative.

Could the changelog name that ordering and a concrete path instead of
"post-enqueue mangling", given the patch is tagged Cc: stable?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922084124.36858-1-ndaugoing%40gmail.com

Re: [PATCH v2] net/sched: sch_cake: prevent shaper corruption and stall in cake_overhead()

[Simon Horman]

On Fri, Sep 25, 2026 at 11:42:45AM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 1
> 
> 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] Single `Fixes: a41851bea7bf` tag plus `Cc: stable` for three
>   independent defects, none of which was introduced by the tagged commit.
> - [Low] The changelog justifies change 3 with "post-enqueue mangling
>   (such as BPF packet trimming via bpf_skb_net_shrink())", but that…
> 
> --
> 
> net/sched: sch_cake: prevent shaper corruption and stall in cake_overhead()
> 
> This changes cake_overhead() to bail out to cake_calc_overhead() for
> segs <= 1, to skip the multi-segment path when the transport header was
> never set, and to treat a negative header offset as a fallback case by
> making hdr_len signed. The stated goal is to avoid inflated lengths being
> charged to the CAKE shaper.
> 
> > diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> > index dc93267029e70..45969c1b95fc9 100644
> > --- a/net/sched/sch_cake.c
> > +++ b/net/sched/sch_cake.c
> > @@ -1413,21 +1413,28 @@ static u32 cake_calc_overhead(struct cake_sched_data *qd, u32 len, u32 off)
> >  static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
> >  {
> >  	const struct skb_shared_info *shinfo = skb_shinfo(skb);
> > -	unsigned int hdr_len, last_len = 0;
> > +	unsigned int last_len = 0;
> >  	u32 off = skb_network_offset(skb);
> >  	u16 segs = qdisc_pkt_segs(skb);
> >  	u32 len = qdisc_pkt_len(skb);
> > +	int hdr_len;
> >  
> >  	WRITE_ONCE(q->avg_netoff, cake_ewma(q->avg_netoff, off << 16, 8));
> >  
> > -	if (segs == 1)
> > +	if (segs <= 1)
> >  		return cake_calc_overhead(q, len, off);
> >  
> >  	/* borrowed from qdisc_pkt_len_segs_init() */
> > -	if (!skb->encapsulation)
> > +	if (!skb->encapsulation) {
> > +		if (unlikely(!skb_transport_header_was_set(skb)))
> > +			return cake_calc_overhead(q, len, off);
> >  		hdr_len = skb_transport_offset(skb);
> > -	else
> > +	} else {
> >  		hdr_len = skb_inner_transport_offset(skb);
> > +	}
> 
> [Severity: Medium]
> Should the three changes here carry their own Fixes tags rather than a
> single Fixes: a41851bea7bf plus Cc: stable?
> 
> None of the three defects look like they were introduced by
> a41851bea7bf. git show a41851bea7bf only replaces
> 
> 	hdr_len = skb_transport_offset(skb);
> 
> with the encapsulation if/else, so it neither added the segs handling nor
> the missing sentinel check.
> 
> The segs early return that changes from segs == 1 to segs <= 1 came in
> with c5d34f4583ea88 ("net_sched: cake: use qdisc_pkt_segs()"). Before
> that commit cake_overhead() recomputed the count itself for dodgy GSO
> skbs:
> 
> 	segs = DIV_ROUND_UP(skb->len - hdr_len, shinfo->gso_size);
> 
> so a cached segs == 0 could not be observed in older trees.
> 
> The missing skb_transport_header_was_set() guard, on the other hand,
> dates back to a729b7f0bd5bf4, where the same
> hdr_len = skb_transport_offset(skb); had the identical ~0U sentinel
> behaviour.
> 
> With only the a41851bea7bf tag, doesn't the segs <= 1 hunk get applied to
> trees that never had the cached count, while the sentinel fix never
> reaches the trees between a729b7f0bd5bf4 and a41851bea7bf that are
> exposed to the ~66 KB per segment accounting the changelog describes?

The above analysis does seem to be correct.

> 
> Would splitting this into one logical fix per patch, each with its own
> Fixes tag, make the stable ranges match the defects?

And, yes, I think one fix per patch is the right approach.

> 
> > +
> > +	if (unlikely(hdr_len < 0))
> > +		return cake_calc_overhead(q, len, off);
> >  
> >  	/* + transport layer */
> >  	if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 |
> 
> [Severity: Low]
> The changelog justifies this guard with:
> 
>     If post-enqueue mangling (such as BPF packet trimming via
>     bpf_skb_net_shrink()) produces a negative offset, hdr_len wraps to
>     near UINT_MAX
> 
> Is that ordering possible? __dev_queue_xmit() calls
> 
> 	reason = qdisc_pkt_len_segs_init(skb);
> 
> near its top, and only afterwards runs sch_handle_egress() and
> __dev_xmit_skb() -> cake_enqueue() -> cake_overhead(), which is the only
> caller. So cake_overhead() runs during enqueue, not after it.
> 
> The two windows that do exist look like:
> 
>   - between qdisc_pkt_len_segs_init() and enqueue, i.e. tc/BPF egress in
>     sch_handle_egress()
>   - inside cake_enqueue() itself, since cake_classify() -> tcf_classify()
>     (act_bpf, act_pedit, act_mpls, act_vlan) runs before
>     get_cobalt_cb(skb)->adjusted_len = cake_overhead(q, skb)
> 
> The underlying mechanism still holds: bpf_skb_adjust_room() with
> BPF_ADJ_ROOM_MAC ends up in bpf_skb_net_hdr_pop(), which adjusts only
> mac_header/network_header and leaves transport_header stale, so
> skb_transport_offset() becomes old_offset - len_diff and can go negative.
> 
> Could the changelog name that ordering and a concrete path instead of
> "post-enqueue mangling", given the patch is tagged Cc: stable?

FWIIW, I think this is less of a concern.

-- 
pw-bot: changes-requested

Re: [PATCH v2] net/sched: sch_cake: prevent shaper corruption and stall in cake_overhead()

[Simon Horman]

On Tue, Sep 22, 2026 at 04:41:24PM +0800, Yuchao Zhang wrote:
> In cake_overhead(), the header length up to the transport layer is
> computed using logic borrowed from qdisc_pkt_len_segs_init():
> 
>         /* borrowed from qdisc_pkt_len_segs_init() */
>         if (!skb->encapsulation)
>                 hdr_len = skb_transport_offset(skb);
>         else
>                 hdr_len = skb_inner_transport_offset(skb);
> 
> However, cake_overhead() suffers from several issues:
> 
> 1. When segs == 0 (e.g. from dodgy GSO packets where gso_segs is not
>    recomputed), the multi-segment arithmetic underflows: (segs - 1) wraps
>    to 4294967295, causing cake_calc_overhead() * (segs - 1) to produce
>    an enormous length. When charged in cake_advance_shaper(),
>    time_next_packet is pushed decades into the future, permanently
>    stalling the CAKE dequeue queue.
>    Fix this by returning cake_calc_overhead(q, len, off) for segs <= 1.
> 
> 2. When the transport header was never set, skb->transport_header holds
>    the sentinel value ~0U. skb_transport_offset() returns ~65535, which
>    is not caught by a negative offset check. skb_header_pointer() fails,
>    and hdr_len stays ~65535, charging ~66 KB per segment to the shaper.
>    Fix this by mirroring qdisc_pkt_len_segs_init() and checking
>    unlikely(!skb_transport_header_was_set(skb)).
> 
> 3. Both skb_transport_offset() and skb_inner_transport_offset() return
>    signed int, but hdr_len is declared as unsigned int. If post-enqueue
>    mangling (such as BPF packet trimming via bpf_skb_net_shrink())
>    produces a negative offset, hdr_len wraps to near UINT_MAX, causing
>    corrupted header length accounting.
>    Fix this by declaring hdr_len as int and falling back to
>    cake_calc_overhead(q, len, off) if hdr_len is negative.
> 
> Fixes: a41851bea7bf ("net: account for encap headers in qdisc pkt len")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yuchao Zhang <ndaugoing@gmail.com>
> ---
> v2:
>  - Accurately describe the impact as shaper accounting corruption / stall
>    rather than OOB read past the allocation per Sashiko review.
>  - Fix pre-existing high-severity integer underflow when segs == 0 by
>    checking segs <= 1.
>  - Import companion check !skb_transport_header_was_set(skb) from
>    qdisc_pkt_len_segs_init() to prevent unset transport header sentinel
>    (~0U) from inflating packet length to ~66 KB.


Please do include lore.kernel.org links to earlier versions in
the changelog. This helps tremendously with review.

Please do not post updated patch revisions as responses to earlier versions.
Rather, please start a new thread for each version.

Thanks!