[PATCH 0/2] net: fix negative transport offset handling in GSO pkt len calculation

[PATCH 0/2] net: fix negative transport offset handling in GSO pkt len calculation

[zjamg]

Hi,

This series fixes an out-of-bounds read and kernel crash in
qdisc_pkt_len_segs_init() (and similar borrowed logic in sch_cake) when
processing packets whose transport offset has become negative.

Both skb_transport_offset() and skb_inner_transport_offset() return a
signed int. However, qdisc_pkt_len_segs_init() stores the offset into an
unsigned int hdr_len.

When an encapsulated or tagged packet has undergone 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 computed offset becomes negative.

Converting this negative int to unsigned int results in a value near
UINT_MAX (e.g. 0xFFFFFFF0). Consequently:

1. pskb_may_pull(skb, hdr_len + sizeof(struct tcphdr)) computes
   0xFFFFFFF0 + 20, which wraps around in 32-bit unsigned arithmetic to 4.
   This passes the check if skb->len >= 4, completely defeating the guard.

2. th = (const struct tcphdr *)(skb->data + hdr_len) zero-extends hdr_len
   to 64 bits, producing a wild pointer ~4 GiB past skb->data.

3. __tcp_hdrlen(th) dereferences th->doff at that address, triggering an
   immediate translation fault / KASAN wild-memory-access panic.

Patch 1 declares hdr_len as int and drops packets with negative offset
via SKB_DROP_REASON_SKB_BAD_GSO.
Patch 2 fixes the corresponding logic borrowed in sch_cake.

Thanks,
zjamg

zjamg (2):
  net: fix OOB read in qdisc_pkt_len_segs_init() on negative transport
    offset
  net/sched: sch_cake: check negative transport offset in
    cake_overhead()

 net/core/dev.c       | 5 ++++-
 net/sched/sch_cake.c | 6 +++++-
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.53.0

[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