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

[PATCH net v5] 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 check and the skb_protocol() dispatch in
ip6gre_tunnel_xmit(). Three 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 TTL and traffic class inheritance for
a tagged frame on ip6gretap with a key or on ip6erspan is still taken
from the wrong offset; the ip6gre header_ops branch kept above still has
the check/parse mismatch once a remote is set with changelink; and
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>

---
v5: no functional change from v4; only the commit message and two
    comments. The AI review asked whether the check/parse mismatch is
    fully fixed here. I compared v4 against net on the cases it named:
    no regression. The ip6gretap-with-key and ip6erspan outer headers
    are unchanged, the forwarded ip6gretap-without-key case starts to
    inherit correctly, and the short tagged frames are now dropped.
    Spell out in the message and the comments what is and is not fixed
    here; ip6_tnl_xmit(), the header_ops branch and the IPv4 paths are
    follow-ups.
    Link: https://lore.kernel.org/netdev/179121211133.434549.1381649683777834470@kernel.org/
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 e61cb10b50dc..8b286bfd2c4c 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;
@@ -934,7 +949,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 v5] 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 2 potential
issue(s) to consider.

Critical: 0 · High: 2 (1) · Medium: 0 · 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] ip6erspan_tunnel_xmit() still lets short frames through,
  including short frames with a VLAN skb->protocol, and…
- [High] ip6gre_tunnel_xmit() still checks one thing and parses another
  in the header_ops branch the patch keeps.

Pre-existing issues:
- [High] ip6_tnl_xmit() recomputes `payload_protocol = skb_protocol(skb,
  true)` (net/ipv6/ip6_tunnel.c:1119) after __gre6_xmit() or…

--

ip6_gre: use skb_vlan_inet_prepare() instead of pskb_inet_may_pull()

This patch replaces pskb_inet_may_pull() with skb_vlan_inet_prepare()
in ip6gre_tunnel_xmit() and ip6erspan_tunnel_xmit(). The goal is that
the length check matches the VLAN-aware protocol parsing done later.
skb->mac_len is cleared for Ethernet devices first, and the old check
is kept for ip6gre devices that use ip6gre_header_ops.

> diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
> index e61cb10b50dc9..8b286bfd2c4cc 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;

[Severity: High]
The commit message says "This fixes the check and the skb_protocol()
dispatch in ip6gre_tunnel_xmit()". Can this branch still read an inner
header that was never pulled?

ip6gre_tunnel_init() sets header_ops only when no remote is configured:

	if (ipv6_addr_any(&tunnel->parms.raddr))
		dev->header_ops = &ip6gre_header_ops;

Nothing clears it when changelink later sets a remote. After that,
ip6_tnl_xmit_ctl() passes.

When skb->protocol is ETH_P_8021Q, pskb_inet_may_pull() takes the
default case with nhlen = 0. So nothing is pulled past the network
offset. The shared dispatch below then does:

	payload_protocol = skb_protocol(skb, true);

That walk starts at skb->data + ETH_HLEN, which is inside the
user-supplied pseudo IPv6 header pushed by ip6gre_header(). It can
return ETH_P_IPV6 or ETH_P_IP.

The selected handlers then read the inner header without any further
length check:

  - ip6gre_xmit_ipv6() reads ipv6h->saddr
  - ip6_tnl_parse_tlv_enc_lim() reads nexthdr
  - prepare_ip6gre_xmit_ipv6() reads tclass and flowlabel
  - ip6gre_xmit_ipv4() reads the dsfield
  - ip6_tnl_xmit() reads the ttl or hop_limit when it is inherited

With PACKET_TX_RING, tpacket_fill_skb() copies only hard_header_len
into the linear area and puts the rest in page frags. Would these reads
then come from uninitialized tailroom, with some of the bits copied
into the outer IPv6 header on the wire?

The commit message lists this branch as a follow-up. Should the "This
fixes the check and the skb_protocol() dispatch" sentence be narrowed
to say this case is not covered yet?

> +	} 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: High]
This is a pre-existing issue, and the commit message lists
ip6_tnl_xmit() as a follow-up. Even so, can a short tagged frame whose
real inner type is not IP still cause an uninitialized read after this
check?

Take an 18-byte tagged ARP frame on a keyed ip6gretap device (or on
ip6erspan). skb_vlan_inet_prepare() succeeds with maclen = 18 and
nhlen = 0, and it leaves the network header at the end of the data.

After __gre6_xmit() (or ip6erspan_tunnel_xmit()) pushes the GRE header
(and the ERSPAN header), ip6_tnl_xmit() parses the protocol again:

	payload_protocol = skb_protocol(skb, true);

skb->mac_len is now 0, so this walk starts at ETH_HLEN from the GRE
header. The encapsulated protocol is then read from one of these
places:

  - inner bytes 8-9 (the source MAC) on keyed ip6gretap
  - inner bytes 0-1 (the destination MAC) on ip6erspan v1

The sender controls both.

With ttl inherit, ip6_tnl_xmit() then does:

		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;

That reads 7 or 8 bytes past the skb tail. Can that uninitialized byte
go out on the wire as the outer hop limit?

The commit message describes this case as inheritance that "is still
taken from the wrong offset". Would it be more accurate to say it can
also read past the end of the packet?

>  
>  	if (!ip6_tnl_xmit_ctl(t, &t->parms.laddr, &t->parms.raddr))
>  		goto tx_err;
> @@ -934,7 +949,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;

[Severity: High]
Does this check still let short frames reach erspan_build_header() and
erspan_build_header_v2()?

If skb->protocol is a VLAN type but the tag is truncated,
__vlan_get_protocol() fails inside skb_vlan_inet_prepare(). type
becomes 0 and maclen stays at ETH_HLEN, so only 14 bytes have to be
present:

	if (eth_type_vlan(type))
		type = __vlan_get_protocol(skb, type, &maclen);
	...
	reason = pskb_may_pull_reason(skb, maclen + nhlen);

A non-IP frame likewise only needs maclen bytes, which is ETH_HLEN, or
ETH_HLEN plus the tags.

Both ERSPAN builders always read the first two bytes at the network
header:

	tos = is_ipv4 ? ip_hdr(skb)->tos :
			(ipv6_hdr(skb)->priority << 4) +
			(ipv6_hdr(skb)->flow_lbl[0] >> 4);

When the in-frame h_proto is 0x8100, they also read the TCI at
skb->data + 14:

	if (eth->h_proto == htons(ETH_P_8021Q)) {
		qp = (struct qtag_prefix *)(skb->data + 2 * ETH_ALEN);
		vlan_tci = ntohs(qp->tci);

These frames would pass the new check and then cause reads up to 2
bytes past skb->len:

  - a 14 or 15 byte frame sent through AF_PACKET with
    sll_protocol = ETH_P_8021Q
  - a 14 or 15 byte non-IP frame
  - an 18 or 19 byte tagged non-IP frame

Can this put uninitialized tailroom into ershdr->cos, and into the
ERSPAN VLAN field when the in-frame ethertype is 0x8100?

The commit message says the short ip6erspan frame is now dropped. This
looks like the same over-read, just a few bytes higher.

[ ... ]

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

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

[Anton Danilov]

On Wed, Oct 07, 2026 at 11:16:37PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.

Thanks. All three reads are real, and all three are the same without
this patch; numbers below. The frames are written to the device through
an AF_PACKET socket, on net (9b6c20f78847) and on net with this patch;
the receiving end runs KMSAN and reads the packets with a packet socket.
"reports" is the number of KMSAN reports there (kernel-infoleak in the
packet socket, or uninit-value in the ICMPv6 error that quotes the
packet).

[...]
>> +	if (dev->type != ARPHRD_ETHER && dev->header_ops) {
[...]
> [Severity: High]
> The commit message says "This fixes the check and the skb_protocol()
> dispatch in ip6gre_tunnel_xmit()". Can this branch still read an inner
> header that was never pulled?
[...]
> The commit message lists this branch as a follow-up. Should the "This
> fixes the check and the skb_protocol() dispatch" sentence be narrowed
> to say this case is not covered yet?

Yes. That branch keeps pskb_inet_may_pull() byte for byte as net has
it, and it is reachable only on an ip6gre device created without a
remote and later given one with changelink. v6 narrows the sentence to
ip6gretap and ip6gre devices created with a remote; the branch stays
listed as a follow-up.

>> +		if (skb_vlan_inet_prepare(skb, dev->type != ARPHRD_ETHER))
[...]
> [Severity: High]
> This is a pre-existing issue, and the commit message lists
> ip6_tnl_xmit() as a follow-up. Even so, can a short tagged frame whose
> real inner type is not IP still cause an uninitialized read after this
> check?
[...]
> That reads 7 or 8 bytes past the skb tail. Can that uninitialized byte
> go out on the wire as the outer hop limit?

It can, on net and with this patch alike. The 18 byte tagged ARP frame
from the review, with 0x0800 at bytes 8-9 (ip6gretap with a key) or at
bytes 0-1 (ip6erspan v1), both devices with hoplimit inherit:

                                     net             + patch
  ip6gretap, bytes 8-9 = 0x0800      hop limit 0     hop limit 0 or 173
                                     15 reports      15 reports
  ip6gretap, bytes 8-9 = 0x86DD      0, 15           0, 15
  ip6gretap, control (no 0x0800)     64, 0           64, 0
  ip6erspan, bytes 0-1 = 0x0800      0, 17           0, 17
  ip6erspan, control                 64, 15          64, 15

The hop limit is whatever lies past the end of the frame: 0 in three
runs, 173 in one, with KMSAN reporting it as uninitialized on net and
with the patch alike. The 15 reports on the ip6erspan control frame
are the TOS that erspan_build_header() reads past the end, see below.
With the follow-up that makes ip6_tnl_xmit() walk the tags from the
inner MAC header (it depends on this patch and will follow it) all
rows give hop limit 64 and 0 reports.

> The commit message describes this case as inheritance that "is still
> taken from the wrong offset". Would it be more accurate to say it can
> also read past the end of the packet?

Yes; v6 says so.

>> +	skb->mac_len = 0;
>> +	if (skb_vlan_inet_prepare(skb, false))
[...]
> [Severity: High]
> Does this check still let short frames reach erspan_build_header() and
> erspan_build_header_v2()?
[...]
> Can this put uninitialized tailroom into ershdr->cos, and into the
> ERSPAN VLAN field when the in-frame ethertype is 0x8100?

It does, on net as well. The check here is for the inner IP header,
which is what the skb_protocol() dispatch reads; the TOS and TCI reads
in erspan_build_header{,_v2}() have had no length check of their own
since 84e54fe0a5ea, and this patch only moves the minimum from 10 to
14 bytes:

  ip6erspan v1                        net          + patch
  14B frame, EtherType 0x8100         18 reports   18 reports
  14B non-IP frame                    15           15
  18B tagged non-IP frame             15           15
  10B frame via tun + mirred          3            dropped

The 14 byte frame with 0x8100 is reported as "Bytes 62-63 of 84 are
uninitialized": the ver/vlan word of the ERSPAN header. That is a
separate fix with its own Fixes tags: two patches, queued after this
one, that read the TCI with skb_header_pointer() and take the COS from
the frame's own protocol; with them the rows above give 0 reports.

> The commit message says the short ip6erspan frame is now dropped. This
> looks like the same over-read, just a few bytes higher.

Agreed. v6 lists erspan_build_header() as a follow-up instead of
leaving "the short ip6erspan frame is now dropped" to suggest that all
short frames are.

v6 changes the commit message only; the code is identical to v4 and v5.

pw-bot: cr

---

Anton Danilov