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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 3 · Medium: 3 · 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.
- [High] net: skb_mpls_pop() checks wrong offset and hits mac_header
  sentinel
- [High] net: u16 transport_header wrap after skb_mpls_pop() and
  mpls_forward()
- [Medium] skb_header_offsets_fit() (net/core/skbuff.c ~2121-2135) does
  not check every field that skb_headers_offset_update() shifts right…
- [Medium] The new check in mpls_xmit() (net/mpls/mpls_iptunnel.c, after
  skb_cow_head()) carries the comment 'The aligned expansion must still…
- [Medium] skb_header_offsets_fit() rejects only when `nhead > U16_MAX -
  skb->network_header`, so after an accepted expansion network_header…

--

net: preserve skb header offsets during headroom changes

This patch adds a skb_header_offsets_fit() check to pskb_expand_head()
and skb_copy_expand(). The check rejects headroom growth that would wrap
the u16 network, transport or csum_start offsets. It also adds bounds to
mpls_forward(), mpls_xmit() and skb_mpls_pop() for header moves that
do not expand the allocation.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 8da53d5b76fc2..6e0205f923f38 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -2118,6 +2118,22 @@ 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->ip_summed == CHECKSUM_PARTIAL &&
> +	    nhead > U16_MAX - skb->csum_start)
> +		return false;
> +	return true;
> +}
> +
>  void skb_headers_offset_update(struct sk_buff *skb, int off)
>  {

[Severity: Medium]
Can the inner header offsets still wrap after this check passes?

skb_header_offsets_fit() checks network_header, transport_header and
csum_start. skb_headers_offset_update() also shifts mac_header (when set),
and it always shifts the inner offsets:

    skb->inner_transport_header += off;
    skb->inner_network_header += off;
    skb->inner_mac_header += off;

On encapsulated skbs the inner offsets sit above network_header.
mpls_xmit() in this same patch builds that layout:

    skb_reset_inner_network_header(skb);   /* inner = H */
    skb_push(skb, new_header_size);
    skb_reset_network_header(skb);         /* network = H - new_header_size */

UDP/GRE tunnels build a similar layout after iptunnel_handle_offloads().

Take a later pskb_expand_head() or skb_copy_expand() with nhead in the
range (U16_MAX - inner_network_header, U16_MAX - network_header]. It
passes this check, and inner_network_header then wraps to a small value.

mpls_gso_segment() computes:

    mpls_hlen = skb_inner_network_header(skb) - skb_network_header(skb);

That consumer seems to fail safely through pskb_may_pull(). The tunnel
GSO and driver offload users of the inner offsets were not traced.

There is also an edge case with mac_header. The network check uses '>',
so network_header + nhead can equal 0xFFFF. When mac_header equals
network_header (mac_len == 0 on L3 devices), mac_header becomes 0xFFFF.
skb_mac_header_was_set() treats that value as unset.

The transport check uses '>=' to protect its sentinel. Should mac_header
get the same protection, and should the inner offsets be checked too?

[ ... ]

> @@ -2305,6 +2321,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;
> +

[Severity: High]
Does this cover the allocation paths that can wrap network_header without
ever calling pskb_expand_head()?

netkit_new_link() and netkit_set_headroom() in drivers/net/netkit.c take
any u16 IFLA_NETKIT_HEADROOM value and store it in dev->needed_headroom.
There is no upper bound. netkit uses ether_setup(), so hard_header_len is
14. With needed_headroom 65535:

    LL_RESERVED_SPACE(dev) = ((14 + 65535) & ~15) + 16 = 65552

Several transmit paths reserve that much on a fresh skb and then set the
network header from skb->data:

net/ipv4/raw.c:raw_send_hdrinc() {
    hlen = LL_RESERVED_SPACE(rt->dst.dev);
    ...
    skb_reserve(skb, hlen);
    ...
    skb_reset_network_header(skb);
}

__ip_append_data() in net/ipv4/ip_output.c follows the same pattern with
LL_RESERVED_SPACE(). packet_snd() in net/packet/af_packet.c follows it
with LL_RESERVED_SPACE_EX().

In each case network_header is stored as 65552 & 0xffff = 16. The
DEBUG_NET_WARN_ON_ONCE() in skb_reset_network_header() compiles away
without CONFIG_DEBUG_NET.

ip_finish_output2() only expands when skb_headroom(skb) < hh_len. That is
false here, so skb_header_offsets_fit() never runs. The packet then goes
through:

neigh_resolve_output()
  __skb_pull(skb, skb_network_offset(skb))   /* -65536 -> 0xFFFF0000 */
  dev_hard_header()
    eth_header()
      skb_push(skb, ETH_HLEN)   /* 14 bytes written at the wild address */

neigh_connected_output() has the same __skb_pull().

This looks like the out-of-bounds Ethernet header write described in the
commit message. It is reachable from a plain raw, UDP or ping sendmsg over
a netkit device with large headroom. CAP_NET_ADMIN/CAP_NET_RAW inside a
user namespace netns is enough.

Would capping dev->needed_headroom (or LL_RESERVED_SPACE()) below U16_MAX,
with some slack, fix the root cause and make the per-site checks
unnecessary?

[ ... ]

> @@ -6771,6 +6795,9 @@ int skb_mpls_pop(struct sk_buff *skb, __be16 next_proto, int mac_len,
>  	if (unlikely(!eth_p_mpls(skb->protocol)))
>  		return 0;
>  
> +	if (unlikely(skb->network_header > U16_MAX - MPLS_HLEN))
> +		return -EINVAL;
> +

[Severity: High]
Is there enough room left for the inner L3 header after the pop?

This check lets the post-pop network_header reach U16_MAX. The bound
added to skb_mpls_pop() does the same.

skb_header_offsets_fit() also lets an earlier expansion get there.
While the skb is still MPLS, transport_header is either unset or
equal to the outer network_header. Headroom and network_header can
therefore grow to about 65534, for example by skb_cow() towards a
netkit device with a large IFLA_NETKIT_HEADROOM. For penultimate hop
popping, this check also runs before the skb_cow() for out_dev further
down.

When the inner IPv4 packet reaches ip_rcv_core(), for example after
an act_mpls or Open vSwitch pop at ingress, it does:

    skb->transport_header = skb->network_header + iph->ihl*4;

With network_header in [65516, 65535], or a wider range with IP
options, the u16 wraps to a small value.

ip_local_deliver_finish() then calls __skb_pull() with
skb_network_header_len(skb). That is the u32 difference and comes out
around 0xFFFF0014. On a linear skb data_len is 0, and the wrapped
skb->len stays above it, so the BUG() in __skb_pull() does not fire.
The len > INT_MAX check only warns under CONFIG_DEBUG_NET. skb->data
ends up about 4 GiB past skb->head before ip_protocol_deliver_rcu()
reads the L4 header.

ip6_rcv_core() sets transport_header the same way, using
sizeof(struct ipv6hdr).

This also needs only CAP_NET_ADMIN in a user namespace netns.

Should the pop paths leave room for the following L3 header? Or would
bounding headroom well below U16_MAX at creation avoid picking a
margin at every site?

[Severity: High]
Does this check bound the offsets that skb_mpls_pop() actually writes?

After the pull, skb_reset_mac_header() stores skb_headroom(skb) +
MPLS_HLEN in mac_header. skb_set_network_header(skb, mac_len) stores
skb_headroom(skb) + MPLS_HLEN + mac_len in network_header. Neither
value comes from the incoming skb->network_header. The guard only
holds when network_header already equals skb_headroom(skb) + mac_len
on entry.

Two cases seem to get past it.

The first is mac_len == 0, for example an L3 MPLS packet in Open
vSwitch, or act_mpls after the MAC header was popped. A packet with
skb_headroom(skb) == network_header == U16_MAX - MPLS_HLEN passes the
check. skb_reset_mac_header() then sets mac_header to U16_MAX, which
is the unset sentinel. skb_mac_header_was_set() returns false from
then on, and later skb_mac_header() users trip
DEBUG_NET_WARN_ON_ONCE().

The second is TC ingress, where tcf_mpls_act() in net/sched/act_mpls.c
does:

    skb_push_rcsum(skb, skb->mac_len);
    mac_len = skb->mac_len;

That moves skb->data back to the MAC header without touching
network_header. If __netif_receive_skb_core() already stored a wrapped
network_header (0..13 for a headroom of 65536..65549), headroom here
is 65522..65535. The check sees a small network_header and passes.
skb_reset_mac_header() and skb_set_network_header() then both wrap.
A preceding skb_vlan_push(), which decrements network_header, can
produce a similar mismatch.

Both cases need only CAP_NET_ADMIN in a user namespace netns, using
act_mpls or an Open vSwitch pop_mpls action on a device with large
headroom.

Should the check be on skb_headroom(skb) + MPLS_HLEN + mac_len, and
keep mac_header strictly below U16_MAX? Or does this suggest the
headroom should be bounded where it is created?

[Severity: Medium]
What about other header pops that do not expand the skb?

skb_header_offsets_fit() lets network_header reach exactly U16_MAX after
an expansion. That means every later forward header move without
expansion needs its own bound. This patch adds one to skb_mpls_pop(),
mpls_forward() and mpls_xmit().

bpf_skb_net_hdr_pop() in net/core/filter.c does the same kind of pop for
bpf_skb_adjust_room() on shrink (up to 0xfff bytes), with no U16_MAX
check:

net/core/filter.c:bpf_skb_net_hdr_pop() {
    ...
    ret = bpf_skb_generic_pop(skb, off, len);
    if (likely(!ret)) {
        skb->mac_header += len;
        skb->network_header += len;
        if (trans_moves) {
            skb->transport_header += len;
    ...
}

Here is one possible trigger. A tc/tcx BPF program runs on an skb whose
network_header is within 0xfff of U16_MAX, for example after
skb_expand_head() on a large-headroom netkit device. If the program
shrinks the room, network_header wraps to a small value.

ip_rcv_core() would then parse the IP header from uncopied headroom. On
forwarding, neigh_resolve_output() would see a negative
skb_network_offset() again.

Whether an skb really reaches such a program with network_header that
close to U16_MAX was not fully traced. Does bpf_skb_net_hdr_pop() need
the same bound?

[ ... ]

> 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]
Can this check fire because of the expansion, as the comment says?

On this LWT output path skb->data is at the network header. With the new
guard in pskb_expand_head(), a successful skb_cow_head() expansion keeps
network_header + nhead <= U16_MAX. Headroom after the expansion is
therefore at most U16_MAX.

So skb_headroom(skb) > U16_MAX can only be true here if headroom was
already above U16_MAX on entry. That would mean network_header was
already wrapped by an allocation path such as raw_send_hdrinc() or
__ip_append_data().

In that case mpls_xmit() has already read through the wrapped offset a
few lines earlier:

    ttl = ip_hdr(skb)->ttl;

The IPv4/IPv6 output code before the LWT hook has used it as well.

Could the comment be reworded to say what this check actually catches?
Alternatively, would bounding headroom at allocation time make this check
unnecessary?

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