Re: [PATCH net v6 1/2] net: validate virtio checksum start after network header

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 3

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).

- [Medium] Undisclosed tightening of an existing uAPI in a stable-tagged
  patch.
- [Low] Commit message does not describe the actual defect and states no
  user-visible symptom for a Fixes:/Cc: stable patch.
- [Low] The new Ethernet/VLAN parser invoked from raw_verify_header()
  (arch/um/drivers/vector_transports.c) treats the skb's current length…
- [Low] Reported for completeness, but the claimed bypass does not reach
  the asserted malformed geometry.

--

net: validate virtio checksum start after network header

This passes a data-relative L3 offset and L3 protocol into
__virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb(), adds an
Ethernet/VLAN parser (virtio_net_hdr_eth_get_l3_offset()) used by
AF_PACKET, tap/tun, virtio_net and the UML vector raw transport, and
tightens the CHECKSUM_PARTIAL minimum-header check accordingly.

Link: https://lore.kernel.org/netdev/20260920004733.6473-2-habte.yibelo@gmail.com/

A few questions below.

The changelog says:

> __virtio_net_hdr_to_skb() checks a minimum network-header length for
> CHECKSUM_PARTIAL packets. Its checksum start is relative to skb->data,
> but some callers have not established skb->network_header when they
> convert the virtio header.

[Severity: Low]
Is this the actual defect?  The pre-patch test in __virtio_net_hdr_to_skb()
is:

	if (skb_transport_offset(skb) < nh_min_len)
		return -EINVAL;

	nh_min_len = skb_transport_offset(skb);

which never reads skb->network_header, so an unestablished network header
does not affect it.

What the old test misses looks different: the minimum is measured from
skb->data, which for the Ethernet-framed callers (AF_PACKET, tap,
virtio_net, UML raw/tap) is the MAC header, so a csum_start of 20..33 passes
while leaving fewer than 20 bytes of L3 header.

Also, for AF_PACKET the caller does establish the network header before
conversion:

net/packet/af_packet.c:packet_parse_headers() {
	...
	if (likely(skb->dev->type == ARPHRD_ETHER) &&
	    eth_type_vlan(skb->protocol) &&
	    vlan_get_protocol_and_depth(skb, skb->protocol, &depth) != 0)
		skb_set_network_header(skb, depth);
}

Could the changelog describe the data-relative minimum instead of the
network header, and also state the user-visible symptom?  With two Fixes:
tags and Cc: stable, and Reported-by: naming the author, there is no
reproducer or symptom given.  The referenced commit 49d14b54a527 documented
its own symptom (the "offset (-6) >= skb_headlen() (14)" warning from
skb_checksum_help() via ip_do_fragment()); is the same warning, a
mis-placed checksum, or something else being fixed here?

> diff --git a/arch/um/drivers/vector_transports.c b/arch/um/drivers/vector_transports.c
> index ddd127ee96785..5a9d8f49a64ba 100644
> --- a/arch/um/drivers/vector_transports.c
> +++ b/arch/um/drivers/vector_transports.c
> @@ -197,6 +197,8 @@ static int raw_verify_header(
>  	uint8_t *header, struct sk_buff *skb, struct vector_private *vp)
>  {
>  	struct virtio_net_hdr *vheader = (struct virtio_net_hdr *) header;
> +	__be16 network_protocol;
> +	int network_offset;
>  
>  	if ((vheader->gso_type != VIRTIO_NET_HDR_GSO_NONE) &&
>  		(vp->req_size != 65536)) {
> @@ -209,8 +211,14 @@ static int raw_verify_header(
>  	if ((vheader->flags & VIRTIO_NET_HDR_F_DATA_VALID) > 0)
>  		return 1;
>  
> -	virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());
> -	return 0;
> +	network_offset = virtio_net_hdr_eth_get_l3_offset(skb, vheader,
> +							  &network_protocol);
> +	if (network_offset < 0)
> +		return network_offset;

[Severity: Low]
Does this parse bytes that have not been received yet?  Both UML receive
callers run verify_header() before the skb is trimmed to the recvmsg
result, and prep_skb() has already set the length to the full buffer
capacity:

arch/um/drivers/vector_kern.c:prep_skb() {
	...
	skb_put(result, vp->max_packet);
	result->data_len = len - vp->max_packet;
	result->len += len - vp->max_packet;
	...
}

arch/um/drivers/vector_kern.c:vector_legacy_rx() {
	...
	header_check = vp->verify_header(vp->header_rxbuffer, skb, vp);
	...
	pskb_trim(skb, pkt_len - vp->rx_header_size);
	...
}

vector_mmsg_rx() has the same ordering.

virtio_net_hdr_eth_get_l3_offset() bounds its reads with
skb_header_pointer(), i.e. against skb_headlen(), so for a NEEDS_CSUM frame
that was truncated inside its Ethernet or nested VLAN headers the parser
reads stale bytes of the freshly allocated head rather than reporting a
short header.  The resulting network_offset/network_protocol, and now the
return value of raw_verify_header(), then come from uninitialized buffer
contents.

Would it make sense for the callers to establish the received length before
calling verify_header()?

> +
> +	return virtio_net_hdr_to_skb(skb, vheader,
> +				     virtio_legacy_is_little_endian(),
> +				     network_offset, network_protocol);
>  }
>  
>  static bool get_uint_param(

[ ... ]

> diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h
> index f4c652b1fa44d..e004213fad22c 100644
> --- a/drivers/net/tun_vnet.h
> +++ b/drivers/net/tun_vnet.h
> @@ -177,10 +177,49 @@ static inline int tun_vnet_hdr_put(int sz, struct iov_iter *iter,
>  	return __tun_vnet_hdr_put(sz, 0, iter, hdr);
>  }
>  
> +static inline int
> +tun_vnet_hdr_get_l3_offset(unsigned int flags, const struct sk_buff *skb,
> +			   const struct virtio_net_hdr *hdr,
> +			   __be16 *network_protocol)
> +{
> +	if ((flags & TUN_TYPE_MASK) != IFF_TAP) {
> +		u8 version, first_byte;
> +		const u8 *first;
> +
> +		*network_protocol = 0;
> +		if (!(hdr->flags & VIRTIO_NET_HDR_F_NEEDS_CSUM))
> +			return 0;
> +
> +		first = skb_header_pointer(skb, 0, sizeof(first_byte),
> +					   &first_byte);
> +		if (!first)
> +			return -EINVAL;
> +
> +		version = *first >> 4;
> +		if (version == 4)
> +			*network_protocol = htons(ETH_P_IP);
> +		else if (version == 6)
> +			*network_protocol = htons(ETH_P_IPV6);
> +		return 0;
> +	}

[Severity: Low]
For IFF_TUN this derives network_protocol from the payload version nibble,
while skb->protocol is set later from the user-supplied tun_pi.proto:

drivers/net/tun.c:tun_get_user() {
	...
	case IFF_TUN:
		if (tun->flags & IFF_NO_PI) {
			...
			ip_version = skb->data[0] >> 4;
			...
		}
		skb_reset_mac_header(skb);
		skb->protocol = pi.proto;
	...
}

The nibble/pi.proto consistency check only runs under IFF_NO_PI, so a
writer can declare pi.proto = ETH_P_IPV6 with a first nibble of 4, keeping
nh_min_len at sizeof(struct iphdr) while the stack labels the skb IPv6.

That particular combination looks harmless in practice, since
ip6_rcv_core() drops it before any header parsing:

net/ipv6/ip6_input.c:ip6_rcv_core() {
	...
	if (hdr->version != 6) {
		SKB_DR_SET(reason, UNHANDLED_PROTO);
		goto err;
	}
	...
}

and a genuine IPv6 payload always yields nibble 6 and thus the intended
40-byte minimum.  Still, is keying the length bound off the payload nibble
while the rest of the stack acts on pi.proto intentional here?

> +
> +	return virtio_net_hdr_eth_get_l3_offset(skb, hdr,
> +					      network_protocol);
> +}
> +

[ ... ]

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e34c52d059d39..8e93dad287911 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2515,9 +2517,13 @@ static void virtnet_receive_done(struct virtnet_info *vi, struct receive_queue *
>  		goto frame_err;
>  	}
>  
> -	if (virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
> +	network_offset = virtio_net_hdr_eth_get_l3_offset(skb, &hdr->hdr,
> +							  &network_protocol);
> +	if (network_offset < 0 ||
> +	    virtio_net_hdr_tnl_to_skb(skb, &hdr->tnl_hdr, vi->rx_tnl,
>  				      vi->rx_tnl_csum,
> -				      virtio_is_little_endian(vi->vdev))) {
> +				      virtio_is_little_endian(vi->vdev),
> +				      network_offset, network_protocol)) {
>  		net_warn_ratelimited("%s: bad gso: type: %x, size: %u, flags %x tunnel %d tnl csum %d\n",
>  				     dev->name, hdr->hdr.gso_type,
>  				     hdr->hdr.gso_size, hdr->hdr.flags,
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index c381b916c1b54..b89d821a0dd2d 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -48,16 +48,61 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,

[ ... ]

>  static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
>  					  const struct virtio_net_hdr *hdr,
> -					  bool little_endian, u8 hdr_gso_type)
> +					  bool little_endian, u8 hdr_gso_type,
> +					  int network_offset,
> +					  __be16 network_protocol)
>  {
> -	unsigned int nh_min_len = sizeof(struct iphdr);
> +	int nh_min_len = sizeof(struct iphdr);
>  	unsigned int gso_type = 0;
>  	unsigned int thlen = 0;
>  	unsigned int p_off = 0;
>  	unsigned int ip_proto;
>  
> +	if (network_protocol == htons(ETH_P_IPV6))
> +		nh_min_len = sizeof(struct ipv6hdr);
> +

[ ... ]

> @@ -98,16 +143,20 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
>  		u32 start = __virtio16_to_cpu(little_endian, hdr->csum_start);
>  		u32 off = __virtio16_to_cpu(little_endian, hdr->csum_offset);
>  		u32 needed = start + max_t(u32, thlen, off + sizeof(__sum16));
> +		int transport_offset;
>  
>  		if (!pskb_may_pull(skb, needed))
>  			return -EINVAL;
>  
>  		if (!skb_partial_csum_set(skb, start, off))
>  			return -EINVAL;
> -		if (skb_transport_offset(skb) < nh_min_len)
> +
> +		transport_offset = skb_transport_offset(skb);
> +		if (transport_offset < nh_min_len || network_offset < 0 ||
> +		    network_offset > transport_offset - nh_min_len)
>  			return -EINVAL;

[Severity: Medium]
Does this change the accepted csum_start range for existing users, and
should the changelog say so?

The floor moves from 20 (relative to skb->data) to parsed_L3_offset + 20 or
40: 34 for a plain Ethernet frame, 54 for Ethernet plus IPv6, and more with
VLAN or QinQ.

Three concrete behaviour changes:

- AF_PACKET sendmsg() and tpacket_snd() now return -EINVAL for virtio
  headers with csum_start in 20..33 that were accepted before.

- The new

	if (network_protocol == htons(ETH_P_IPV6))
		nh_min_len = sizeof(struct ipv6hdr);

  applies the 40-byte minimum to any frame whose parsed ethertype is
  ETH_P_IPV6, whereas previously only VIRTIO_NET_HDR_GSO_TCPV6 did that.

- On the receive side, virtnet_receive_done() now takes the frame_err path
  (DEV_STATS_INC(rx_frame_errors) plus dev_kfree_skb) for host descriptors
  that were accepted before.

There is also the non-IP case: when the parsed ethertype is neither
ETH_P_IPV4 nor ETH_P_IPV6, nh_min_len stays sizeof(struct iphdr), so a
non-IP Ethernet frame whose checksum field sits close to the L2 header is
rejected even though it has no 20-byte L3 header at all.  Is that
intended?

Given the Cc: stable, would it be worth stating the new accept/reject
boundary in the changelog and adding a selftest that pins it?

> -		nh_min_len = skb_transport_offset(skb);
> +		nh_min_len = transport_offset;
>  		p_off = nh_min_len + thlen;
>  		if (!pskb_may_pull(skb, p_off))
>  			return -EINVAL;

[ ... ]

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 50cae32ae2692..0b37d5474f868 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -3103,7 +3123,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>  	packet_parse_headers(skb, sock);
>  
>  	if (vnet_hdr_sz) {
> -		err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le());
> +		err = packet_vnet_hdr_to_skb(skb, &vnet_hdr);
>  		if (err)
>  			goto out_free;
>  		len += vnet_hdr_sz;

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