Re: [PATCH net] net: mpls: keep transport offsets valid across label changes

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 1

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.
- [Low] The new `if (skb->encapsulation) return -EINVAL;` in
  skb_mpls_pop() (net/core/skbuff.c) rejects every encapsulated MPLS…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but nsh_pop()
  (net/nsh/nsh.c) leaves transport_header behind skb->data in the same…

--

net: mpls: keep transport offsets valid across label changes

This commit changes skb_mpls_push() and skb_mpls_pop(). After a label push
or pop, a transport header that sat at or before the old network header is
moved to the new network header. The commit also makes skb_mpls_pop() reject
encapsulated skbs and refresh mac_len after a pop. The goal is to prevent the
negative skb_transport_offset() that makes qdisc_pkt_len_segs_init()
dereference about 4 GiB past skb->data.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 41beaf625421a..eb49201534fff 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c

[ ... ]

> @@ -6676,6 +6681,10 @@ int skb_mpls_pop(struct sk_buff *skb, __be16 next_proto, int mac_len,
>  	if (unlikely(!eth_p_mpls(skb->protocol)))
>  		return 0;
>  
> +	/* Networking stack does not allow simultaneous Tunnel and MPLS GSO. */
> +	if (skb->encapsulation)
> +		return -EINVAL;

[Severity: Low]
Will this drop non-GSO encapsulated MPLS packets that were popped and
forwarded before this patch?

The crash only happens with GSO skbs. qdisc_pkt_len_segs_init() returns
before it reads either transport offset when gso_size is 0. Also, a pop only
moves bytes up to the MAC header, so the absolute inner header positions
stay the same.

Encapsulated MPLS skbs can exist in tree. Take a locally generated VXLAN or
GRE packet with an inner CHECKSUM_PARTIAL that is not GSO: it keeps
skb->encapsulation set, because iptunnel_handle_offloads() only clears the
flag when ip_summed != CHECKSUM_PARTIAL. If the outer route uses an MPLS
lwtunnel, mpls_xmit() does:

	skb->dev = out_dev;
	skb->protocol = htons(ETH_P_MPLS_UC);

and does not clear skb->encapsulation. skb_scrub_packet() does not clear it
either when the skb crosses a veth into another netns.

If tc act_mpls pop or OVS pop_mpls then handles that packet, skb_mpls_pop()
now returns -EINVAL, and tcf_mpls_act() drops the packet:

	case TCA_MPLS_ACT_POP:
		if (skb_mpls_pop(skb, p->tcfm_proto, mac_len,
				 skb->dev && skb->dev->type == ARPHRD_ETHER))
			goto drop;

The commit message says this is deliberate, to match skb_mpls_push(), which
already rejects all encapsulated skbs. This topology is also unusual.

Since this is going to stable, would a narrower check such as
skb_is_gso(skb) && skb->encapsulation close the crash without these drops?

> +
>  	err = skb_ensure_writable(skb, mac_len + MPLS_HLEN);
>  	if (unlikely(err))
>  		return err;
> @@ -6687,6 +6696,10 @@ int skb_mpls_pop(struct sk_buff *skb, __be16 next_proto, int mac_len,
>  	__skb_pull(skb, MPLS_HLEN);
>  	skb_reset_mac_header(skb);
>  	skb_set_network_header(skb, mac_len);
> +	if (skb_transport_header_was_set(skb) &&
> +	    skb->transport_header <= skb->network_header)
> +		skb->transport_header = skb->network_header;
> +	skb_reset_mac_len(skb);

[Severity: High]
Is this fix complete? The subject says transport offsets stay valid across
label changes. However, mpls_forward() in net/mpls/af_mpls.c also pops a
label without moving skb->transport_header or checking skb->encapsulation:

net/mpls/af_mpls.c:mpls_forward() {
    ...
	/* Pop the label */
	skb_pull(skb, sizeof(*hdr));
	skb_reset_network_header(skb);
    ...
		skb_push(skb, new_header_size);
		skb_reset_network_header(skb);
    ...
}

If the route has no outgoing labels and the label is not BOS, nothing is
pushed back. The skb then goes to neigh_xmit() with transport_header behind
network_header.

Here is a single-hop sequence that looks reachable:

1. A tap writer sends a VID 0 tagged MPLS frame with a vnet header that has
   NEEDS_CSUM, gso_type TCPV4, csum_start=20 and csum_offset=16.
   __virtio_net_hdr_to_skb() does not check the protocol, so
   transport_header ends up at mac+20.

2. skb_vlan_untag() moves network_header to mac+18 but keeps the
   transport_header that is already set. transport_header now sits inside
   the first label.

3. mpls_forward() pops that label. network_header moves to 22 and
   transport_header stays at 20.

4. The route is "ip -f mpls route add L dev tunX", with no via and no
   labels. neigh_xmit(NEIGH_LINK_TABLE) calls dev_hard_header(), which does
   nothing for an IFF_TUN device, and then dev_queue_xmit().

5. __dev_queue_xmit() resets mac_header to data. Then, in
   qdisc_pkt_len_segs_init():

	hdr_len = skb_transport_offset(skb);
	...
		if (!pskb_may_pull(skb, hdr_len + sizeof(struct tcphdr)))
			return SKB_DROP_REASON_SKB_BAD_GSO;

		th = (const struct tcphdr *)(skb->data + hdr_len);

   hdr_len is (unsigned)-2, so the pskb_may_pull() length wraps to 18 and
   the check passes. th then points about 4 GiB past skb->data, which is the
   same oops as in the commit message.

mpls_pkt_too_big() does not catch this, because it only validates GSO skbs
when skb->len > mtu.

Multi-hop paths over veth reach the same state, since each hop pops another
4 bytes. A clsact sch_handle_ingress() on the peer also calls
qdisc_pkt_len_segs_init().

An encapsulated skb in mpls_forward() also keeps stale
inner_transport_header metadata.

The tun device, MPLS routes and the related sysctls only need CAP_NET_ADMIN
in a netns owned by a user namespace.

Could mpls_forward() get the same adjustment? Alternatively,
qdisc_pkt_len_segs_init() could reject a negative transport offset itself.

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. nsh_pop()
in net/nsh/nsh.c also leaves transport_header behind skb->data, and the skb
reaches the same qdisc_pkt_len_segs_init() consumer:

net/nsh/nsh.c:nsh_pop() {
    ...
	skb_pull_rcsum(skb, length);
	skb_reset_mac_header(skb);
	skb_reset_network_header(skb);
	skb_reset_mac_len(skb);
    ...
}

Here is a possible trigger:

1. A tap writer sends an ETH_P_NSH frame with a vnet header that has
   NEEDS_CSUM, gso_type TCPV4, csum_start=20 and csum_offset=16.
   __virtio_net_hdr_to_skb() does not check the protocol on the NEEDS_CSUM
   path, so transport_header lands inside the 8-byte NSH header.

2. The tap is an OVS netdev vport. key_extract() leaves transport_header
   alone for non-IP eth types.

3. A flow runs with pop_eth, pop_nsh (np = TUN_P_ETHERNET) and output to an
   Ethernet netdev vport. pop_nsh() only invalidates the flow key, and
   do_output() -> ovs_vport_send() -> dev_queue_xmit() runs without
   extracting the key again.

4. qdisc_pkt_len_segs_init() sees skb_transport_offset() == -2 and reads
   about 4 GiB past skb->data, as described above.

OVS genl commands use GENL_UNS_ADMIN_PERM, so this can also be triggered
from inside a user namespace.

This patch fixes producers of the bad offset one at a time. Would a check
in qdisc_pkt_len_segs_init() cover all of them? It could drop skbs with a
negative transport or inner transport offset, using
SKB_DROP_REASON_SKB_BAD_GSO.

>  
>  	if (ethernet && mac_len >= ETH_HLEN) {
>  		struct ethhdr *hdr;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011041436.1424222-2-bestswngs%40gmail.com