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

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

[Kyle Zeng]

An MPLS LWT route over a netkit device configured with large headroom
can make aligned skb expansion wrap the 16-bit network_header.
Neighbour output interprets the resulting negative skb_network_offset()
as a large unsigned pull length, causing eth_header() to write outside
the skb allocation.

Reject growth that cannot preserve the network, set MAC and transport,
recorded inner-network, or partial-checksum offsets. Do not relocate a
set MAC or transport header onto its unset value. Check the actual
post-pop positions in skb_mpls_pop(), bound the native MPLS label pop,
and reject excessive LWT headroom before reading the IP header.

The issue was found by an LLM.

Fixes: 1a37e412a022 ("net: Use 16bits for *_headers fields of struct skbuff")
Cc: stable@vger.kernel.org
Signed-off-by: Kyle Zeng <kylebot@openai.com>
---
Changes in v3:
- Check the actual post-pop MAC/network positions and reject the MAC
  unset value in skb_mpls_pop().
- Check a set outer MAC header and a recorded inner network header
  during positive relocation.
- Move the MPLS LWT entry check before reading the IP header.

v2: https://lore.kernel.org/netdev/20261110051258.64350-1-kylebot@openai.com/
v1: https://lore.kernel.org/netdev/20261030042246.74144-1-kylebot@openai.com/

I have a reproducer for this issue and can share via DM.

 net/core/skbuff.c        | 35 +++++++++++++++++++++++++++++++++++
 net/mpls/af_mpls.c       |  3 +++
 net/mpls/mpls_iptunnel.c |  3 +++
 3 files changed, 41 insertions(+)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 1ac1e327ac4c..bc836adafecd 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;
+}
+
 void skb_headers_offset_update(struct sk_buff *skb, int off)
 {
 	/* Only adjust this if it actually is csum_start rather than csum */
@@ -2317,6 +2339,9 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int ntail,
 		return -EINVAL;
 	}
 
+	if (unlikely(!skb_header_offsets_fit(skb, nhead)))
+		return -EINVAL;
+
 	if (unlikely(skb_shared(skb))) {
 		DEBUG_NET_WARN_ON_ONCE(1);
 		return -EINVAL;
@@ -2535,6 +2560,11 @@ struct sk_buff *skb_copy_expand(const struct sk_buff *skb,
 		return NULL;
 
 	oldheadroom = skb_headroom(skb);
+	if (unlikely(newheadroom > oldheadroom &&
+		     !skb_header_offsets_fit(skb, (unsigned int)newheadroom -
+						  oldheadroom)))
+		return NULL;
+
 	n = __alloc_skb(newheadroom + skb->len + newtailroom,
 			gfp_mask, skb_alloc_rx_flag(skb),
 			NUMA_NO_NODE);
@@ -6804,11 +6834,16 @@ EXPORT_SYMBOL_GPL(skb_mpls_push);
 int skb_mpls_pop(struct sk_buff *skb, __be16 next_proto, int mac_len,
 		 bool ethernet)
 {
+	unsigned int headroom = skb_headroom(skb);
 	int err;
 
 	if (unlikely(!eth_p_mpls(skb->protocol)))
 		return 0;
 
+	if (unlikely(headroom >= U16_MAX - MPLS_HLEN ||
+		     (unsigned int)mac_len > U16_MAX - MPLS_HLEN - headroom))
+		return -EINVAL;
+
 	err = skb_ensure_writable(skb, mac_len + MPLS_HLEN);
 	if (unlikely(err))
 		return err;
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index ad4ad6f2001a..18c9654a6b36 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -419,6 +419,9 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev,
 	if (!nh)
 		goto err;
 
+	if (unlikely(skb_headroom(skb) > U16_MAX - sizeof(*hdr)))
+		goto err;
+
 	/* Pop the label */
 	skb_pull(skb, sizeof(*hdr));
 	skb_reset_network_header(skb);
diff --git a/net/mpls/mpls_iptunnel.c b/net/mpls/mpls_iptunnel.c
index 1a1a0eb5b787..5123394234cc 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;
+
 	skb_forward_csum(skb);
 
 	tun_encap_info = mpls_lwtunnel_encap(dst->lwtstate);

base-commit: 2800868f63227c8237d70c41e70d441c919620d6
-- 
2.55.0.openai.911.g06873a30d2f3

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