Re: [PATCH v3 net 0/3] net: always dissect GSO packets in __virtio_net_hdr_to_skb()

On Tue, Oct 6, 2026 at 8:18 PM Michael S. Tsirkin [off-list ref] wrote:

On Tue, Oct 06, 2026 at 08:13:43PM -0400, Willem de Bruijn wrote:

quoted

On Tue, Oct 6, 2026 at 7:50 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Tue, Oct 06, 2026 at 07:26:46PM -0400, Willem de Bruijn wrote:

quoted

On Tue, Oct 6, 2026 at 6:38 PM Michael S. Tsirkin [off-list ref] wrote:

quoted

On Thu, Oct 01, 2026 at 07:11:37PM +0000, Eric Dumazet wrote:

quoted

This series fixes a bypass of untrusted GSO flow dissection in
__virtio_net_hdr_to_skb() when VIRTIO_NET_HDR_F_NEEDS_CSUM is not set,
and adds a kselftest covering VLAN-tagged GSO packets without NEEDS_CSUM:

- Patch 1 fixes __skb_flow_dissect(), which computes key_control->thoff
  with min_t(u16, ...). This truncates skb->len and returns a bogus small
  transport offset when skb->len modulo 65536 is smaller than the
  transport offset. Offsets that do not fit in the u16 thoff now fail the
  dissection instead of being silently truncated.

- Patch 2 initializes skb->dev and skb->network_header before calling
  virtio_net_hdr_*_to_skb() in tun_get_user(), tun_xdp_one(),
  virtnet_receive_done(), and raw_verify_header(), removes the
  '&& skb->network_header' condition and the unvalidated
  'else if (gso_type)' fallback in __virtio_net_hdr_to_skb(), and moves
  virtio_net_hdr_match_proto() after skb_flow_dissect_flow_keys_basic()
  so it validates the dissected L3 protocol (keys.basic.n_proto) rather
  than the outer L2 protocol.

- Patch 3 adds kselftests in tools/testing/selftests/net/tun.c verifying
  that VLAN-tagged (802.1Q) TCPv4 GSO packets without NEEDS_CSUM (both
  flags = 0 and flags = VIRTIO_NET_HDR_F_DATA_VALID) are accepted on a
  TAP device, that mismatched GSO types and truncated TCP headers without
  NEEDS_CSUM are rejected with -EINVAL, and that a 65540-byte frame is
  accepted.

v3:
 - New patch 1: avoid u16 truncation of skb->len when computing thoff in
   __skb_flow_dissect(). Patch 2 makes tun_get_user() dissect IFF_TAP
   frames before eth_type_trans(), with skb->len up to 65549 for a GSO
   frame carrying a maximal IPv4 packet (Sashiko).
 - Patch 3: truncate the TCP header after 10 bytes so that the test
   requires the transport offset found by flow dissection, and add a
   65540-byte frame test (Sashiko).
 - Link to v2: https://lore.kernel.org/netdev/20260928144254.3361044-1-edumazet@kernel.org/ (local)

v2:
 - Patch 2: drop the pre-dissection virtio_net_hdr_match_proto() check
   inside 'if (!skb->protocol)' so VLAN-tagged GSO frames without
   NEEDS_CSUM are not rejected before flow dissection (Michael S. Tsirkin).
 - Patch 2: clarify the changelog regarding why skb->network_header was 0
   in those callers and why skb_reset_mac_header() is dropped in
   tun_get_user() for IFF_TUN (Michael S. Tsirkin).
 - Patch 3: add selftest in tools/testing/selftests/net/tun.c based on
   Michael's reproducer.
 - Link to v1: https://lore.kernel.org/netdev/20260927195536.2489079-1-edumazet@google.com/ (local)

Not without trepidation about the amount of stuff we are shoving
into virtio_net_hdr_to_skb which, believe me or not, used to be 50 LOC
of trivial code in 2019:

Unfortunately that let through many bad packets and unintentional
(ab)uses of the API.

The current state is the result of numerous fixes we had to apply
since then to protect the kernel. Generally there are two approaches:

1. make every reachable path in the kernel robust against unexpected
input. Frequently that means checks in the hot path that penalizes all
normal traffic, only to catch a bad actor or fuzzer. And it's not
straightforward to prove that all reachable paths are protected.
2. strict input validation.

With strict input validation from the start the checks could have been
simpler (hindsight is 20/20). Unfortunately, now we are stuck with
weird input (GSO without NEEDS_CSUM, skb protocol 0, encapsulation
headers, ..) that we now have to work around and try to not break,
that may or may not have real users.

This patch actually makes the function simpler. By reducing the
differences between the various callers of the function. This is great.

It sucks how complex this function has become, hopefully we can
find more such ways of making it simpler. The strict validation itself
is a good thing imho.

Yes indeed. I have a vague idea how to do it: check some
performance-critical types of packets and for the rest just calculate
the checksum then and there.

That sounds promising.

Checksumming is only one of the risks. Segmentation is another.

Same approach for segmentation would be great but how do we know how to
segment at input?

gso_type?

The main issue I see is that we do not have a way to then process the
chain of segs. So maybe we cannot realistically do it at this stage.

Come to think of it, I previously considered this and ended up with
commit 121d57af308d ("gso: validate gso_type in GSO handlers")
instead.