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

[Anton Danilov <littlesmilingcloud@gmail.com>]

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