Le lun. 9 nov. 2026 à 10:55, <[email protected]> a écrit : > > Thank you for your contribution! Sashiko AI review found 1 potential > issue(s) to consider. > > Critical: 0 · High: 0 · Medium: 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] The new strict outer-offset check rejects UDP-tunnel GSO > frames that the kernel's own tun TX path produces whenever the skb… > > Pre-existing issues: > - [Low] In virtio_net_hdr_tnl_to_skb(), outer_l3min = ETH_HLEN + > virtio_l3min(outer_isv6) (include/linux/virtio_net.h:491). > > -- > > net: virtio: validate outer protocol of UDP tunnel GSO packets > > This commit makes virtio_net_hdr_tnl_to_skb() check that the frame's outer > L3 protocol matches the declared tunnel type. It also checks that the outer > UDP header sits exactly at outer_th_offset. Callers pass a new has_eth > argument that says whether the frame starts with an Ethernet header. This > closes a path where an arbitrary outer ethertype combined with > SKB_GSO_UDP_TUNNEL could crash the host in skb_under_panic(). > > Link: > https://lore.kernel.org/netdev/93d0e37c99711bebdf60c120af03010455c2f2e2.1793276116.git....@redhat.com/ > > > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > > index d6466f96cdd00..b7b6b542f4763 100644 > > --- a/include/linux/virtio_net.h > > +++ b/include/linux/virtio_net.h > > [ ... ] > > > @@ -282,9 +424,33 @@ static inline unsigned int virtio_l3min(bool is_ipv6) > > return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr); > > } > > > > +/* Do the tunnel offsets match the outer headers in the frame? > > + * The L3 header the tunnel claims must be the one the frame > > + * carries, with UDP following it at @outer_th. > > + */ > > +static inline bool > > +virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool has_eth, bool > > outer_isv6, > > + unsigned int outer_th) > > +{ > > + int nhoff; > > + __be16 proto; > > + unsigned int thoff; > > + int ip_proto; > > + > > + if (!virtio_net_l3_start(skb, has_eth, &nhoff, &proto)) > > + return false; > > + if (outer_isv6 != (proto == htons(ETH_P_IPV6))) > > + return false; > > + if (!virtio_net_l4_start(skb, nhoff, proto, &thoff, &ip_proto)) > > + return false; > > + > > + return ip_proto == IPPROTO_UDP && outer_th == thoff; > > [Severity: Medium] > > Can this exact match reject UDP tunnel GSO frames that tun itself produces > when the skb carries a VLAN hwaccel tag? > > On the producer side, tun_put_user() writes the 802.1Q tag into the frame > it copies to userspace or vhost-net. It calls tun_vnet_hdr_tnl_from_skb() > with vlan_hlen = VLAN_HLEN. Only csum_start accounts for the tag: > > include/linux/virtio_net.h:virtio_net_hdr_from_skb() { > ... > hdr->csum_start = __cpu_to_virtio16(little_endian, > skb_checksum_start_offset(skb) + vlan_hlen); > ... > } > > The tunnel offsets do not: > > include/linux/virtio_net.h:virtio_net_hdr_tnl_from_skb() { > ... > inner_nh = skb->inner_network_header - skb_headroom(skb); > outer_th = skb->transport_header - skb_headroom(skb); > vhdr->inner_nh_offset = cpu_to_le16(inner_nh); > vhdr->outer_th_offset = cpu_to_le16(outer_th); > ... > } > > So outer_th_offset and inner_nh_offset both end up 4 bytes short of the > real UDP and inner headers in the emitted frame. > > On the consumer side, with has_eth set, virtio_net_l3_start() walks the > in-band VLAN tag with __vlan_get_protocol(). That gives nhoff = 18, so > thoff points at the real UDP header (38 for IPv4). The emitted outer_th is > 34, this comparison fails, and virtio_net_hdr_tnl_to_skb() returns -EINVAL. > > Before this patch the same frame was accepted. Only transport_header was > off by 4, and ip_rcv_core() and ip6_rcv_core() reset it for local delivery. > > With this change, would a guest that gets these frames from a host tap > drop them in virtnet_receive_done() as "bad gso" and bump > rx_frame_errors? A tun/tap writer that re-injects frames it read from > another tun would see the same rejection. > > This looks reachable with VLAN-tagged UDP tunnel GSO traffic. Two examples > are a VLAN filtering bridge with a tagged egress port, and a vlan device on > top of a tap. tun advertises NETIF_F_HW_VLAN_CTAG_TX/STAG_TX in > hw_features and hw_enc_features, so the tag stays in hwaccel until > tun_put_user(). > > The later patch in this series, "tun: clear stale skb->protocol of NAPI > frags skbs", does not seem to fix the producer side. Should > virtio_net_hdr_tnl_from_skb() add vlan_hlen to outer_th_offset and > inner_nh_offset, with that fix landing with or before this check? >
Right. tun keeps the tag in hwaccel for UDP tunnel GSO packets, and only csum_start accounts for it. The wrong offsets were already a bug: the receiver set inner_network_header 4 bytes off, and a forwarded packet segmented again would use the wrong inner headers. v5 will add a patch before this one, so that virtio_net_hdr_tnl_from_skb() adds vlan_hlen to both offsets. > > +} > > + > > static inline int > > virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, > > const struct virtio_net_hdr_v1_hash_tunnel *vhdr, > > + bool has_eth, > > bool tnl_hdr_negotiated, > > bool tnl_csum_negotiated, > > bool little_endian) > > @@ -338,6 +504,9 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, > > if (ret) > > return ret; > > > > + if (!virtio_net_tnl_offsets_ok(skb, has_eth, outer_isv6, outer_th)) > > + return -EINVAL; > > + > > [Severity: Low] > > This is a pre-existing issue, not one introduced by this patch, but does > the has_eth = false path ever see ordinary IFF_TUN frames? > > Earlier in virtio_net_hdr_tnl_to_skb(), the minimum outer offset still > assumes an Ethernet header, whatever has_eth says: > > outer_l3min = ETH_HLEN + virtio_l3min(outer_isv6); > ... > if (outer_th < outer_l3min || > > For IFF_TUN, tun_get_user() passes has_eth = false and the network header > is at offset 0. virtio_net_tnl_offsets_ok() therefore needs outer_th == 20 > for plain IPv4 or 40 for plain IPv6. Both values are below the 34/54 floor, > so these frames are rejected before the new check runs. > > That leaves the !has_eth branch of virtio_net_l3_start() reachable only for > outer IPv4 with at least 16 bytes of options, or outer IPv6 with at least > 16 bytes of extension headers. The L3-relative offsets produced by tun TX > and by the selftest helper build_virtio_net_hdr_v1_hash_tunnel() > (eth_hlen = 0 for !is_tap) are still rejected. > > The ETH_HLEN floor dates from a2fb4bc4e2a6. Now that callers pass has_eth, > should outer_l3min depend on it? > This predates this series: UDP tunnel GSO over IFF_TUN has never worked with plain IPv4/IPv6 outer headers. Removing the floor would enable a path that has never been used, which does not belong in this fix. I will send this as a follow-up. pw-bot: cr
