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

[netdev-bot+sashiko@kernel.org]

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