On Fri, Oct 23, 2026 at 03:37:48PM -0400, Willem de Bruijn wrote: > Michael S. Tsirkin wrote: > > As reported first by Paulos Yibelo <[email protected]> (after which > > others, credited below, found more ways the bug can affect users) a > > virtio net header can place CHECKSUM_PARTIAL at any in-packet offset. > > If that state is retained, later checksum completion can modify the > > packet after protocol processing has made decisions from it. > > > > Let's fix that: let's validate csum_start and csum_offset and only keep > > CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or > > UDP header, while handling VLAN headers, IPv4 options and IPv6 extension > > headers. Complete other non-GSO checksums immediately, before any > > protocol processing runs. > > > > We can't easily "complete the checksum" for GSO we don't recognize - GSO > > assumes knowledge of the protocol. We could segment ignoring the > > checksum then do whatever csum_start and csum_offset say, but that > > seems like an overkill for something that shouldn't happen. > > > > There's some risk here: > > - non-gso packets which can't be parsed get checksum calculated in > > software, adding performance overhead > > - gso packets which can't be parsed get dropped > > > > Note that for zero copy, userspace can change the data after the > > checksum has been calculated. This is fundamental to zero copy by > > design, and the only outcome is a bad checksum and a packet drop. > > > > To mitigate the risks, the following patch will add a sysctl knob to > > bypass the resolution. This is reasonable if the setup is such that > > there is no protocol processing at all. > > > > UDP tunnel headers get the same handling on the inner header. > > > > Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected. > > > > Reported-by: Paulos Yibelo <[email protected]> > > Reported-by: Chris Arges <[email protected]> > > Tested-by: Chris Arges <[email protected]> > > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr") > > Suggested-by: Willem de Bruijn <[email protected]> > > Signed-off-by: Michael S. Tsirkin <[email protected]> > > A few comments inline. All but one cosmetic. Feel free to ignore > those. Please do review the comment from an LLM about the tunnel. > Aside from that LGTM. >
... > > @@ -368,7 +637,20 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, > > skb->inner_mac_header = inner_nh + skb_headroom(skb); > > skb->transport_header = outer_th + skb_headroom(skb); > > skb->encapsulation = 1; > > - return 0; > > + > > + if (!skb_is_gso(skb)) > > + skb_shinfo(skb)->gso_type &= ~(SKB_GSO_UDP_TUNNEL_CSUM | > > + SKB_GSO_UDP_TUNNEL); > > + > > + /* > > + * We know VIRTIO_NET_HDR_F_NEEDS_CSUM was set so > > __virtio_net_hdr_to_skb > > + * set CHECKSUM_PARTIAL. > > + */ > > + if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh, > > + inner_proto)) > > + return 0; > > + > > + return __virtio_net_resolve_csum_partial_mismatch(skb); > > Allowing `!skb_is_gso(skb)` packets through `virtio_net_hdr_tnl_to_skb()` > has a few issues: > > 1. When `!skb_is_gso(skb)` and `virtio_net_csum_partial_ok_at_offset()` > returns false, `__virtio_net_resolve_csum_partial_mismatch()` completes > the checksum in software and lets the packet pass, but > `skb->encapsulation`, > `skb->inner_protocol`, `skb->inner_transport_header`, > `skb->inner_network_header`, and `skb->inner_mac_header` have already > been populated with the unvalidated `inner_th` (`csum_start`) and > `inner_nh` offsets (unlike `virtio_net_hdr_to_skb()`, which restores > `skb->transport_header`). Oh right. Hmm. I can just run the check first. > 2. If `!skb_is_gso(skb)` and `VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM` is set, > clearing `SKB_GSO_UDP_TUNNEL_CSUM` drops the outer UDP checksum > request, > since a non-GSO skb can only carry a single `CHECKSUM_PARTIAL` offset > (at `inner_th`). Right. But it works! Because the checksum is 0 then. I'll ponder what is best. > 3. On RX, a non-GSO UDP packet with `CHECKSUM_PARTIAL` at `inner_th` > causes > `skb_csum_unnecessary()` (`skb_checksum_start_offset(skb) >= 0`) to > treat > the outer UDP checksum as already validated. > > Unlike plain GSO (commit 7c6d2ecbda83), UDP tunnel offload is modern-only > and > explicitly requires GSO (`/* The UDP tunnel must carry a GSO packet, but > no UFO. */`). > Why not reject `!skb_is_gso(skb)` right after > `__virtio_net_hdr_to_skb()`, and > return `-EINVAL` directly if `!virtio_net_csum_partial_ok_at_offset()`? This will break apps, I think. See 7c6d2ecbda83 -- MST
