Re: [PATCH net 1/3] net: virtio: validate outer protocol of UDP tunnel GSO packets

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: virtio: unreliable L3 check in virtio_net_tnl_outer_proto_ok()
--

commit 02f420dee7a050b3832ecf552c44f9c2eeaaddc1
Author: Eric Dumazet <edumazet@kernel.org>

net: virtio: validate outer protocol of UDP tunnel GSO packets

This commit validates that the outer L3 protocol of a UDP tunnel GSO packet
matches the tunnel type declared in the virtio header in
virtio_net_hdr_tnl_to_skb().

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index d6466f96cdd00..d3f97d71d81cc 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -282,6 +282,31 @@ static inline unsigned int virtio_l3min(bool is_ipv6)
>  	return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr);
>  }
>  
> +/* The outer network header of a UDP tunnel GSO packet must match the
> + * tunnel type declared in the virtio header, otherwise the GSO stack
> + * would parse the packet as something else (e.g. NSH).
> + */
> +static inline bool virtio_net_tnl_outer_proto_ok(const struct sk_buff *skb,
> +						 bool outer_isv6,
> +						 unsigned int outer_th)
> +{
> +	__be16 expected = outer_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
> +	__be16 proto = skb->protocol;
> +	int nh_off = ETH_HLEN;
> +
> +	/* L3 device (IFF_TUN): the caller already set the protocol. */
> +	if (proto)
> +		return proto == expected;

[Severity: High]
Can testing if (proto) here be bypassed in tun_get_user() in two ways?

First, on an IFF_TAP device with IFF_NAPI_FRAGS enabled, tun_get_user()
allocates the skb via tun_napi_alloc_frags() -> napi_get_frags(). When a
prior packet was merged by GRO (GRO_MERGED_FREE in napi_frags_finish()),
napi_reuse_skb() caches napi->skb for reuse without clearing skb->protocol
(which napi_frags_skb() set to the previous frame's eth->h_proto, such as
htons(ETH_P_IP)).

In drivers/net/tun.c:tun_get_user(), tun_vnet_hdr_tnl_to_skb() runs before
eth_type_trans() updates skb->protocol for the new frame:

	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
		atomic_long_inc(&tun->rx_frame_errors);
		err = -EINVAL;
		goto free_skb;
	}

	if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP)
		skb->protocol = eth_type_trans(skb, tun->dev);

With a reused napi->skb still carrying skb->protocol == htons(ETH_P_IP),
virtio_net_tnl_outer_proto_ok() takes the if (proto) branch and returns true
without inspecting eth_hdr(skb)->h_proto or checking nh_off +
virtio_l3min(outer_isv6) <= outer_th, and eth_type_trans() then sets
skb->protocol to the new frame's actual EtherType (such as ETH_P_NSH).

Second, on an IFF_TUN device without IFF_NO_PI, tun_get_user() copies struct
tun_pi from userspace and sets skb->protocol = pi.proto:

		skb_reset_network_header(skb);
		skb->protocol = pi.proto;
		break;

If userspace passes pi.proto == 0, proto is 0, so
virtio_net_tnl_outer_proto_ok() falls through to the Ethernet path and checks
eth_hdr(skb)->h_proto inside the L3 payload, leaving skb->protocol == 0 with
SKB_GSO_UDP_TUNNEL set if those bytes match expected.

Since all callers set skb->network_header before calling
virtio_net_hdr_tnl_to_skb() (to 0 via skb_reset_network_header() for IFF_TUN,
and to ETH_HLEN via skb_set_network_header() for IFF_TAP and virtio_net),
would checking !skb_network_offset(skb) instead of if (proto) avoid both
issues?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103203113.550604-1-edumazet@kernel.org?part=1