Re: [PATCH v2 net 1/2] net: always dissect GSO packets in __virtio_net_hdr_to_skb()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0 (1)

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] `__skb_flow_dissect()` sets `key_control->thoff = min_t(u16,
  nhoff, skb ?

Pre-existing issues:
- [Medium] In af_packet, `packet_parse_headers()` runs before
  `virtio_net_hdr_to_skb()` (`packet_snd()` lines 3130/3133;…
- [Low] In the UML vector driver, `prep_skb()` sets `skb->len` to the
  full receive-buffer capacity (`skb_put(result, vp->max_packet)`, plus…

--

net: always dissect GSO packets in __virtio_net_hdr_to_skb()

This patch drops the skb->network_header condition and the unvalidated
fallback in __virtio_net_hdr_to_skb(). Every GSO packet without
VIRTIO_NET_HDR_F_NEEDS_CSUM is now flow-dissected and gets a transport
header. The tun, virtio_net and UML vector callers now set skb->dev and
skb->network_header before the conversion, and the
virtio_net_hdr_match_proto() check now runs after dissection.

Closes: https://lore.kernel.org/netdev/20260927163117.746432-2-bestswngs@gmail.com/

> diff --git a/arch/um/drivers/vector_transports.c b/arch/um/drivers/vector_transports.c
> index ddd127ee9678..e1fb2a76fdff 100644
> --- a/arch/um/drivers/vector_transports.c
> +++ b/arch/um/drivers/vector_transports.c
> @@ -209,6 +209,7 @@ static int raw_verify_header(
>  	if ((vheader->flags & VIRTIO_NET_HDR_F_DATA_VALID) > 0)
>  		return 1;
>  
> +	skb_set_network_header(skb, ETH_HLEN);
>  	virtio_net_hdr_to_skb(skb, vheader, virtio_legacy_is_little_endian());

[Severity: Low]
This isn't a bug introduced by this patch, but raw_verify_header() now
takes the dissection path. At this point, isn't skb->len still the full
receive buffer capacity rather than the length actually received?

prep_skb() sizes the skb up front:

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

Both vector_legacy_rx() and vector_mmsg_rx() call verify_header() before
pskb_trim() cuts the skb down to the received length:

vector_legacy_rx()
    header_check = vp->verify_header(vp->header_rxbuffer, skb, vp);
    ...
    pskb_trim(skb, pkt_len - vp->rx_header_size);

So the dissection, pskb_may_pull() of thoff + thlen and
skb_set_transport_header() all work on stale buffer bytes past the
received length.

After the trim, can a short frame with a GSO vnet header end up with
transport_header and GSO metadata beyond skb->tail?

The reads stay inside the allocated linear head. Triggering this needs
the host peer to send a GSO vnet header for a frame shorter than its
headers. The verify-before-trim ordering predates this patch, and the
old fallback also set gso_size/gso_type from the untrimmed length.

>  	return 0;
>  }

[ ... ]

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index c381b916c1b5..d6466f96cdd0 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -111,48 +111,38 @@ static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb,
>  		p_off = nh_min_len + thlen;
>  		if (!pskb_may_pull(skb, p_off))
>  			return -EINVAL;
> -	} else {
> +	} else if (gso_type) {
>  		/* gso packets without NEEDS_CSUM do not set transport_offset.
>  		 * probe and drop if does not match one of the above types.
>  		 */

[ ... ]

> +		struct flow_keys_basic keys;
> +
> +		if (!skb->protocol) {
> +			skb->protocol = dev_parse_header_protocol(skb);
> +			if (!skb->protocol)
> +				virtio_net_hdr_set_proto(skb, hdr);
> +		}
>  retry:

[ ... ]

> +		if (!skb_flow_dissect_flow_keys_basic(NULL, skb, &keys,
> +						      NULL, 0, 0, 0,
> +						      0)) {
> +			/* UFO does not specify ipv4 or 6: try both */
> +			if (gso_type & SKB_GSO_UDP &&
> +			    skb->protocol == htons(ETH_P_IP)) {
> +				skb->protocol = htons(ETH_P_IPV6);
> +				goto retry;
>  			}
> +			return -EINVAL;
> +		}

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. The
commit message mentions VLAN-tagged frames, though. For af_packet VLAN
frames, does this dissector call get a protocol and a network offset
that describe different headers?

packet_snd() calls packet_parse_headers() before
virtio_net_hdr_to_skb(). tpacket_snd() does the same through
tpacket_fill_skb():

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

For VLAN-tagged frames this moves network_header past the tags, but
skb->protocol stays ETH_P_8021Q or ETH_P_8021AD.

The call here passes data == NULL, so the dissector uses
proto = skb->protocol and nhoff = skb_network_offset(skb). It then
reads the inner L3 header as a vlan_hdr:

net/core/flow_dissector.c:__skb_flow_dissect() {
    ...
		proto = vlan->h_vlan_encapsulated_proto;
    ...
}

That value would come from the IPv4 tot_len field or the IPv6 payload
length bytes. Would VLAN GSO packets without VIRTIO_NET_HDR_F_NEEDS_CSUM
sent through AF_PACKET with PACKET_VNET_HDR then fail with -EINVAL? And
could they be misparsed if those bytes happen to form a valid ethertype?

tap_get_user() keeps network_header at ETH_HLEN while it calls
tun_vnet_hdr_to_skb(), and only moves it for VLAN afterwards. That is
the layout this helper expects.

The af_packet network header adjustment comes from commit dfed913e8b55a
("net/af_packet: add VLAN support for AF_PACKET SOCK_RAW GSO"). These
skbs already hit the same dissector call before this patch.

>  
> -			p_off = keys.control.thoff + thlen;
> -			if (!pskb_may_pull(skb, p_off) ||
> -			    keys.basic.ip_proto != ip_proto)
> -				return -EINVAL;
> +		p_off = keys.control.thoff + thlen;
> +		if (!pskb_may_pull(skb, p_off) ||
> +		    keys.basic.ip_proto != ip_proto ||
> +		    !virtio_net_hdr_match_proto(keys.basic.n_proto,
> +						hdr_gso_type))
> +			return -EINVAL;
>  
> -			skb_set_transport_header(skb, keys.control.thoff);
> -		} else if (gso_type) {
> -			p_off = nh_min_len + thlen;
> -			if (!pskb_may_pull(skb, p_off))
> -				return -EINVAL;
> -		}
> +		skb_set_transport_header(skb, keys.control.thoff);

[Severity: Medium]
Can keys.control.thoff be truncated here for large skbs?
__skb_flow_dissect() ends with:

net/core/flow_dissector.c:__skb_flow_dissect() {
    ...
out:
	key_control->thoff = min_t(u16, nhoff, skb ? skb->len : hlen);
    ...
}

min_t(u16, ...) casts skb->len to 16 bits. When skb->len is in
[65536, 65536 + nhoff), the result wraps to a small value.

With this patch, tun_get_user() calls tun_vnet_hdr_tnl_to_skb() before
eth_type_trans():

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

So skb->len still includes the 14-byte Ethernet header during
dissection. Take an IFF_TAP frame with skb->len = 65540 and a plain
IPv4/TCP header (real nhoff of 34). That gives
thoff = (u16)65540 = 4.

ip_proto and n_proto are still correct, so every check above passes.
pskb_may_pull() pulls only 24 bytes, and skb_set_transport_header(skb, 4)
points into the destination MAC. The later skb_probe_transport_header()
in tun_get_user() does nothing, because the transport header is already
set.

Before this patch, tun got its transport header from
skb_probe_transport_header() after eth_type_trans() had pulled 14
bytes. Frames of 65536..65549 bytes were not affected.

This does not look like an out-of-bounds read, since
transport_header + 12 falls inside the pulled thoff + thlen bytes. But
would tcp_hdrlen() in skb_gso_transport_seglen() and
qdisc_pkt_len_init() then read the wrong bytes? That would give wrong
qdisc pkt_len and gso_segs accounting, and a wrong tbf MAC length check.

It also seems to contradict this part of the commit message:

    so all GSO packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are
    flow-dissected, have their transport header pulled into linear
    data, and have skb->transport_header set.

In this case the real TCP header at offset 34..54 can stay in page
frags.

This is reachable through IFF_TAP with IFF_VNET_HDR, from a vhost-net
guest or an unprivileged tun owner in a user namespace. tun_alloc_skb()
allows skb->len well above 64K. tun_xdp_one() does not look affected,
since datasize is limited to one page.

>  	}
>  
>  	if (hdr_gso_type != VIRTIO_NET_HDR_GSO_NONE) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928144254.3361044-1-edumazet%40kernel.org