Re: [PATCH net] net: reject a negative TCP header offset in qdisc_pkt_len_segs_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: negative hdr_len in qdisc_pkt_len_segs_init() only checked for TCP
--

commit dddca5a283a51c0b9bd1d1aef355eda12933179b
Author: Alexei Starovoitov <ast@kernel.org>

net: reject a negative TCP header offset in qdisc_pkt_len_segs_init()

This commit adds a negative offset check on hdr_len inside the TCP GSO
branch of qdisc_pkt_len_segs_init() to prevent an out-of-bounds read when
a stale transport header points in front of skb->data.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index f32353b0467cd..af948e4424035 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4320,7 +4320,9 @@ static enum skb_drop_reason qdisc_pkt_len_segs_init(struct sk_buff *skb)
>  	if (likely(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))) {
>  		const struct tcphdr *th;
>  
> -		if (!pskb_may_pull(skb, hdr_len + sizeof(struct tcphdr)))
> +		/* A header in front of skb->data gives a negative offset. */
> +		if ((int)hdr_len < 0 ||
> +		    !pskb_may_pull(skb, hdr_len + sizeof(struct tcphdr)))
>  			return SKB_DROP_REASON_SKB_BAD_GSO;

[Severity: High]
Should the (int)hdr_len < 0 check be placed immediately after hdr_len is
computed rather than only inside the SKB_GSO_TCPV4 | SKB_GSO_TCPV6 branch?

In net/core/dev.c:qdisc_pkt_len_segs_init(), leaving non-TCP GSO types
unguarded when skb_transport_offset(skb) or skb_inner_transport_offset(skb)
is negative appears to leave three issues:

First, for SKB_GSO_UDP_L4 packets:

	} else if (shinfo->gso_type & SKB_GSO_UDP_L4) {
		if (!pskb_may_pull(skb, hdr_len + sizeof(struct udphdr)))
			return SKB_DROP_REASON_SKB_BAD_GSO;
		hdr_len += sizeof(struct udphdr);
	}

When (int)hdr_len < -8, hdr_len + sizeof(struct udphdr) wraps to an
unsigned int in [0x80000008, 0xffffffff] (> INT_MAX), which triggers
DEBUG_NET_WARN_ON_ONCE(len > INT_MAX) in
include/linux/skbuff.h:pskb_may_pull_reason() before returning
SKB_DROP_REASON_SKB_BAD_GSO.

Second, when -8 <= (int)hdr_len < 0 on an SKB_GSO_UDP_L4 packet,
hdr_len + sizeof(struct udphdr) wraps to 0..7. pskb_may_pull() succeeds
without pulling the UDP header into skb->head, hdr_len wraps to 0..7, and
qdisc_pkt_len_segs_init() returns SKB_NOT_DROPPED_YET. If the packet is
then transmitted on a NETIF_F_GSO_UDP_L4 device whose driver uses the TSO
helpers in net/core/tso.c (such as bnxt_sw_udp_gso_xmit(),
enetc_map_tx_tso_buffs(), or otx2_sq_append_tso()), tso_build_hdr()
advances hdr by the negative transport offset:

	hdr += skb_transport_offset(skb);
	if (tso->tlen != sizeof(struct udphdr)) {
		...
	} else {
		struct udphdr *uh = (struct udphdr *)hdr;

		/* size is after segmentation. */
		udp_set_len_short(uh, sizeof(*uh) + size);
	}

Can this write uh->len (and uh->check in bnxt_sw_udp_gso_xmit() and
enetc_tso_hdr_csum()) before the start of the header buffer when
skb_transport_offset(skb) < -4?

Third, for other GSO types (such as SKB_GSO_FRAGLIST, SKB_GSO_UDP,
SKB_GSO_SCTP, or SKB_GSO_ESP), hdr_len remains 2^32 - |offset| at the end
of qdisc_pkt_len_segs_init():

	qdisc_skb_cb(skb)->pkt_len += (gso_segs - 1) * hdr_len;

When (gso_segs - 1) * |offset| > skb->len, can qdisc_skb_cb(skb)->pkt_len
underflow in 32-bit unsigned arithmetic to ~4.29 GiB (exceeding
QDISC_PKT_LEN_MAX), causing deficit-based schedulers such as drr_dequeue()
and ets_dequeue() to spin in their deficit-refill loops?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101194433.2637113-1-alexei.starovoitov@gmail.com?part=1