Re: [PATCH net v2 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: L2 check bypass in virtio_net_l3_start() via TUNSETLINK
--

commit 546d87e0158fb3d32de1c338228f7d9a8f02bb4e
Author: Eric Dumazet <edumazet@kernel.org>

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

This commit validates the outer L2/L3/L4 headers of UDP tunnel GSO packets in
virtio_net_hdr_tnl_to_skb() so that frames carrying mismatched outer protocols
or offsets are rejected before GSO segmentation.

Link: https://lore.kernel.org/netdev/93d0e37c99711bebdf60c120af03010455c2f2e2.1793276116.git.mst@redhat.com/

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index d6466f96cdd00..b9c8babb1fd73 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
[ ... ]
> +/* Where does the L3 header start, and what protocol is it?
> + * Handles Ethernet and VLAN headers; other devices use the
> + * network header offset the caller set up.
> + */
> +static inline bool
> +virtio_net_l3_start(struct sk_buff *skb, int *nhoff, __be16 *proto)
> +{
> +	/* skb->dev should always be set here already,
> +	 * but let's not crash when it is not.
> +	 */
> +	if (WARN_ON_ONCE(!skb->dev))
> +		return false;
> +
> +	if (skb->dev->type == ARPHRD_ETHER ||
> +	    skb->dev->type == ARPHRD_LOOPBACK) {
> +		__be16 h_proto;
> +
> +		if (!pskb_may_pull(skb, ETH_HLEN))
> +			return false;
> +		h_proto = eth_hdr(skb)->h_proto;
> +		/* skb->protocol is a sender claim, and parts of the
> +		 * stack parse the packet per it: when it disagrees with
> +		 * the frame we do not know which view the packet will
> +		 * be parsed as, so do not validate.
> +		 */
> +		if (skb->protocol && skb->protocol != h_proto)
> +			return false;
> +		*nhoff = ETH_HLEN;
> +		*proto = h_proto;
> +		if (eth_type_vlan(*proto)) {
> +			*proto = __vlan_get_protocol(skb, *proto, nhoff);
> +			if (!*proto)
> +				return false;
> +		}
> +	} else {
> +		const struct iphdr *iph;
> +
> +		*nhoff = skb_network_offset(skb);
> +		/* An unset network header shows up as an offset
> +		 * outside of the packet.
> +		 */
> +		if (*nhoff < 0 || (unsigned int)*nhoff >= skb->len)
> +			return false;
> +		/* No L2 header to cross-check with: take the protocol
> +		 * from the IP version at the network header.
> +		 */
> +		if (!pskb_may_pull(skb, *nhoff + 1))
> +			return false;
> +		iph = (const struct iphdr *)(skb->data + *nhoff);
> +		if (iph->version == 4)
> +			*proto = htons(ETH_P_IP);
> +		else if (iph->version == 6)
> +			*proto = htons(ETH_P_IPV6);
> +		else
> +			return false;
> +		if (skb->protocol && skb->protocol != *proto)
> +			return false;
> +	}
> +	return true;
> +}

[Severity: High]
Can a tap writer bypass the Ethernet header check in virtio_net_l3_start() by
changing tun->dev->type via the TUNSETLINK ioctl?

In drivers/net/tun.c, __tun_chr_ioctl() allows changing tun->dev->type while
the device is down without changing tun->flags:

drivers/net/tun.c:__tun_chr_ioctl() {
	case TUNSETLINK:
		...
		tun->dev->type = (int) arg;
		tun->dev->addr_len = tun_get_addr_len(tun->dev->type);
}

Meanwhile, tun_get_user() (and tun_xdp_one()) decides whether the frame starts
with an Ethernet header based on (tun->flags & TUN_TYPE_MASK) == IFF_TAP,
setting the network header to ETH_HLEN before calling
tun_vnet_hdr_tnl_to_skb(), and only populating skb->protocol via
eth_type_trans() afterwards:

drivers/net/tun.c:tun_get_user() {
	switch (tun->flags & TUN_TYPE_MASK) {
	...
	case IFF_TAP:
		...
		skb_set_network_header(skb, ETH_HLEN);
		break;
	}

	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
		...
	}

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

If an IFF_TAP device has tun->dev->type set to another type such as
ARPHRD_NONE via TUNSETLINK, virtio_net_l3_start() takes the else branch, reads
*nhoff = skb_network_offset(skb) (which is ETH_HLEN), derives *proto from the
IP version nibble at offset 14, and skips the skb->protocol check because
skb->protocol is still 0.

The Ethernet header at offset 0 is never inspected by virtio_net_l3_start(),
so eth_type_trans() will subsequently set skb->protocol from the unvalidated
eth_hdr(skb)->h_proto (for example ETH_P_NSH) on a packet with
SKB_GSO_UDP_TUNNEL set, triggering the same skb_under_panic() during GSO
segmentation.

Conversely, on an IFF_TUN device whose tun->dev->type is changed to
ARPHRD_ETHER via TUNSETLINK, virtio_net_l3_start() validates a 14-byte
Ethernet header at offset 0 and an outer IP header at offset 14, but
tun_get_user() does not call eth_type_trans() and resets skb->network_header
to offset 0, leaving the outer network header pointing at the fake Ethernet
header.

Should tun_get_user() and tun_xdp_one() set skb->protocol before calling
tun_vnet_hdr_tnl_to_skb(), or should virtio_net_l3_start() avoid relying on
skb->dev->type matching the caller's L2 framing?

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