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

[netdev-bot+sashiko@kernel.org]

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