Re: [PATCH net v2] net: gso: validate transport header before computing segment length

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · 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] This patch makes legitimate encapsulated TCP fraglist GRO
  packets fail the GSO length checks.
- [High] net: gso: skb_gso_transport_header_ok() drops XFRM tunnel TCP
  GSO packets
- [Medium] net: skb_inner_transport_header_was_set() returns true for ~0U
  sentinel

--

net: gso: validate transport header before computing segment length

This patch adds skb_gso_transport_header_ok(). With it, the TCP GSO
length validators reject an skb when its transport header (or inner
transport header) is unset or lies outside the linear data.
skb_gso_network_seglen() and skb_gso_mac_seglen() now return a negative
errno, and skb_gso_validate_network_len() and skb_gso_validate_mac_len()
turn that into false.

> diff --git a/net/core/gso.c b/net/core/gso.c
> index e96ef63500648..874e4c8b1f1bb 100644
> --- a/net/core/gso.c
> +++ b/net/core/gso.c
> @@ -131,6 +131,35 @@ struct sk_buff *__skb_gso_segment(struct sk_buff *skb,
>  }
>  EXPORT_SYMBOL(__skb_gso_segment);
>  
> +/*
> + * TSO processing (but not UDP/SCTP GSO, which uses fixed header
> + * sizes) reads the TCP header as specified by
> + * skb->transport_header/inner_transport_header.  Make sure it is
> + * there to read.
> + */
> +static bool skb_gso_transport_header_ok(const struct sk_buff *skb)
> +{
> +	const struct skb_shared_info *shinfo = skb_shinfo(skb);
> +	int thoff;
> +
> +	if (unlikely(!(shinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))))
> +		return true;
> +
> +	if (skb->encapsulation) {
> +		if (!skb_inner_transport_header_was_set(skb))
> +			return false;

[Severity: High]
Does this break TCP GSO over IPsec tunnel and BEET mode SAs that use
ESP offload?

xfrm_dev_offload_ok() succeeds for software ESP GSO via
esp4_offload/esp6_offload and also for XFRM_DEV_OFFLOAD_CRYPTO. When
it does, xfrm_output() sets skb->encapsulation on a GSO skb before the
outer mode has run:

net/xfrm/xfrm_output.c:xfrm_output() {
	...
		skb->encapsulation = 1;

		if (skb_is_gso(skb)) {
			...
			skb_shinfo(skb)->gso_type |= SKB_GSO_ESP;
			goto out;
		}
	...
}

For a plain TCP skb, inner_protocol is 0, so xfrm_output_gso() is not
taken. The skb goes through xfrm_output2(), xfrm_output_resume()
and xfrm_output_one() to xfrm4_prepare_output(). That calls
xfrm_inner_extract_output(), which reaches xfrm4_tunnel_check_size()
and skb_gso_validate_network_len(). Only after that does
xfrm4_tunnel_encap_add() call skb_set_inner_transport_header(). In
BEET mode, xfrm4_beet_encap_add() does not set it at all.
xfrm6_prepare_output() and xfrm6_tunnel_check_size() follow the same
order.

So the skb seen by skb_gso_transport_header_ok() has encapsulation
set, SKB_GSO_TCPV4 or SKB_GSO_TCPV6 in gso_type and
inner_transport_header == 0. Before this patch,
skb_gso_network_seglen() returned a bogus but small value that passed
the MTU check. The unsigned subtraction of transport_header in
skb_gso_transport_seglen() cancelled against the transport_header -
network_header term. Now the check fails every time.

xfrm4_tunnel_check_size() (with DF set) and xfrm6_tunnel_check_size()
then return -EMSGSIZE and report an MTU error. For local sockets that
goes through xfrm_local_error(), otherwise through ICMP_FRAG_NEEDED or
ICMPV6_PKT_TOOBIG. The reported MTU is one the flow already fits in.
As far as I can tell, every TCP GSO packet on such an SA is dropped.
That makes this a regression for what looks like a common IPsec
setup, and the Fixes: tag means it is likely to be backported to
stable.

Should xfrm set the inner headers before xfrm_inner_extract_output()
runs? Or should this check key off the tunnel gso_type bits rather
than skb->encapsulation alone? Falling back to the outer transport
header when inner_transport_header is unset might be another option,
though skb_gso_transport_seglen() would need the same treatment.

[Severity: Medium]
Does this treat an inner transport header that was copied from an
unset outer one as set?

skb_inner_network_header_was_set() can get away with a > 0 test
because network_header starts at 0 and is never set to ~0U. That does
not hold for transport_header. __finalize_skb_around() initialises it
to (typeof(skb->transport_header))~0U and skb_unset_transport_header()
puts it back there. skb_reset_inner_headers() copies it verbatim:

include/linux/skbuff.h:skb_reset_inner_headers() {
	...
	skb->inner_transport_header = skb->transport_header;
}

bpf_skb_net_grow() does the same. iptunnel_handle_offloads(),
bpf_lwt_push_ip_encap(), bpf_push_seg6_encap() and gtp_dev_xmit() all
go through skb_reset_inner_headers(). An skb whose transport header
was never set, or was unset as gro_cells_receive() does, therefore
ends up with skb->encapsulation = 1 and inner_transport_header ==
0xffff.

For such an skb, skb_inner_transport_header_was_set() returns true.
skb_gso_transport_header_ok() then only rejects it through the bounds
test on thoff = 0xffff - skb_headroom(skb). Consider a large linear
GSO skb where
skb_headroom(skb) + skb_headlen(skb) >= 65535 + sizeof(struct tcphdr),
for example one that bpf_skb_net_grow() has just linearized. It passes
that test, and inner_tcp_hdrlen() in skb_gso_transport_seglen() then
reads payload at skb->head + 0xffff as a TCP header. The access stays
within the linear area, but the segment length is then computed from
arbitrary payload bytes. That is what the unset check is meant to
prevent.

The helper lives in include/linux/skbuff.h, so other callers may pick
it up with the same blind spot. Should it also treat
(typeof(skb->inner_transport_header))~0U as unset, the way
skb_transport_header_was_set() does for the outer header?

[Severity: High]
Can this reject legitimate TCP fraglist GRO packets that arrive inside a
UDP tunnel?

udp_gro_complete() sets skb->encapsulation before it calls the tunnel's
inner gro_complete. It also clears encap_mark so that inner frag_list
completion can run:

net/ipv4/udp_offload.c:udp_gro_complete() {
    ...
		NAPI_GRO_CB(skb)->encap_mark = 0;
		...
		skb->encapsulation = 1;
		err = udp_sk(sk)->gro_complete(sk, skb,
				nhoff + sizeof(struct udphdr));
    ...
}

tcp4_check_fraglist_gro() sets is_flist = !sk && !skb_is_gso(skb) and
never looks at encapsulation. Any inner flow without a local socket
qualifies, for example forwarded traffic or containers behind a VXLAN
bridge. tcp4_gro_complete() then returns early:

net/ipv4/tcp_offload.c:tcp4_gro_complete() {
    ...
	if (unlikely(NAPI_GRO_CB(skb)->is_flist)) {
		skb_shinfo(skb)->gso_type |= SKB_GSO_FRAGLIST | SKB_GSO_TCPV4;
		skb_shinfo(skb)->gso_segs = NAPI_GRO_CB(skb)->count;

		__skb_incr_checksum_unnecessary(skb);

		return 0;
	}
    ...
	tcp_gro_complete(skb);
    ...
}

tcp_gro_complete() is the only TCP GRO code that sets the inner
transport header:

net/ipv4/tcp_offload.c:tcp_gro_complete() {
    ...
	if (skb->encapsulation)
		skb->inner_transport_header = skb->transport_header;
    ...
}

tcp6_gro_complete() has the same flist early return.

The resulting skb has encapsulation set, a gso_type that includes
SKB_GSO_UDP_TUNNEL, SKB_GSO_FRAGLIST and SKB_GSO_TCPV4, and
inner_transport_header == 0.

Before this patch, skb_gso_transport_seglen() read the TCP data offset
relative to skb->head, which is inside the headroom. The unsigned
arithmetic gave a value that passed the size check. After this patch,
skb_gso_validate_network_len() and skb_gso_validate_mac_len() always
return false for these skbs.

Some effects of that:

tcf_police_mtu_check() always takes the exceed action, even with the
default tcfp_mtu of ~0.

ip_exceeds_mtu() returns true, so ip_forward() sends ICMP_FRAG_NEEDED
with the route MTU and drops DF transit tunnel traffic. GRO applies to
that traffic when it matches a wildcard-bound local tunnel socket. The
sender already honours that MTU, so the flow stays blackholed.

ip_finish_output_gso() falls back to software segmentation and
fragmentation.

Triggering this needs rx-gro-list enabled on the NIC and a UDP tunnel
socket with gro_receive, but the packets themselves come from remote
traffic. The patch also has a Fixes: tag, so it will likely be
backported to stable kernels that have TCP fraglist GRO.

Would it make sense to set inner_transport_header in the flist branch of
tcp4_gro_complete() and tcp6_gro_complete() when skb->encapsulation is
set? Another option is to skip the encapsulated check here for
SKB_GSO_FRAGLIST skbs.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/a53dbcdf068b3813689cf2359949578f627f9edd.1792410745.git.mst%40redhat.com