Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] net: virtio: unreliable L3 check in virtio_net_tnl_outer_proto_ok() --
commit 02f420dee7a050b3832ecf552c44f9c2eeaaddc1 Author: Eric Dumazet <[email protected]> net: virtio: validate outer protocol of UDP tunnel GSO packets This commit validates that the outer L3 protocol of a UDP tunnel GSO packet matches the tunnel type declared in the virtio header in virtio_net_hdr_tnl_to_skb(). > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > index d6466f96cdd00..d3f97d71d81cc 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h > @@ -282,6 +282,31 @@ static inline unsigned int virtio_l3min(bool is_ipv6) > return is_ipv6 ? sizeof(struct ipv6hdr) : sizeof(struct iphdr); > } > > +/* The outer network header of a UDP tunnel GSO packet must match the > + * tunnel type declared in the virtio header, otherwise the GSO stack > + * would parse the packet as something else (e.g. NSH). > + */ > +static inline bool virtio_net_tnl_outer_proto_ok(const struct sk_buff *skb, > + bool outer_isv6, > + unsigned int outer_th) > +{ > + __be16 expected = outer_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP); > + __be16 proto = skb->protocol; > + int nh_off = ETH_HLEN; > + > + /* L3 device (IFF_TUN): the caller already set the protocol. */ > + if (proto) > + return proto == expected; [Severity: High] Can testing if (proto) here be bypassed in tun_get_user() in two ways? First, on an IFF_TAP device with IFF_NAPI_FRAGS enabled, tun_get_user() allocates the skb via tun_napi_alloc_frags() -> napi_get_frags(). When a prior packet was merged by GRO (GRO_MERGED_FREE in napi_frags_finish()), napi_reuse_skb() caches napi->skb for reuse without clearing skb->protocol (which napi_frags_skb() set to the previous frame's eth->h_proto, such as htons(ETH_P_IP)). In drivers/net/tun.c:tun_get_user(), tun_vnet_hdr_tnl_to_skb() runs before eth_type_trans() updates skb->protocol for the new frame: if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) { atomic_long_inc(&tun->rx_frame_errors); err = -EINVAL; goto free_skb; } if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP) skb->protocol = eth_type_trans(skb, tun->dev); With a reused napi->skb still carrying skb->protocol == htons(ETH_P_IP), virtio_net_tnl_outer_proto_ok() takes the if (proto) branch and returns true without inspecting eth_hdr(skb)->h_proto or checking nh_off + virtio_l3min(outer_isv6) <= outer_th, and eth_type_trans() then sets skb->protocol to the new frame's actual EtherType (such as ETH_P_NSH). Second, on an IFF_TUN device without IFF_NO_PI, tun_get_user() copies struct tun_pi from userspace and sets skb->protocol = pi.proto: skb_reset_network_header(skb); skb->protocol = pi.proto; break; If userspace passes pi.proto == 0, proto is 0, so virtio_net_tnl_outer_proto_ok() falls through to the Ethernet path and checks eth_hdr(skb)->h_proto inside the L3 payload, leaving skb->protocol == 0 with SKB_GSO_UDP_TUNNEL set if those bytes match expected. Since all callers set skb->network_header before calling virtio_net_hdr_tnl_to_skb() (to 0 via skb_reset_network_header() for IFF_TUN, and to ETH_HLEN via skb_set_network_header() for IFF_TAP and virtio_net), would checking !skb_network_offset(skb) instead of if (proto) avoid both issues? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
