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

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Oct 06, 2026 at 08:13:43PM -0400, Willem de Bruijn wrote:
> On Tue, Oct 6, 2026 at 7:50 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Oct 06, 2026 at 07:26:46PM -0400, Willem de Bruijn wrote:
> > > On Tue, Oct 6, 2026 at 6:38 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Thu, Oct 01, 2026 at 07:11:37PM +0000, Eric Dumazet wrote:
> > > > > 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/
> > > > >
> > > > > 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/
> > > >
> > > >
> > > > 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?

> Perhaps the general idea can be extended: harden the kernel to the
> small set of well known types, strict validation and even processing
> for the long tail of other traffic.
> 
> I'd even (optionally, maybe behind a sysctl/static-branch) run
> flow_dissection to ensure the packets are what they claim. When
> only unencapsulated TCP/IP is expected, this is cheap enough. And
> it avoids the manual sort-of parser code that we have now.