Re: [PATCH] mpls: reject headroom that overflows skb header offsets

[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] The new `skb_headroom(skb) > U16_MAX` check in mpls_xmit() and
  mpls_forward() only shows that the data offset (skb->data - skb->head)…
- [Medium] mpls: mpls_forward() resets network_header before U16_MAX
  headroom check

--

mpls: reject headroom that overflows skb header offsets

This adds a check after skb_cow()/skb_cow_head() in mpls_forward() and
mpls_xmit(). The check drops the packet if skb_headroom() exceeds U16_MAX.
The aim is to stop the u16 header offsets from truncating when the aligned
headroom expansion pushes skb->data more than U16_MAX bytes past skb->head.

> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> index ad4ad6f2001ab..4bfdb7c81f046 100644
> --- a/net/mpls/af_mpls.c
> +++ b/net/mpls/af_mpls.c
> @@ -453,6 +453,10 @@ static int mpls_forward(struct sk_buff *skb, struct net_device *dev,
>  	if (skb_cow(skb, hh_len + new_header_size))
>  		goto tx_err;
>  
> +	/* The aligned expansion must still fit the u16 header offsets. */
> +	if (unlikely(skb_headroom(skb) > U16_MAX))
> +		goto tx_err;
> +
>  	skb->dev = out_dev;
>  	skb->protocol = htons(ETH_P_MPLS_UC);
>  
> diff --git a/net/mpls/mpls_iptunnel.c b/net/mpls/mpls_iptunnel.c
> index 1a1a0eb5b787f..4eccbd911fe1f 100644
> --- a/net/mpls/mpls_iptunnel.c
> +++ b/net/mpls/mpls_iptunnel.c
> @@ -109,6 +109,10 @@ static int mpls_xmit(struct sk_buff *skb)
>  	if (skb_cow_head(skb, hh_len + new_header_size))
>  		goto drop;
>  
> +	/* The aligned expansion must still fit the u16 header offsets. */
> +	if (unlikely(skb_headroom(skb) > U16_MAX))
> +		goto drop;

[Severity: Medium]
Is the new check early enough in mpls_forward()?

Before skb_cow() is reached, the label pop has already moved skb->data
forward and written the new offset into the u16 network_header:

net/mpls/af_mpls.c:mpls_forward() {
	...
	/* Pop the label */
	skb_pull(skb, sizeof(*hdr));
	skb_reset_network_header(skb);
	...
	if (mpls_pkt_too_big(skb, mtu - new_header_size))
		goto tx_err;
	...
}

Suppose an MPLS skb arrives with skb_headroom() in [65532, 65535]. That
is still within U16_MAX, so nothing rejects it on the way in. The
4-byte skb_pull() then takes the headroom to 65536..65539.
skb_reset_network_header() hits its DEBUG_NET_WARN_ON_ONCE() and
truncates network_header to 0..3.

One source of such an skb is a previous mpls_forward() hop. With
nh_labels == 0 and !dec.bos, new_header_size is 0, so the new check
accepts a headroom of up to 65535 and sends the packet on. After the
peer's eth_type_trans(), the next hop sees the MPLS header at that same
offset. AF_PACKET on a netkit device with a near-maximal
needed_headroom looks like another way to inject one.

The truncated offset is also used before the packet is dropped. For a
GSO skb larger than mtu - new_header_size, mpls_pkt_too_big() calls
skb_gso_validate_network_len(). That goes through
skb_gso_network_seglen(), which computes skb_transport_header() -
skb_network_header(), so the MTU decision is based on a bogus header
length.

skb_cow() can only grow the headroom, so the new check after it will
still drop the skb. By then, though, the warning has already fired. On
CONFIG_DEBUG_NET kernels with panic_on_warn, that is a crash reachable
from a user-owned network namespace.

Could the bound be enforced before the pop, for example by rejecting
an incoming skb whose headroom plus sizeof(*hdr) would exceed U16_MAX,
before skb_reset_network_header() runs? Or is there a better place to
catch this for skbs that reach mpls_forward() with headroom close to
the limit?

[Severity: High]

Does this check cover all of the u16 header offsets that the comment
refers to?

By the time it runs, skb_cow_head() has already gone through
__skb_cow()->pskb_expand_head()->skb_headers_offset_update(), and that
function does unchecked u16 additions:

net/core/skbuff.c:skb_headers_offset_update() {
	if (skb->ip_summed == CHECKSUM_PARTIAL)
		skb->csum_start += off;
	/* {transport,network,mac}_header and tail are relative to skb->head */
	skb->transport_header += off;
	skb->network_header   += off;
	...
	skb->inner_transport_header += off;
	skb->inner_network_header += off;
	skb->inner_mac_header += off;
}

skb_headroom() only bounds skb->data - skb->head. The transport_header
and csum_start offsets point past skb->data, so they can wrap while the
headroom is still at most U16_MAX.

For example, take a netkit device with needed_headroom in [65426, 65441].
That gives LL_RESERVED_SPACE() = 65456. A UDP packet built by
__ip_append_data() reserves 65456 bytes of headroom, so transport_header
is 65476. netkit advertises NETIF_F_HW_CSUM, so udp_send_skb() sets
CHECKSUM_PARTIAL with csum_start = 65476.

mpls_xmit() then asks for 65460 bytes, and __skb_cow() expands by
ALIGN(4, NET_SKB_PAD) = 64. The headroom becomes 65520 and passes the new
check. transport_header and csum_start become 65540, which wraps to 4.

netkit_setup() sets mpls_features to NETIF_F_HW_CSUM | NETIF_F_GSO_SOFTWARE,
so the skb reaches the peer unchanged. If it is later sent out a device
without checksum offload, validate_xmit_skb() ends up in
skb_checksum_help():

net/core/dev.c:skb_checksum_help() {
	...
	offset = skb_checksum_start_offset(skb);
	ret = -EINVAL;
	if (unlikely(offset >= skb_headlen(skb))) {
		DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false);
		WARN_ONCE(true, "offset (%d) >= skb_headlen() (%u)\n",
	...
}

The negative offset is compared as a large unsigned value, so skb_dump()
and WARN_ONCE() fire. A device with checksum offload would instead get a
bogus checksum start offset.

mpls_forward() has the same headroom-only check after skb_cow(). The
same window appears to apply there to CHECKSUM_PARTIAL skbs received from
a netkit peer.

Could the check bound the largest header offset, or refuse the expansion
before it wraps, instead of testing skb_headroom() after the offsets have
already been updated?

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