Re: [PATCH net v2 1/3] net: make room for the tunnel header in skb_segment_list()

[netdev-bot+sashiko]

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

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

- [Medium] The commit message describes the bug only as an IPIP case that
  ends in skb_under_panic().

--

net: make room for the tunnel header in skb_segment_list()

This patch adds a skb_cow_head() call for each frag_list member in
skb_segment_list(). The call happens before the member is pushed back to
the outer MAC header and the tunnel headers are copied in front of it.
The stated goal is to avoid skb_under_panic() when a fraglist GRO packet
is forwarded into an IPIP tunnel and segmented in software.

> skb_segment_list() pushes each frag_list member back to the outer MAC
> header and copies the tunnel headers in front of it, but never checks
> that the member has the headroom. A fraglist GRO packet forwarded into
> an IPIP tunnel and segmented in software hits skb_under_panic() when
> the members were received with less headroom than the tunnel headers
> need.

[Severity: Medium]

Does this description cover the whole impact? The same missing headroom
check seems to also allow a silent out-of-bounds write before
nskb->head on the UDP tunnel GSO path (VXLAN, GENEVE and similar). On
that path there is no skb_under_panic() at all.

On that path, __skb_udp_tunnel_segment() resets the mac header to the
inner header before segmenting:

net/ipv4/udp_offload.c:__skb_udp_tunnel_segment() {
    ...
	__skb_pull(skb, tnl_hlen);
	skb_reset_mac_header(skb);
    ...
}

After that, skb_tnl_header_len() returns the full outer header length
inside skb_segment_list(). That is 50 bytes for VXLAN over IPv4 and 70
over IPv6. Meanwhile, offset is only the inner MAC length.

Before this patch, skb_push(nskb, -skb_network_offset(nskb) + offset)
only moved each member back to its own received inner MAC header, so it
never triggered skb_under_panic(). The copy that follows is a plain
memcpy with no bounds check:

net/core/skbuff.c:skb_segment_list() {
    ...
		skb_copy_from_linear_data_offset(skb, -tnl_hlen,
						 nskb->data - tnl_hlen,
						 offset + tnl_hlen);
    ...
}

If a member has less headroom than tnl_hlen, this copy writes before
nskb->head. For example, a napi_alloc_skb() member with NET_SKB_PAD (64)
headroom, forwarded into VXLAN over IPv6 (70 bytes), would get 6 bytes
written before head.

vxlan_setup() advertises NETIF_F_GSO_SOFTWARE, which includes
NETIF_F_GSO_FRAGLIST. So fraglist GRO packets can reach this path
without being segmented first.

The new skb_cow_head(nskb, offset + tnl_hlen - skb_network_offset(nskb))
reserves the push length plus tnl_hlen. The code change therefore seems
to fix this case too.

Could the commit message describe the UDP tunnel case and the heap
corruption, so that stable and security triage don't treat this as a
crash-only issue?

It might also be worth saying that skb_cow_head() now unclones members
whose header is cloned before the tunnel headers are written into them.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011162957.1328474-1-julius%40bairaktaris.de