Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] net: virtio: L2 check bypass in virtio_net_l3_start() via TUNSETLINK --
commit 546d87e0158fb3d32de1c338228f7d9a8f02bb4e Author: Eric Dumazet <[email protected]> net: virtio: validate outer protocol of UDP tunnel GSO packets This commit validates the outer L2/L3/L4 headers of UDP tunnel GSO packets in virtio_net_hdr_tnl_to_skb() so that frames carrying mismatched outer protocols or offsets are rejected before GSO segmentation. 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..b9c8babb1fd73 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h [ ... ] > +/* Where does the L3 header start, and what protocol is it? > + * Handles Ethernet and VLAN headers; other devices use the > + * network header offset the caller set up. > + */ > +static inline bool > +virtio_net_l3_start(struct sk_buff *skb, int *nhoff, __be16 *proto) > +{ > + /* skb->dev should always be set here already, > + * but let's not crash when it is not. > + */ > + if (WARN_ON_ONCE(!skb->dev)) > + return false; > + > + if (skb->dev->type == ARPHRD_ETHER || > + skb->dev->type == ARPHRD_LOOPBACK) { > + __be16 h_proto; > + > + if (!pskb_may_pull(skb, ETH_HLEN)) > + return false; > + h_proto = eth_hdr(skb)->h_proto; > + /* skb->protocol is a sender claim, and parts of the > + * stack parse the packet per it: when it disagrees with > + * the frame we do not know which view the packet will > + * be parsed as, so do not validate. > + */ > + if (skb->protocol && skb->protocol != h_proto) > + return false; > + *nhoff = ETH_HLEN; > + *proto = h_proto; > + if (eth_type_vlan(*proto)) { > + *proto = __vlan_get_protocol(skb, *proto, nhoff); > + if (!*proto) > + return false; > + } > + } else { > + const struct iphdr *iph; > + > + *nhoff = skb_network_offset(skb); > + /* An unset network header shows up as an offset > + * outside of the packet. > + */ > + if (*nhoff < 0 || (unsigned int)*nhoff >= skb->len) > + return false; > + /* No L2 header to cross-check with: take the protocol > + * from the IP version at the network header. > + */ > + if (!pskb_may_pull(skb, *nhoff + 1)) > + return false; > + iph = (const struct iphdr *)(skb->data + *nhoff); > + if (iph->version == 4) > + *proto = htons(ETH_P_IP); > + else if (iph->version == 6) > + *proto = htons(ETH_P_IPV6); > + else > + return false; > + if (skb->protocol && skb->protocol != *proto) > + return false; > + } > + return true; > +} [Severity: High] Can a tap writer bypass the Ethernet header check in virtio_net_l3_start() by changing tun->dev->type via the TUNSETLINK ioctl? In drivers/net/tun.c, __tun_chr_ioctl() allows changing tun->dev->type while the device is down without changing tun->flags: drivers/net/tun.c:__tun_chr_ioctl() { case TUNSETLINK: ... tun->dev->type = (int) arg; tun->dev->addr_len = tun_get_addr_len(tun->dev->type); } Meanwhile, tun_get_user() (and tun_xdp_one()) decides whether the frame starts with an Ethernet header based on (tun->flags & TUN_TYPE_MASK) == IFF_TAP, setting the network header to ETH_HLEN before calling tun_vnet_hdr_tnl_to_skb(), and only populating skb->protocol via eth_type_trans() afterwards: drivers/net/tun.c:tun_get_user() { switch (tun->flags & TUN_TYPE_MASK) { ... case IFF_TAP: ... skb_set_network_header(skb, ETH_HLEN); break; } if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) { ... } if ((tun->flags & TUN_TYPE_MASK) == IFF_TAP) skb->protocol = eth_type_trans(skb, tun->dev); } If an IFF_TAP device has tun->dev->type set to another type such as ARPHRD_NONE via TUNSETLINK, virtio_net_l3_start() takes the else branch, reads *nhoff = skb_network_offset(skb) (which is ETH_HLEN), derives *proto from the IP version nibble at offset 14, and skips the skb->protocol check because skb->protocol is still 0. The Ethernet header at offset 0 is never inspected by virtio_net_l3_start(), so eth_type_trans() will subsequently set skb->protocol from the unvalidated eth_hdr(skb)->h_proto (for example ETH_P_NSH) on a packet with SKB_GSO_UDP_TUNNEL set, triggering the same skb_under_panic() during GSO segmentation. Conversely, on an IFF_TUN device whose tun->dev->type is changed to ARPHRD_ETHER via TUNSETLINK, virtio_net_l3_start() validates a 14-byte Ethernet header at offset 0 and an outer IP header at offset 14, but tun_get_user() does not call eth_type_trans() and resets skb->network_header to offset 0, leaving the outer network header pointing at the fake Ethernet header. Should tun_get_user() and tun_xdp_one() set skb->protocol before calling tun_vnet_hdr_tnl_to_skb(), or should virtio_net_l3_start() avoid relying on skb->dev->type matching the caller's L2 framing? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
