[PATCH net v6] ip6_gre: use skb_vlan_inet_prepare() instead of pskb_inet_may_pull()

[PATCH net v6] ip6_gre: use skb_vlan_inet_prepare() instead of pskb_inet_may_pull()

[Anton Danilov]

ip6gre_tunnel_xmit() and ip6erspan_tunnel_xmit() check the packet length
with pskb_inet_may_pull(), which looks at skb->protocol only, while the
code that follows parses the packet through VLAN tags with
skb_protocol(skb, true). The parsing started to look through the tags
with the two commits in the Fixes tags; the check was left as it was.
A VLAN-tagged frame whose inner IPv4/IPv6 header is not in the linear
area passes the check, and the inner header is read anyway. A 20 byte
tagged frame on ip6gretap and a 10 byte frame on ip6erspan are sent
out, while an untagged frame that is too short for its IP header is
already dropped.

Use skb_vlan_inet_prepare(), as the IPv6 receive path does since
commit 81c734dae203 ("ip6_tunnel: use skb_vlan_inet_prepare() in
__ip6_tnl_rcv()"). The second argument (inner_proto_inherit) depends on
the device: ip6gre_tunnel_xmit() serves both ip6gre (ARPHRD_IP6GRE, no
MAC header) and ip6gretap (ARPHRD_ETHER), so it is
dev->type != ARPHRD_ETHER there; ip6erspan is always an Ethernet device,
so it is false. vxlan does the same with no_eth_encap.

skb_vlan_inet_prepare() walks the VLAN tags from skb->mac_len - VLAN_HLEN,
or from ETH_HLEN if skb->mac_len is 0, and on transmit skb->mac_len is
still what the skb was received with. For a packet that came in through
an NBMA gre device and is routed out of a VLAN on ip6gretap or ip6erspan,
the walk starts inside the inner IPv4 header. Clear skb->mac_len for
Ethernet devices first.

An ip6gre device created without a remote uses ip6gre_header_ops, and
ip6gre_header() pushes a pseudo IPv6 header in front of the packet, so
there skb->data is not where the packet starts. skb_vlan_inet_prepare()
would move the network header onto that pseudo header, whose payload
length and GRE words are never written, and an ICMPv6 error for the
packet then quotes them: KMSAN reports uninit-value in
icmpv6_push_pending_frames(). Keep pskb_inet_may_pull() for that case,
as before this change.

With this change the two short frames above are dropped. gre_gso.sh,
l2_tos_ttl_inherit.sh and the mirror_gre, mirror_gre_vlan,
mirror_gre_bridge_1q and mirror_gre_changes forwarding selftests pass.
A constant true instead breaks the ip6gretap cases of mirror_gre.sh,
and a constant false breaks the ip6gre GSO cases of gre_gso.sh.

This fixes the length check in both functions, and the skb_protocol()
dispatch in ip6gre_tunnel_xmit() for ip6gretap and for ip6gre devices
created with a remote. Four related, pre-existing problems are left for
follow-up patches:

 - ip6_tnl_xmit() parses the VLAN tags once more after the GRE header
   has been pushed, so for a tagged frame on ip6gretap with a key or
   on ip6erspan the TTL and traffic class are not inherited, and if
   the bytes that parse lands on happen to read as ETH_P_IP, an
   inherited hop limit is taken from past the end of the frame;
 - erspan_build_header() and erspan_build_header_v2() read the TOS and,
   behind an 0x8100 EtherType, the TCI without checking that the frame
   is long enough, so a 14 to 19 byte frame that passes the check here
   still gets bytes from past its end into the ERSPAN header;
 - the ip6gre header_ops branch kept above still has the check/parse
   mismatch once a remote is set with changelink;
 - gre_tap_xmit(), erspan_xmit() and ip_tunnel_rcv() on the IPv4 side
   have the same mismatch.

Fixes: 3f8a8447fd0b ("ip6_gre: use actual protocol to select xmit")
Fixes: b09ab9c92e50 ("ip6_tunnel: allow to inherit from VLAN encapsulated IP")
Reported-by: syzbot+6023ea32e206eef7920a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=6023ea32e206eef7920a
Suggested-by: Eric Dumazet <edumazet@kernel.org>
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Anton Danilov <littlesmilingcloud@gmail.com>
---
v6: no functional change from v5; commit message only. The AI review
    of v5 again pointed at wording that reads wider than the code:
    "fixes the dispatch in ip6gre_tunnel_xmit()" now excludes the
    header_ops branch, which keeps the old check; the
    erspan_build_header() reads on 14 to 19 byte frames are listed as
    a follow-up instead of "the short frame is dropped" suggesting that
    all of them are; and the ip6_tnl_xmit() item says that an inherited
    hop limit can come from past the end of a crafted frame, not only
    from the wrong offset. Measured on net and on this patch: the same
    bytes, see the reply to the review.
    Link: https://lore.kernel.org/netdev/179141499719.434549.15788178109134377075@kernel.org/
v5: https://lore.kernel.org/netdev/20261005231414.932997-1-littlesmilingcloud@gmail.com/
v4: https://lore.kernel.org/netdev/20261004145205.226974-1-littlesmilingcloud@gmail.com/
v3: https://lore.kernel.org/netdev/20260119112512.28196-1-fw@strlen.de/
v2: https://lore.kernel.org/netdev/20260106144529.1424886-1-edumazet@google.com/
v1: https://lore.kernel.org/netdev/20260105100330.2258612-1-edumazet@google.com/

 net/ipv6/ip6_gre.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 04f7c70b7320..06dd53c5a9e2 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -883,8 +883,23 @@ static netdev_tx_t ip6gre_tunnel_xmit(struct sk_buff *skb,
 	__be16 payload_protocol;
 	int ret;
 
-	if (!pskb_inet_may_pull(skb))
-		goto tx_err;
+	if (dev->type != ARPHRD_ETHER && dev->header_ops) {
+		/* ip6gre_header() has pushed a pseudo header in front of
+		 * the packet, so skb->data is not where the packet starts.
+		 */
+		if (!pskb_inet_may_pull(skb))
+			goto tx_err;
+	} else {
+		/* skb_vlan_inet_prepare() and the skb_protocol() dispatch
+		 * below walk the VLAN tags from skb->mac_len - VLAN_HLEN, or
+		 * from ETH_HLEN if it is 0; a forwarded skb still has the
+		 * mac_len of the device it was received on.
+		 */
+		if (dev->type == ARPHRD_ETHER)
+			skb->mac_len = 0;
+		if (skb_vlan_inet_prepare(skb, dev->type != ARPHRD_ETHER))
+			goto tx_err;
+	}
 
 	if (!ip6_tnl_xmit_ctl(t, &t->parms.laddr, &t->parms.raddr))
 		goto tx_err;
@@ -933,7 +948,13 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
 	__u32 mtu;
 	int nhoff;
 
-	if (!pskb_inet_may_pull(skb))
+	/* skb_vlan_inet_prepare() below walks the VLAN tags from
+	 * skb->mac_len - VLAN_HLEN, or from ETH_HLEN if it is 0, to check
+	 * the length; a forwarded skb still has the mac_len of the device
+	 * it was received on.
+	 */
+	skb->mac_len = 0;
+	if (skb_vlan_inet_prepare(skb, false))
 		goto tx_err;
 
 	if (!ip6_tnl_xmit_ctl(t, &t->parms.laddr, &t->parms.raddr))
-- 
2.47.3

Re: [PATCH net v6] ip6_gre: use skb_vlan_inet_prepare() instead of pskb_inet_may_pull()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 (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).

- [Medium] Regression: in ip6gre_tunnel_xmit() (ip6gretap branch) and
  ip6erspan_tunnel_xmit(), skb_vlan_inet_prepare() now always calls…

Pre-existing issues:
- [Medium] ip6_tnl_xmit() calls skb_protocol(skb, true) again (line 1119)
  after the GRE header (and, for ip6erspan, the ERSPAN header) has been…

--

ip6_gre: use skb_vlan_inet_prepare() instead of pskb_inet_may_pull()

This patch changes ip6gre_tunnel_xmit() and ip6erspan_tunnel_xmit() to
use skb_vlan_inet_prepare() instead of pskb_inet_may_pull(). The length
check then matches the VLAN-aware skb_protocol() parsing that comes after
it. For Ethernet devices it first clears skb->mac_len, and it keeps
pskb_inet_may_pull() for ip6gre devices that use ip6gre_header_ops.

Closes: https://syzkaller.appspot.com/bug?extid=6023ea32e206eef7920a

> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index 04f7c70b7320e..06dd53c5a9e25 100644
> --- a/net/ipv6/ip6_gre.c
> +++ b/net/ipv6/ip6_gre.c
> @@ -883,8 +883,23 @@ static netdev_tx_t ip6gre_tunnel_xmit(struct sk_buff *skb,
>  	__be16 payload_protocol;
>  	int ret;
>  
> -	if (!pskb_inet_may_pull(skb))
> -		goto tx_err;
> +	if (dev->type != ARPHRD_ETHER && dev->header_ops) {
> +		/* ip6gre_header() has pushed a pseudo header in front of
> +		 * the packet, so skb->data is not where the packet starts.
> +		 */
> +		if (!pskb_inet_may_pull(skb))
> +			goto tx_err;
> +	} else {
> +		/* skb_vlan_inet_prepare() and the skb_protocol() dispatch
> +		 * below walk the VLAN tags from skb->mac_len - VLAN_HLEN, or
> +		 * from ETH_HLEN if it is 0; a forwarded skb still has the
> +		 * mac_len of the device it was received on.
> +		 */
> +		if (dev->type == ARPHRD_ETHER)
> +			skb->mac_len = 0;
> +		if (skb_vlan_inet_prepare(skb, dev->type != ARPHRD_ETHER))
> +			goto tx_err;

[Severity: Medium]

Can this move the network header onto the VLAN TCI? This would happen for
skbs that already carry an in-band tag while skb->protocol holds the inner
protocol.

skb_vlan_inet_prepare() works out maclen from skb->protocol alone, and then
always sets the network header:

include/net/ip_tunnels.h:skb_vlan_inet_prepare() {
    ...
	if (eth_type_vlan(type))
		type = __vlan_get_protocol(skb, type, &maclen);
    ...
	skb_set_network_header(skb, maclen);
}

vlan_do_receive() creates such skbs on a VLAN device with reorder_hdr off
that is not a bridge or macvlan port:

net/8021q/vlan_core.c:vlan_do_receive() {
    ...
		skb = *skbp = vlan_insert_inner_tag(skb, skb->vlan_proto,
						    skb->vlan_tci, skb->mac_len);
    ...
		skb_reset_mac_len(skb);
}

That leaves skb->protocol as ETH_P_IP or ETH_P_IPV6, with the tag at
offset 12 and the network header at offset 18 (mac_len is 18). Say tc
mirred on that VLAN device's ingress sends the skb to ip6gretap or
ip6erspan. tcf_mirred_to_dev() pushes mac_len, so the frame reaches the
tunnel with the tag still in-band.

With pskb_inet_may_pull() the network header stayed at 18. Here
eth_type_vlan(ETH_P_IP) is false, so maclen stays ETH_HLEN and the network
header moves to 14, which is the TCI.

Would everything after the check then read the inner header 4 bytes off?
For example:

  - prepare_ip6gre_xmit_ipv4() with IP6_TNL_F_USE_ORIG_TCLASS takes the
    dsfield from the low byte of the TCI.

  - In ip6erspan_tunnel_xmit(), ip_hdr(skb)->tot_len reads the
    encapsulated EtherType (0x0800). ipv6_hdr(skb)->payload_len reads the
    0x6xxx version word. Either way truncate (the ERSPAN T bit) is set
    for nearly every mirrored frame.

  - prepare_ip6gre_xmit_ipv6() does the raddr loop check and
    ip6_tnl_parse_tlv_enc_lim() on shifted bytes, so a flow label byte
    acts as nexthdr. That can cause drops or bogus ICMPV6_PARAMPROB
    replies.

  - ICMP errors sent on -EMSGSIZE, and the inherited hop limit in
    ip6_tnl_xmit(), are taken from the shifted header.

All of these reads stay inside the pulled area.

> +	}
>  
>  	if (!ip6_tnl_xmit_ctl(t, &t->parms.laddr, &t->parms.raddr))
>  		goto tx_err;
> @@ -933,7 +948,13 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
>  	__u32 mtu;
>  	int nhoff;
>  
> -	if (!pskb_inet_may_pull(skb))
> +	/* skb_vlan_inet_prepare() below walks the VLAN tags from
> +	 * skb->mac_len - VLAN_HLEN, or from ETH_HLEN if it is 0, to check
> +	 * the length; a forwarded skb still has the mac_len of the device
> +	 * it was received on.
> +	 */
> +	skb->mac_len = 0;

[Severity: Medium]

This is a pre-existing issue that the patch did not introduce, and the
commit message already lists it as a follow-up. Still, clearing mac_len
here changes where the second VLAN walk in ip6_tnl_xmit() lands.

ip6_tnl_xmit() runs after the GRE header has been pushed (for ip6erspan,
the ERSPAN header too):

net/ipv6/ip6_tunnel.c:ip6_tnl_xmit() {
    ...
	payload_protocol = skb_protocol(skb, true);
    ...
	if (hop_limit == 0) {
		if (payload_protocol == htons(ETH_P_IP))
			hop_limit = ip_hdr(skb)->ttl;
		else if (payload_protocol == htons(ETH_P_IPV6))
			hop_limit = ipv6_hdr(skb)->hop_limit;
    ...
}

Now that mac_len is 0, the walk starts ETH_HLEN past the GRE header. For
keyed ip6gretap and for ip6erspan, that is on MAC or ERSPAN bytes the
sender controls.

Suppose those bytes read as ETH_P_IP or ETH_P_IPV6 and hop limit
inheritance is on. Then ttl or hop_limit is read at the network header
that skb_vlan_inet_prepare() set. For a short tagged frame with a non-IP
inner type, only maclen bytes were pulled.

Could this read a byte past skb->tail and put it in the outer IPv6 hop
limit? A locally generated short frame, for example from veth or
AF_PACKET, could hit this.

In the other cases the real inner TTL and traffic class are just not
inherited. For unkeyed ip6gretap, the second walk now lands on the real
inner protocol.

> +	if (skb_vlan_inet_prepare(skb, false))
>  		goto tx_err;
>  
>  	if (!ip6_tnl_xmit_ctl(t, &t->parms.laddr, &t->parms.raddr))

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008012554.202925-1-littlesmilingcloud%40gmail.com