Re: [PATCH net v3] net: preserve skb header offsets during headroom changes

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 · 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] The fix is incomplete.
- [Medium] skb_header_offsets_fit() checks only some of the head-relative
  offsets that skb_headers_offset_update() moves.
- [Medium] mpls: mpls_forward() checks skb_headroom() after reading
  wrapped mpls_hdr()

--

net: preserve skb header offsets during headroom changes

This patch adds skb_header_offsets_fit() so that pskb_expand_head() and
skb_copy_expand() reject headroom growth that would wrap the 16-bit skb
header offsets. It also adds headroom bounds checks to skb_mpls_pop(),
mpls_forward() and mpls_xmit(), so a large-headroom device such as netkit
can no longer push network_header past U16_MAX on those paths.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 1ac1e327ac4c6..bc836adafecd9 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2130,6 +2130,28 @@ struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
>  }
>  EXPORT_SYMBOL(skb_clone);
>  
> +static bool skb_header_offsets_fit(const struct sk_buff *skb,
> +				   unsigned int nhead)
> +{
> +	if (!nhead)
> +		return true;
> +	if (nhead > U16_MAX - skb->network_header)
> +		return false;
> +	if (skb_transport_header_was_set(skb) &&
> +	    nhead >= U16_MAX - skb->transport_header)
> +		return false;
> +	if (skb_mac_header_was_set(skb) &&
> +	    nhead >= U16_MAX - skb->mac_header)
> +		return false;
> +	if (skb->ip_summed == CHECKSUM_PARTIAL &&
> +	    nhead > U16_MAX - skb->csum_start)
> +		return false;
> +	if (skb_inner_network_header_was_set(skb) &&
> +	    nhead > U16_MAX - skb->inner_network_header)
> +		return false;
> +	return true;
> +}

[Severity: Medium]
Should inner_transport_header and inner_mac_header be checked here too?

skb_headers_offset_update() moves all three inner offsets unconditionally:

net/core/skbuff.c:skb_headers_offset_update() {
    ...
	skb->inner_transport_header += off;
	skb->inner_network_header += off;
	skb->inner_mac_header += off;
}

On an encapsulated skb, inner_transport_header usually sits past
inner_network_header by the inner L3 header length (20 bytes for IPv4, 40
or more for IPv6). An nhead between U16_MAX - inner_transport_header and
U16_MAX - inner_network_header passes this check. inner_transport_header
then still wraps to a small value behind skb->data.

One way to reach this is __skb_cow() from ip_forward() with a
large-headroom egress device such as netkit. inner_mac_header isn't
checked either, and it has no unset sentinel.

Could the wrapped inner offsets then confuse tunnel GSO and header length
users such as skb_inner_transport_offset(), inner_tcp_hdrlen() and driver
TSO setup?

> +
>  void skb_headers_offset_update(struct sk_buff *skb, int off)
>  {
>  	/* Only adjust this if it actually is csum_start rather than csum */

[ ... ]

> diff --git a/net/mpls/mpls_iptunnel.c b/net/mpls/mpls_iptunnel.c
> index 1a1a0eb5b787f..5123394234cc5 100644
> --- a/net/mpls/mpls_iptunnel.c
> +++ b/net/mpls/mpls_iptunnel.c
> @@ -59,6 +59,9 @@ static int mpls_xmit(struct sk_buff *skb)
>  	    !dst->lwtstate || skb_warn_if_lro(skb))
>  		goto drop;
>  
> +	if (unlikely(skb_headroom(skb) > U16_MAX))
> +		goto drop;
> +

[Severity: Medium]
This check runs after mpls_forward() has already read the label.

By the time mpls_forward() is called, __netif_receive_skb_core() has
done skb_reset_network_header(). Suppose the skb arrived with headroom
above U16_MAX, for example looped back from a netkit peer with a large
needed_headroom after eth_type_trans() pulled ETH_HLEN. Then
network_header is already truncated and no longer points at skb->data.

mpls_hdr() is just skb_network_header(). So before the new check is
reached:

mpls_forward()
    hdr = mpls_hdr(skb);
    dec = mpls_entry_decode(hdr);
    rt = mpls_route_input_rcu(net, dec.label);
    ...
    nh = mpls_select_multipath(rt, skb);

This decodes a label from stale headroom bytes near skb->head.
mpls_select_multipath() -> mpls_multipath_hash() then walks up to
MAX_MP_SELECT_LABELS entries plus an inner IPv4/IPv6 header from the
same wrapped pointer.

The earlier pskb_may_pull(skb, sizeof(*hdr)) only guarantees bytes at
skb->data, not at the wrapped network header. The route lookup and the
multipath hash are computed on garbage. A lookup miss is also counted
as rx_noroute rather than rx_errors.

v3 moved the mpls_xmit() check ahead of ip_hdr()/ipv6_hdr() for exactly
this reason. Shouldn't the mpls_forward() check also go before the
first mpls_hdr() access, for example right after pskb_may_pull()?
skb_headroom() doesn't change between there and the pop. The same
U16_MAX - sizeof(*hdr) bound would then reject both the offset that
wrapped on receive and the one skb_reset_network_header() would
produce after skb_pull().

[Severity: High]
This check covers the MPLS LWT path. Can the same network_header wrap and
out-of-bounds eth_header() write still happen with no MPLS involved and
without going through pskb_expand_head()?

netkit accepts any u16 headroom and stores it in needed_headroom:

drivers/net/netkit.c:netkit_new_link() {
    ...
		if (data[IFLA_NETKIT_HEADROOM])
			headroom = nla_get_u16(data[IFLA_NETKIT_HEADROOM]);
    ...
	if (headroom)
		dev->needed_headroom = headroom;
    ...
}

netkit_set_headroom() does the same. netkit uses ether_setup(), so
hard_header_len is 14. With a headroom of about 65506 or more,
LL_RESERVED_SPACE(dev) becomes ((14 + 65535) & ~15) + 16 = 65552, which is
above U16_MAX.

A UDP socket or an unprivileged ping socket sending over a route to that
device then goes through:

__ip_append_data()
    hh_len = LL_RESERVED_SPACE(rt->dst.dev);
    skb_reserve(skb, hh_len + exthdrlen);
    skb_reset_network_header(skb);

skb_reset_network_header() only has a DEBUG_NET_WARN_ON_ONCE(), so
network_header is silently truncated to 16 and transport_header to 36.

ip_finish_output2() only expands the head when headroom is short:

	if (unlikely(skb_headroom(skb) < hh_len && dev->header_ops)) {
		skb = skb_expand_head(skb, hh_len);

Here headroom is already >= hh_len, so neither pskb_expand_head() nor the
new skb_header_offsets_fit() check runs.

neigh_resolve_output() or neigh_connected_output() then does:

	__skb_pull(skb, skb_network_offset(skb));

The offset is -65536, which is 0xFFFF0000 as an unsigned value. skb->data
moves forward by about 4 GiB, and dev_hard_header()->eth_header()->
skb_push() writes 14 bytes there. skb_under_panic() doesn't fire because
data is still above head.

This is the failure the commit message describes. Creating the netkit
device only needs CAP_NET_ADMIN, which an unprivileged user namespace
provides.

Would it be better to fix the root cause, for example by capping the
netkit headroom so LL_RESERVED_SPACE() always fits in the 16-bit offsets,
rather than adding checks to individual consumers like mpls_xmit()?

>  	skb_forward_csum(skb);
>  
>  	tun_encap_info = mpls_lwtunnel_encap(dst->lwtstate);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113235019.74384-1-kylebot%40openai.com