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

[Anton Danilov <littlesmilingcloud@gmail.com>]

On Tue, Jan 20, 2026 at 01:01:01AM +0100, Florian Westphal wrote:

> It has to be either true or false depending on test case 8-/
>
> gre_gso.sh needs this to be set to true, skbs don't have a mac
> header: with "false": skb nhoff gets munged from 0 to 14.
>
> But in mirror_gre.sh test case, skbs do have a mac header:
> "true" munges nh offset from 14 to 0 and test fails.

Sorry for reviving an old thread: the syzbot report is still open and
net still has pskb_inet_may_pull() in both functions.

The two tests use different devices behind the same
ip6gre_tunnel_xmit(): ip6gre (no MAC header) in gre_gso.sh and
ip6gretap (ARPHRD_ETHER) in mirror_gre.sh. So the argument can
follow the device type, as vxlan does with no_eth_encap:

	skb_vlan_inet_prepare(skb, dev->type != ARPHRD_ETHER)

ip6erspan is always ARPHRD_ETHER, so false there, as Eric had it.

Tested on net (6e0022b5ae3d), changing only ip6_gre.c:

  ip6gre, ip6erspan     gre_gso.sh          mirror_gre.sh
  --------------------------------------------------------------
  unpatched             pass                pass
  false, false (v2)     2 GSO tests fail    pass
  true,  true (v3)      pass                2 ip6gretap tests fail
  true,  false          pass                2 ip6gretap tests fail
  diff below            pass                pass

The selftests only check that normal traffic still goes. They don't
send short tagged frames, so they do not hit this bug. The bug needs
such a frame: a 20 byte tagged frame on ip6gretap and a 10 byte frame
on ip6erspan are sent out on net, and dropped with the diff. An untagged
frame that is too short for its IP header is already dropped by 
pskb_inet_may_pull(); the diff only applies the same rule to tagged
frames.

The diff also fixes two problems I found while testing:

1. On transmit, skb->mac_len is still that of the receiving device,
   and the VLAN walk starts from it. A packet that came in on an NBMA
   ipgre device (mac_len 24) and is routed to a VLAN on ip6gretap has
   its walk start inside the inner IPv4 header. On net the tag is
   then missed and nothing is inherited; with skb_vlan_inet_prepare()
   alone, a crafted packet also gets its traffic class from the wrong
   byte. The diff clears skb->mac_len for Ethernet devices.

2. ip6gre without a remote has header_ops, so skb->data points at the
   pseudo header from ip6gre_header(). With true, the network header
   lands on it, and an ICMPv6 error then quotes its unwritten bytes
   (KMSAN in icmpv6_push_pending_frames()). The diff keeps
   pskb_inet_may_pull() there, as in net.

With the diff, gre_gso, l2_tos_ttl_inherit and
mirror_gre{,_vlan,_bridge_1q,_changes} pass. KMSAN, run with my
other GRE fixes on top, shows nothing on these paths.

About Fixes: v3 blames d8a6213d70ac ("geneve: fix header validation
in geneve[6]_xmit_skb"). That commit only added
skb_vlan_inet_prepare(); the bug in ip6_gre is older.

The length check, pskb_inet_may_pull(), looks at skb->protocol. For
a tagged frame that is ETH_P_8021Q, so the inner IP header is not
checked at all. This used to be fine: the code after the check also
used skb->protocol, so tagged frames went to ip6gre_xmit_other(),
which does not read the inner header.

Two later commits made the code look through the tags, but did not
change the check:

 - 3f8a8447fd0b ("ip6_gre: use actual protocol to select xmit") made
   ip6gre_tunnel_xmit() switch on skb_protocol(skb, true). Tagged
   IPv4 and IPv6 frames now go to ip6gre_xmit_ipv4() and
   ip6gre_xmit_ipv6(), which read the inner header.

 - b09ab9c92e50 ("ip6_tunnel: allow to inherit from VLAN encapsulated
   IP") did the same in ip6_tnl_xmit(), which reads the inner header
   to inherit the hop limit. ip6erspan goes through this path too.

Since then a short tagged frame passes the check, and its inner
header is read from bytes that were never pulled into the linear
data. That is the uninit-value syzbot reports.

skb_vlan_inet_prepare() walks the tags the same way as
skb_protocol(skb, true), pulls the inner IP header and points the
network header at it. So the check now covers what the code reads
later. That is why I would point Fixes at the two commits above:

Fixes: 3f8a8447fd0b ("ip6_gre: use actual protocol to select xmit")
Fixes: b09ab9c92e50 ("ip6_tunnel: allow to inherit from VLAN encapsulated IP")

The IPv4 side has the same problems: gre_tap_xmit(), erspan_xmit()
and ip_tunnel_rcv(). ip6_tnl_xmit() has one more: it walks the tags
after the GRE header is pushed. I will send these fixes separately;
the ip6_tnl_xmit() one depends on this patch.

Eric, would you like to respin with this, or should I send it as v4?

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index e61cb10b50dc..f48141820fb5 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -883,8 +883,22 @@ 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 {
+		/* The VLAN tag walks below start at skb->mac_len - VLAN_HLEN,
+		 * or at ETH_HLEN if it is 0, and 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;
@@ -934,7 +948,12 @@ static netdev_tx_t ip6erspan_tunnel_xmit(struct sk_buff *skb,
 	__u32 mtu;
 	int nhoff;
 
-	if (!pskb_inet_may_pull(skb))
+	/* The VLAN tag walks below start at skb->mac_len - VLAN_HLEN, or at
+	 * ETH_HLEN if it is 0, and 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))

-- 
Anton Danilov