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

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

[Yuchao Zhang]

This series addresses two issues in cake_overhead() that can lead to
corrupted rate shaper accounting or long dequeue stalls:

Patch 1 fixes an integer underflow when segs == 0. When an skb with
segs == 0 (such as dodgy GSO packets where gso_segs is not recomputed)
reaches cake_overhead(), (segs - 1) wraps around to UINT32_MAX,
multiplying per-segment overhead by ~4.29 billion and returning a length
close to 4.29 GB. cake_advance_shaper() then charges that length to the
shaper, stalling the dequeue queue for tens of seconds at 1 Gbit/s, and
for minutes to hours at lower rates. This was introduced by commit
c5d34f4583ea ("net_sched: cake: use qdisc_pkt_segs()").

Patch 2 validates the transport header offset computed in cake_overhead().
When the transport header was never set, skb_transport_offset() returns
the ~0U sentinel (~65535), which inflates shaper accounting by ~66 KB
per segment. Furthermore, if preceding egress BPF filters (e.g.
sch_handle_egress()) or cake classifier actions (e.g. act_bpf trimming
headers via bpf_skb_adjust_room(BPF_ADJ_ROOM_MAC)) leave the transport
header stale (bpf_skb_net_hdr_pop() only re-syncs it when it aliased
network_header), skb_transport_offset() can become negative. Because
hdr_len was declared as unsigned int, a negative offset wraps to near
UINT_MAX. Patch 2 checks !skb_transport_header_was_set() and ensures
hdr_len >= 0. Both hunks date back to commit a729b7f0bd5b ("sch_cake:
Add overhead compensation support to the rate shaper"), so they share a
single Fixes: tag and stable range.

Changes in v3:
  - Split the v2 patch into a 2-patch series per Simon Horman and Sashiko
    AI review so each logical fix carries its own accurate Fixes: tag and
    matches proper stable tree backport ranges:
    - Patch 1 Fixes: c5d34f4583ea ("net_sched: cake: use qdisc_pkt_segs()")
    - Patch 2 Fixes: a729b7f0bd5b ("sch_cake: Add overhead
      compensation support to the rate shaper")
  - Clarify the timing and code paths where header mangling can occur
    (sch_handle_egress() and cake_classify() before cake_overhead()) rather
    than inaccurate "post-enqueue mangling" wording.
  - Link to v2: https://lore.kernel.org/netdev/20260922084124.36858-1-ndaugoing@gmail.com/

Changes in v2:
  - Accurately describe the impact as shaper accounting corruption / stall
    rather than OOB read past the allocation.
  - Fix 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.
  - Link to v1: https://lore.kernel.org/netdev/20260917122153.62722-1-ndaugoing@gmail.com/

Yuchao Zhang (2):
  net/sched: sch_cake: fix shaper stall on segs == 0 in cake_overhead()
  net/sched: sch_cake: validate transport header offset in
    cake_overhead()

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

-- 
2.53.0

[PATCH net v3 1/2] net/sched: sch_cake: fix shaper stall on segs == 0 in cake_overhead()

[Yuchao Zhang]

In cake_overhead(), packets with a single segment bypass multi-segment
overhead calculations:

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

Commit c5d34f4583ea ("net_sched: cake: use qdisc_pkt_segs()") switched
cake to retrieve the cached segmentation count via qdisc_pkt_segs(skb)
instead of calculating it locally for dodgy GSO packets. If an skb with
segs == 0 reaches cake_overhead(), it skips the segs == 1 early return
and enters the multi-segment arithmetic:

	len = shinfo->gso_size + hdr_len;
	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));

Because segs is an unsigned 16-bit integer, (segs - 1) underflows to
65535 (and is promoted to 4294967295 in the 32-bit unsigned arithmetic
above). This multiplies the per-segment overhead by UINT32_MAX, so
cake_overhead() returns a length close to 4.29 GB. cake_advance_shaper()
then charges that length to the shaper, stalling the CAKE dequeue path
for tens of seconds at 1 Gbit/s, and for minutes to hours at lower rates.

Fix this by returning early with cake_calc_overhead(q, len, off) whenever
segs <= 1.

Fixes: c5d34f4583ea ("net_sched: cake: use qdisc_pkt_segs()")
Cc: stable@vger.kernel.org
Signed-off-by: Yuchao Zhang <ndaugoing@gmail.com>
---
v3:
 - Split from v2 into a standalone patch with its own Fixes: tag
   (c5d34f4583ea) per Simon Horman and Sashiko review.
 - Link to v2: https://lore.kernel.org/netdev/20260922084124.36858-1-ndaugoing@gmail.com/
 - Link to v1: https://lore.kernel.org/netdev/20260917122153.62722-1-ndaugoing@gmail.com/

 net/sched/sch_cake.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index dc93267029e7..b0d604a7052a 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -1420,7 +1420,7 @@ static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
 
 	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() */
-- 
2.53.0

[PATCH net v3 2/2] net/sched: sch_cake: validate transport header offset 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() does not validate the computed offset:

1. When the transport header was never set, skb->transport_header holds
   the sentinel value ~0U. skb_transport_offset() returns ~65535.
   skb_header_pointer() subsequently fails, leaving hdr_len as ~65535,
   charging ~66 KB per segment to the shaper.
   Mirror qdisc_pkt_len_segs_init() by returning cake_calc_overhead()
   when unlikely(!skb_transport_header_was_set(skb)).

2. While qdisc_pkt_len_segs_init() runs at the start of __dev_queue_xmit(),
   packet headers may be adjusted before cake_overhead() is reached:
   - in sch_handle_egress() via tc/BPF egress filters;
   - inside cake_enqueue() via cake_classify() -> tcf_classify() (e.g.
     act_bpf, act_pedit, act_mpls, act_vlan).
   For example, bpf_skb_adjust_room(..., BPF_ADJ_ROOM_MAC) invokes
   bpf_skb_net_hdr_pop(), which pulls skb->data forward and re-syncs
   transport_header only when it aliased network_header, i.e. when no
   transport header had been parsed. If a transport header had been
   parsed, its offset is left where it was while skb->data moves forward,
   so skb_transport_offset() becomes old_offset - len and can turn
   negative. Since hdr_len was declared as unsigned int, a negative offset
   wraps around to near UINT_MAX, corrupting header length accounting.

Declare hdr_len as int and fall back to cake_calc_overhead(q, len, off) if
unlikely(hdr_len < 0).

Fixes: a729b7f0bd5b ("sch_cake: Add overhead compensation support to the rate shaper")
Cc: stable@vger.kernel.org
Signed-off-by: Yuchao Zhang <ndaugoing@gmail.com>
---
v3:
 - Split from v2 into a standalone patch with its own Fixes: tag
   (a729b7f0bd5b) per Simon Horman and Sashiko review.
 - Clarify header mangling ordering (sch_handle_egress() and cake_classify()
   before cake_overhead()) rather than inaccurate "post-enqueue mangling"
   wording per Sashiko review.
 - Link to v2: https://lore.kernel.org/netdev/20260922084124.36858-1-ndaugoing@gmail.com/
 - Link to v1: https://lore.kernel.org/netdev/20260917122153.62722-1-ndaugoing@gmail.com/

 net/sched/sch_cake.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index b0d604a7052a..45969c1b95fc 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));
 
@@ -1424,10 +1425,16 @@ static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
 		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 net v3 1/2] net/sched: sch_cake: fix shaper stall on segs == 0 in cake_overhead()

[Toke Høiland-Jørgensen]

Yuchao Zhang <ndaugoing@gmail.com> writes:

> In cake_overhead(), packets with a single segment bypass multi-segment
> overhead calculations:
>
> 	if (segs == 1)
> 		return cake_calc_overhead(q, len, off);
>
> Commit c5d34f4583ea ("net_sched: cake: use qdisc_pkt_segs()") switched
> cake to retrieve the cached segmentation count via qdisc_pkt_segs(skb)
> instead of calculating it locally for dodgy GSO packets. If an skb with
> segs == 0 reaches cake_overhead(), it skips the segs == 1 early return
> and enters the multi-segment arithmetic:
>
> 	len = shinfo->gso_size + hdr_len;
> 	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));
>
> Because segs is an unsigned 16-bit integer, (segs - 1) underflows to
> 65535 (and is promoted to 4294967295 in the 32-bit unsigned arithmetic
> above). This multiplies the per-segment overhead by UINT32_MAX, so
> cake_overhead() returns a length close to 4.29 GB. cake_advance_shaper()
> then charges that length to the shaper, stalling the CAKE dequeue path
> for tens of seconds at 1 Gbit/s, and for minutes to hours at lower rates.
>
> Fix this by returning early with cake_calc_overhead(q, len, off) whenever
> segs <= 1.
>
> Fixes: c5d34f4583ea ("net_sched: cake: use qdisc_pkt_segs()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yuchao Zhang <ndaugoing@gmail.com>

Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>

Re: [PATCH net v3 2/2] net/sched: sch_cake: validate transport header offset in cake_overhead()

[Toke Høiland-Jørgensen]

Yuchao Zhang <ndaugoing@gmail.com> writes:

> 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() does not validate the computed offset:
>
> 1. When the transport header was never set, skb->transport_header holds
>    the sentinel value ~0U. skb_transport_offset() returns ~65535.
>    skb_header_pointer() subsequently fails, leaving hdr_len as ~65535,
>    charging ~66 KB per segment to the shaper.
>    Mirror qdisc_pkt_len_segs_init() by returning cake_calc_overhead()
>    when unlikely(!skb_transport_header_was_set(skb)).
>
> 2. While qdisc_pkt_len_segs_init() runs at the start of __dev_queue_xmit(),
>    packet headers may be adjusted before cake_overhead() is reached:
>    - in sch_handle_egress() via tc/BPF egress filters;
>    - inside cake_enqueue() via cake_classify() -> tcf_classify() (e.g.
>      act_bpf, act_pedit, act_mpls, act_vlan).
>    For example, bpf_skb_adjust_room(..., BPF_ADJ_ROOM_MAC) invokes
>    bpf_skb_net_hdr_pop(), which pulls skb->data forward and re-syncs
>    transport_header only when it aliased network_header, i.e. when no
>    transport header had been parsed. If a transport header had been
>    parsed, its offset is left where it was while skb->data moves forward,
>    so skb_transport_offset() becomes old_offset - len and can turn
>    negative. Since hdr_len was declared as unsigned int, a negative offset
>    wraps around to near UINT_MAX, corrupting header length accounting.
>
> Declare hdr_len as int and fall back to cake_calc_overhead(q, len, off) if
> unlikely(hdr_len < 0).
>
> Fixes: a729b7f0bd5b ("sch_cake: Add overhead compensation support to the rate shaper")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yuchao Zhang <ndaugoing@gmail.com>
> ---
> v3:
>  - Split from v2 into a standalone patch with its own Fixes: tag
>    (a729b7f0bd5b) per Simon Horman and Sashiko review.
>  - Clarify header mangling ordering (sch_handle_egress() and cake_classify()
>    before cake_overhead()) rather than inaccurate "post-enqueue mangling"
>    wording per Sashiko review.
>  - Link to v2: https://lore.kernel.org/netdev/20260922084124.36858-1-ndaugoing@gmail.com/
>  - Link to v1: https://lore.kernel.org/netdev/20260917122153.62722-1-ndaugoing@gmail.com/
>
>  net/sched/sch_cake.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index b0d604a7052a..45969c1b95fc 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));
>  
> @@ -1424,10 +1425,16 @@ static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
>  		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);

We now have three identical calls to cake_calc_overhead() in the same
function; let's put these into an 'err' label at the end of the
function, and turn the early returns into 'goto err' statements.

-Toke

Re: [PATCH net v3 1/2] net/sched: sch_cake: fix shaper stall on segs == 0 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 (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] The commit message gets the mechanism and impact of the segs
  == 0 bug wrong.
- [Low] The commit message never says how an skb with qdisc_pkt_segs(skb)
  == 0 reaches the multi-segment arithmetic in cake_overhead().

Pre-existing issues:
- [Low] cake_overhead() says its header-length logic is "borrowed from
  qdisc_pkt_len_segs_init()", but it lacks that function's checks.

--

net/sched: sch_cake: fix shaper stall on segs == 0 in cake_overhead()

This patch changes the single-segment early return in cake_overhead() from
segs == 1 to segs <= 1. The goal is to stop skbs with a cached segment count
of 0 from reaching the multi-segment overhead arithmetic.

> Because segs is an unsigned 16-bit integer, (segs - 1) underflows to
> 65535 (and is promoted to 4294967295 in the 32-bit unsigned arithmetic
> above). This multiplies the per-segment overhead by UINT32_MAX, so
> cake_overhead() returns a length close to 4.29 GB.

[Severity: Medium]
Is this description of the arithmetic accurate? segs is a u16, but it is
promoted to int before the subtraction, so segs - 1 is -1 rather than 65535.

The last-segment term also wraps when segs == 0:

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

This gives skb->len + gso_size. Writing F() for cake_calc_overhead(), the
old return value reduces to:

    F(skb->len + gso_size) - F(gso_size + hdr_len)    (mod 2^32)

F() never decreases as its argument grows. So when hdr_len <= skb->len,
wouldn't the old code charge roughly skb->len - hdr_len rather than
something close to 4.29 GB?

The large wrap seems to need hdr_len > skb->len. At this SHA that looks
possible only when the transport header is unset. In that case
qdisc_pkt_len_segs_init() returns early and leaves pkt_segs == gso_segs:

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;
    ...
}

cake_overhead() then computes hdr_len from the unset transport offset,
which is around 0xFFFF. The next patch in the series, "net/sched: sch_cake:
validate transport header offset in cake_overhead()", targets that same
condition.

The 4.29 GB figure is used to justify Cc: stable. Could the commit message
describe the arithmetic correctly and state the condition needed for the
stall?

> Commit c5d34f4583ea ("net_sched: cake: use qdisc_pkt_segs()") switched
> cake to retrieve the cached segmentation count via qdisc_pkt_segs(skb)
> instead of calculating it locally for dodgy GSO packets. If an skb with
> segs == 0 reaches cake_overhead(), it skips the segs == 1 early return
> and enters the multi-segment arithmetic:

[Severity: Low]
How does an skb with qdisc_pkt_segs(skb) == 0 get here? For every
SKB_GSO_DODGY skb that has a transport header, qdisc_pkt_len_segs_init()
already recomputes pkt_segs, and it drops the skb when payload <= 0:

net/core/dev.c:qdisc_pkt_len_segs_init() {
    ...
	if (unlikely(shinfo->gso_type & SKB_GSO_DODGY)) {
		int payload = skb->len - hdr_len;

		/* Malicious packet. */
		if (payload <= 0)
			return SKB_DROP_REASON_SKB_BAD_GSO;
		gso_segs = DIV_ROUND_UP(payload, shinfo->gso_size);
		shinfo->gso_segs = gso_segs;
		qdisc_skb_cb(skb)->pkt_segs = gso_segs;
	}
    ...
}

pkt_segs == 0 seems to survive in only two cases:

  - the early return for a non-encapsulated GSO skb with no transport
    header set
  - a non-DODGY GSO skb whose gso_segs was left at 0

The in-tree virtio_net_hdr producers set DODGY. On af_packet they also set
a transport header.

In the second case, with a valid transport header, the new early return
charges F(qdisc_pkt_len()), which is F(skb->len - hdr_len). That is about
what the old wrapped path charged.

Could the commit message name the producer or path that leads here? That
would let the reproducer and the "tens of seconds" stall be checked, and
would help stable maintainers judge exposure.

> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index dc93267029e70..b0d604a7052a9 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -1420,7 +1420,7 @@ static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
>  
>  	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() */

[Severity: Low]
This is a pre-existing issue, not one introduced by this patch. The code
after this comment in cake_overhead() lacks several of the checks in
qdisc_pkt_len_segs_init():

net/sched/sch_cake.c:cake_overhead() {
    ...
	/* borrowed from qdisc_pkt_len_segs_init() */
	if (!skb->encapsulation)
		hdr_len = skb_transport_offset(skb);
	else
		hdr_len = skb_inner_transport_offset(skb);
    ...
}

Compared with qdisc_pkt_len_segs_init(), it is missing:

  - a skb_transport_header_was_set() guard
  - a check that __tcp_hdrlen(th) >= sizeof(struct tcphdr)
  - a pskb_may_pull() of the full header

It also adds sizeof(struct udphdr) for every non-TCP GSO type, not just
SKB_GSO_UDP_L4.

Of these, only the missing transport header guard can cause a large
overcharge. With an unset header, hdr_len is around 64 KiB, and each
segment is charged tens of KB when segs >= 2. The follow-up patch
"net/sched: sch_cake: validate transport header offset in cake_overhead()"
adds that guard and a hdr_len < 0 check.

After the full series, is the remaining difference from the core helper
intended? What is left is small misaccounting: the TCP header length term is
bounded at 60 bytes, and the extra UDP header is 8 bytes. This code dates
back to a729b7f0bd5b.

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

Re: [PATCH net v3 2/2] net/sched: sch_cake: validate transport header offset in cake_overhead()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · Medium: 1 (1) · 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] The new `if (unlikely(hdr_len < 0))` check in cake_overhead()
  only catches stale transport offsets that have gone negative.

Pre-existing issues:
- [High] The same negative transport offset stored in an unsigned hdr_len
  also exists in qdisc_pkt_len_segs_init() (net/core/dev.c).
- [Medium] In cake_overhead(), `last_len = skb->len - shinfo->gso_size *
  (segs - 1)` can wrap as a u32 after an egress or classifier BPF header…

--

net/sched: sch_cake: validate transport header offset in cake_overhead()

When the transport header was never set, cake_overhead() now falls back
to cake_calc_overhead(), as qdisc_pkt_len_segs_init() does. The patch
also changes hdr_len to int and uses the same fallback when the transport
offset is negative. A negative offset can happen after egress or
classifier header mangling, for example bpf_skb_adjust_room() in
BPF_ADJ_ROOM_MAC mode.

> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index b0d604a7052a9..45969c1b95fc9 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c

[ ... ]

> @@ -1424,10 +1425,16 @@ static u32 cake_overhead(struct cake_sched_data *q, const struct sk_buff *skb)
>  		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);

[Severity: Medium]
Is a lower bound of 0 enough here? The same bpf_skb_net_hdr_pop()
mechanism can also leave a transport offset that is non-negative but
stale:

net/core/filter.c:bpf_skb_net_hdr_pop() {
    ...
		skb->mac_header += len;
		skb->network_header += len;
		if (trans_same)
			skb->transport_header = skb->network_header;
    ...
}

Take a BPF_ADJ_ROOM_MAC shrink where len_diff is not larger than the old
transport offset. The offset stays >= 0, but it points before the new
network header, into the moved MAC header.

GRE decap is one example. gre_build_header() calls
skb_reset_transport_header() at the GRE header, so the transport offset
is 14 + 20 = 34. An egress tc BPF program then does:

    bpf_skb_adjust_room(skb, -24, BPF_ADJ_ROOM_MAC,
                        BPF_F_ADJ_ROOM_DECAP_L4_GRE |
                        BPF_F_ADJ_ROOM_DECAP_L3_IPV4);

bpf_skb_net_shrink() clears the GRE gso_type bits and
skb->encapsulation. In cake_overhead(), hdr_len then becomes
34 - 24 = 10. That is below skb_network_offset() (14), so this check does
not fire.

skb_header_pointer(skb, 10, sizeof(_tcphdr), ...) then reads Ethernet
and IPv4 bytes. doff comes from the IPv4 TTL byte (TTL 64 gives 16
bytes), so each segment is charged 26 bytes of headers instead of 54.

A plain IPv4/TCP GSO skb shrunk by 30 in MAC mode behaves the same way
(34 - 30 = 4).

Would a check such as hdr_len < (int)off catch this case too?

[Severity: High]
This isn't a bug introduced by this patch, but does qdisc_pkt_len_segs_init()
in net/core/dev.c have the same negative offset problem? There it would
lead to a wild pointer dereference.

The commit message says:

    While qdisc_pkt_len_segs_init() runs at the start of __dev_queue_xmit(),
    packet headers may be adjusted before cake_overhead() is reached

This suggests the core helper only ever sees skbs that have not been
mangled. However, an egress tc BPF program can call bpf_skb_adjust_room()
with a negative len_diff in BPF_ADJ_ROOM_MAC mode and then return
bpf_redirect():

skb_do_redirect()
  __bpf_redirect_common()
    __bpf_tx_skb()
      dev_queue_xmit()
        __dev_queue_xmit()
          qdisc_pkt_len_segs_init()

This runs qdisc_pkt_len_segs_init() a second time on the mangled skb,
and nothing resets the transport header in between. The tc ingress path,
for example after an ingress redirect, is another entry point.

Two examples of the resulting offset:

- A plain Ethernet/IPv4/TCP GSO skb shrunk by 40 has a transport offset
  of 34 - 40 = -6.
- A VXLAN decap with BPF_F_ADJ_ROOM_DECAP_L4_UDP gives 34 - 50 = -16.
  This decap also clears skb->encapsulation.

In qdisc_pkt_len_segs_init(), hdr_len is still unsigned int:

net/core/dev.c:qdisc_pkt_len_segs_init() {
    ...
	unsigned int hdr_len, tlen;
    ...
		hdr_len = skb_transport_offset(skb);
    ...
		if (!pskb_may_pull(skb, hdr_len + sizeof(struct tcphdr)))
			return SKB_DROP_REASON_SKB_BAD_GSO;

		th = (const struct tcphdr *)(skb->data + hdr_len);
		tlen = __tcp_hdrlen(th);
    ...
}

With hdr_len = 0xFFFFFFFA, hdr_len + sizeof(struct tcphdr) is the size_t
value 0x10000000E. pskb_may_pull() takes an unsigned int len, so the
value is truncated to 14 and the check passes.

On 64-bit, skb->data + hdr_len then points about 4 GB past skb->data,
and __tcp_hdrlen() dereferences it. Can this oops, or read arbitrary
memory that then drives the drop and segment count decisions?

In cake_overhead(), the int offset argument of skb_header_pointer()
limits the damage, but the core helper has no such protection. The
dereference came in with commit 7fb4c19670110f ("net: pull headers in
qdisc_pkt_len_segs_init()"). Reaching it requires a privileged
(CAP_BPF/CAP_NET_ADMIN) tc BPF program.

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

[Severity: Medium]
This is a pre-existing issue, but can last_len wrap further down in
cake_overhead() after the same kind of egress or classifier BPF shrink?

net/sched/sch_cake.c:cake_overhead() {
    ...
	u16 segs = qdisc_pkt_segs(skb);
    ...
	len = shinfo->gso_size + hdr_len;
	last_len = skb->len - shinfo->gso_size * (segs - 1);
    ...
}

segs comes from qdisc_skb_cb(skb)->pkt_segs. That value is set in
__dev_queue_xmit() before sch_handle_egress() or cake_classify() run.
bpf_skb_net_shrink() then enlarges gso_size without refreshing pkt_segs:

net/core/filter.c:bpf_skb_net_shrink() {
    ...
		if (!(flags & BPF_F_ADJ_ROOM_FIXED_GSO))
			skb_increase_gso_size(shinfo, len_diff);
    ...
		shinfo->gso_type |= SKB_GSO_DODGY;
		shinfo->gso_segs = 0;
    ...
}

As a result, last_len = old_last_len - len_diff * segs, which wraps when
len_diff * segs is larger than old_last_len.

For example, take IPv4 with 20 bytes of options, gso_size 1000,
skb->len 60075 and 61 cached segments. Removing the options in NET mode
gives 60055 - 1020 * 60 = -1145.

cake_enqueue() stores the result in get_cobalt_cb(skb)->adjusted_len.
Inside cake_calc_overhead() this causes three problems:

- A truncated garbage value is stored in the u16 max_netlen stat.
- The (s32) cast clamps the last segment's charge to rate_mpu.
- segs - 1 segments are charged at the enlarged gso_size, using the
  stale segment count.

The new hdr_len checks don't cover this case.

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