Re: [PATCH 1/2] net: gso: validate TCP headers before segment length checks

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Over-strict ordering check rejects valid decapsulated TCP GSO
  traffic.
- [Medium] skb_gso_tcp_header_valid() ends with `return sizeof(struct
  tcphdr) <= tail - transport;`, where tail is the linear tail.

--

net: gso: validate TCP headers before segment length checks

This adds skb_gso_tcp_header_valid(). The helper checks TCP transport
header ordering, linear bounds and fixed header presence before
skb_gso_validate_network_len() and skb_gso_validate_mac_len() compute
segment lengths. It targets a KASAN out-of-bounds read of tcp->doff when
malformed TUN-supplied TCP GSO skbs reach TBF or police.

> diff --git a/net/core/gso.c b/net/core/gso.c
> index bcd156372f4df..7c76f721fe24e 100644
> --- a/net/core/gso.c
> +++ b/net/core/gso.c
> @@ -240,6 +240,29 @@ static inline bool skb_gso_size_check(const struct sk_buff *skb,
>  	return true;
>  }
>  
> +/* TCP segment length reads doff, so validate its header offsets first. */
> +static bool skb_gso_tcp_header_valid(const struct sk_buff *skb)
> +{
> +	unsigned int transport = skb->transport_header;
> +	unsigned int tail = skb_tail_pointer(skb) - skb->head;
> +
> +	if (!skb_is_gso_tcp(skb))
> +		return true;
> +
> +	if (!skb_transport_header_was_set(skb) ||
> +	    transport <= skb->network_header || transport > tail)
> +		return false;

[Severity: High]
Can this ordering check reject valid decapsulated TCP GSO skbs on the RX
path?

After UDP tunnel decapsulation, a valid TCP GSO skb can keep a stale
transport_header. This happens when the tunnel device skips GRO, either
because NETIF_F_GRO is off or because an XDP program is attached, so
netif_elide_gro() is true:

ip_rcv_core()
    skb->transport_header = skb->network_header + iph->ihl*4;  <- outer UDP

vxlan_rcv()->__iptunnel_pull_header()->iptunnel_pull_offloads()
    clears the encap GSO bits and encapsulation,
    leaves transport_header alone

gro_cells_receive()
    if (!gcells->cells || skb_cloned(skb) || netif_elide_gro(dev)) {
        res = netif_rx(skb);
    so inet_gro_receive() never refreshes the transport header

__netif_receive_skb_core()
    skb_reset_network_header(skb);
    ...
    if (!skb_transport_header_was_set(skb))
        skb_reset_transport_header(skb);

That leaves gso_type=TCPV4 and encapsulation=0, with transport_header still
pointing at the outer UDP header. That offset comes before both the inner
mac header and the inner network header.

In that case transport <= skb->network_header is true. The extra
skb->transport_header <= skb->mac_header test in skb_gso_validate_mac_len()
is also true. So both validators return false.

Callers read false as "segments do not fit". tcf_police_mtu_check() runs
this for every policed GSO packet:

    if (skb_is_gso(skb))
        return skb_gso_validate_mac_len(skb, limit);

tcfp_mtu defaults to ~0 when no mtu is given. Before this patch, the
unsigned wrapped arithmetic happened to fit and these packets passed.

Would a police action on the tunnel device's ingress now send every such
GSO packet to the exceed action?

tbf_enqueue() would likewise drop the packet with qdisc_drop() instead of
calling tbf_segment(). skb_tunnel_check_pmtu() would treat it as a PMTU
violation, which produces ICMP FRAG_NEEDED on bridge ports.

This patch is tagged for stable. Would it be safer to reject only an unset
transport header or offsets outside the linear head, or to read doff with
skb_header_pointer(), rather than rely on header ordering?

> +
> +	if (skb->encapsulation) {
> +		transport = skb->inner_transport_header;
> +		if (transport <= skb->inner_network_header ||
> +		    transport < skb->transport_header || transport > tail)
> +			return false;
> +	}
> +
> +	return sizeof(struct tcphdr) <= tail - transport;

[Severity: Medium]
Does this reject TCP GSO skbs whose headers are valid but whose fixed TCP
header is partly in paged frags?

tail here is the linear tail, so the whole 20-byte tcphdr has to be in the
linear area.

Take the IFF_TAP path of tun_get_user() without
VIRTIO_NET_HDR_F_NEEDS_CSUM, which is the producer the commit message
describes. __virtio_net_hdr_to_skb() has no network header yet, so it takes
this branch:

    } else if (gso_type) {
        p_off = nh_min_len + thlen;
        if (!pskb_may_pull(skb, p_off))
            return -EINVAL;
    }

That pulls only 40 bytes counted from the MAC header. With a small hdr_len
and a packet larger than a page, tun_alloc_skb() leaves the rest in frags.
After eth_type_trans(), skb_probe_transport_header() sets transport to
network + 20 for a valid IPv4 header. Only 6 TCP bytes are then linear.

Before this patch, such a packet was forwarded, or segmented by
skb_gso_segment(), which pulls headers properly. tcp_hdrlen() did read
stale tailroom bytes on that path.

Now both validators report the packet as too big. For example, in
ip_exceeds_mtu():

    if (skb_is_gso(skb) && skb_gso_validate_network_len(skb, mtu))
        return false;

the packet gets ICMP_FRAG_NEEDED and is dropped. ip6_pkt_too_big(),
skb_tunnel_check_pmtu(), tbf_enqueue(), execute_check_pkt_len() and
tcf_police_act() get the same false result.

The commit message doesn't mention that header linearity failures are now
reported as MTU violations.

The validators take a const skb and cannot pull. Would reading doff through
skb_header_pointer() avoid the out-of-bounds read without rejecting
non-linear but valid headers?

> +}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927163117.746432-2-bestswngs%40gmail.com