Re: [PATCH net v4] 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 · Medium: 0 (2) · 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] The check/parse mismatch the commit says it fixes is still
  reachable in ip6gre_tunnel_xmit() on ip6gre devices that have…
- [High] One of the parses this patch is meant to protect is the one
  added by Fixes: b09ab9c92e50, i.e. payload_protocol =…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the IPv4 GRE
  transmit paths have the same check/parse mismatch, and the patch…
- [Medium] This problem wasn't introduced by this patch, but on
  ip6erspan, frames that are not resolved to IPv4/IPv6 pass the new…

--

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(). Before the check it
clears skb->mac_len on Ethernet devices. pskb_inet_may_pull() is still
used for ip6gre devices that have 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 e61cb10b50dc9..f48141820fb5a 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;

[Severity: High]
Can the VLAN check/parse mismatch still be reached on this branch?

ip6gre_tunnel_init() installs header_ops only when the device is created
without a remote:

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

Nothing clears it later. If ip6gre_changelink()->ip6gre_tnl_change() then
sets a remote, ip6gre_tnl_link_config_common() recomputes
IP6_TNL_F_CAP_XMIT. ip6_tnl_xmit_ctl() then passes, and packets still take
this branch.

When skb->protocol == ETH_P_8021Q, pskb_inet_may_pull() uses nhlen = 0 and
checks nothing. The dispatch further down still does:

    payload_protocol = skb_protocol(skb, true);

With mac_len == 0, that walk reads a vlan_hdr at skb->data + ETH_HLEN. That
location is inside the pseudo IPv6 header that ip6gre_header() pushed. The
encapsulated protocol it returns is saddr bytes 8-9, and those come from
t->parms.laddr:

    ipv6h->saddr = t->parms.laddr;

So if the user picks a laddr such as 2001:db8:0:0:86dd::1, the walk returns
ETH_P_IPV6.

One way to get there is AF_PACKET SOCK_DGRAM with sll_protocol set to
htons(ETH_P_8021Q), a non-zero sll_addr and an empty payload.
packet_snd() resets the network header before dev_hard_header(), so the
network header points at the empty payload. ip6gre_xmit_ipv6() then reads
past skb->tail:

    if (ipv6_addr_equal(&t->parms.raddr, &ipv6h->saddr))
        return -1;

prepare_ip6gre_xmit_ipv6() also reads nexthdr, dsfield and flowlabel from
the same unpulled bytes. If the protocol is ETH_P_IP instead,
prepare_ip6gre_xmit_ipv4() reads the tos.

KMSAN would report uninit-value here. With USE_ORIG_TCLASS or
USE_ORIG_FLOWLABEL, the stale bytes can be copied into the outer header.

There is a separate case on the same kind of device. When it receives a
frame through tc mirred or bpf redirect, ip6gre_header() is skipped, and
__gre6_xmit() reads this without any length guarantee:

    if (dev->header_ops && dev->type == ARPHRD_IP6GRE)
        fl6->daddr = ((struct ipv6hdr *)skb->data)->daddr;

NBMA devices that still have no remote don't seem to be affected.
ip6_tnl_get_cap() returns only CAP_PER_PACKET when raddr is any, so
ip6_tnl_xmit_ctl() rejects them.

The commit message names this mismatch in ip6gre_tunnel_xmit(). Does this
branch need a VLAN-aware check measured from the existing network offset?
Alternatively, should VLAN ethertypes not be treated as IP here?

> +	} 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;
> +	}

[Severity: High]
Does clearing mac_len here match the walk in ip6_tnl_xmit() that the second
Fixes tag (b09ab9c92e50) added?

That walk runs after the tunnel headers have been pushed:

__gre6_xmit()
    gre_build_header()
        skb_push(skb, hdr_len);
    ip6_tnl_xmit()
        payload_protocol = skb_protocol(skb, true);

skb_push() moves skb->data but does not change skb->protocol or mac_len.
With mac_len now 0, __vlan_get_protocol_offset() reads its vlan_hdr at
skb->data + ETH_HLEN, counted from the GRE header. skb_vlan_inet_prepare()
checked the Ethernet frame instead.

With a 4-byte GRE header the read happens to land on the TPID. In the
other cases it reads MAC address bytes:

  - tun_hlen 8 (key or seq): source MAC bytes 2-3
  - tun_hlen 12: destination MAC bytes 4-5
  - ip6erspan, after 16 pushed bytes: destination MAC bytes 0-1

Take ip6gretap with a key and an 18-byte VLAN-tagged ARP frame.
skb_vlan_inet_prepare() pulls 18 bytes and lets it through. If the source
MAC is xx:xx:08:00:xx:xx, ip6_tnl_xmit() treats payload_protocol as
ETH_P_IP.

hop_limit is 0 by default over netlink when IFLA_GRE_TTL is omitted, and
also for a collect_md key with ttl 0. In that case ip6_tnl_xmit() does:

    if (hop_limit == 0) {
        if (payload_protocol == htons(ETH_P_IP))
            hop_limit = ip_hdr(skb)->ttl;

This reads past the end of the frame and puts that byte in the outer IPv6
header.

The same applies to ip6erspan_tunnel_xmit(). There, the ip6_tnl_xmit()
walk is the only VLAN-aware parse after the check. So the base described
in the new comment ("The VLAN tag walks below start at ...") doesn't seem
to hold for it.

The NBMA daddr parsing in ip6_tnl_xmit() can't be reached here.
ip6_tnl_xmit_ctl() drops raddr == any first, and collect_md skips that
block.

Should the payload protocol be computed before the header push, or taken
from the network header that skb_vlan_inet_prepare() set?

>  
>  	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;

[Severity: Medium]
This is a pre-existing issue, but on ip6erspan, frames that don't resolve
to IPv4 or IPv6 still get through with only ETH_HLEN bytes guaranteed.

skb_vlan_inet_prepare(skb, false) starts with maclen = ETH_HLEN and adds
nhlen only for IP. For a VLAN-tagged frame of 14 to 17 bytes,
__vlan_get_protocol_offset() fails in skb_header_pointer(), and
vlan_get_protocol_offset_inline() leaves depth unset:

    if (depth && res.type)
        *depth = res.depth;

So pskb_may_pull(14) succeeds, and the network header is set at offset 14.

ip6erspan_tunnel_xmit() then calls erspan_build_header() or
erspan_build_header_v2(), which do:

    tos = is_ipv4 ? ip_hdr(skb)->tos :
            (ipv6_hdr(skb)->priority << 4) +
            (ipv6_hdr(skb)->flow_lbl[0] >> 4);
    ...
    if (eth->h_proto == htons(ETH_P_8021Q)) {
        qp = (struct qtag_prefix *)(skb->data + 2 * ETH_ALEN);
        vlan_tci = ntohs(qp->tci);

Both reads hit skb->data[14..15], past skb->len, and the stale bytes are
sent in the ERSPAN COS and VLAN fields. The tos read also happens for an
untagged 14-byte non-IP frame.

This patch is still an improvement here: before it, the 10-byte frame
from the commit message was not dropped either.

Should ip6erspan_tunnel_xmit() require enough bytes for these reads in
erspan_build_header()?

[ ... ]

[Severity: Medium]
This isn't a bug introduced by this patch, but do the IPv4 GRE transmit
paths have the same check/parse mismatch?

ipgre_xmit(), erspan_xmit() and gre_tap_xmit() in net/ipv4/ip_gre.c still
begin with:

    if (!pskb_inet_may_pull(skb))
        goto free_skb;

That check uses nhlen = 0 for ETH_P_8021Q and ETH_P_8021AD.
ip_tunnel_xmit() then does payload_protocol = skb_protocol(skb, true). If
the walk returns IPv4 or IPv6, it reads
inner_iph = skb_inner_network_header(skb). By then
gre_handle_offloads()->iptunnel_handle_offloads()->skb_reset_inner_headers()
has made that equal to the network header.

The fields it reads include tos (tos inherit), frag_off (unless ignore_df),
ttl or hop_limit (ttl inherit) and, on NBMA, daddr:

    if (payload_protocol == htons(ETH_P_IP) && !tunnel->ignore_df)
        df |= (inner_iph->frag_off & htons(IP_DF));
    ...
        if (payload_protocol == htons(ETH_P_IP))
            ttl = inner_iph->ttl;

For example, send an 18-byte tagged frame with AF_PACKET SOCK_RAW on
gretap. packet_parse_headers() sets the network header at 18, so the
frag_off and ttl reads land past skb->len. Native IPv4 erspan v1 also
reads ip_hdr(skb)->tos in erspan_build_header().

The IPv4 side came from the IPv4 counterpart of b09ab9c92e50. Was ip_gre.c
looked at as part of this change, or would it need a similar follow-up?

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