Le jeu. 5 nov. 2026 à 11:08, <[email protected]> a écrit :
>
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 · Low: 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).
>
> - [High] The new outer-header validator can still be bypassed, so the
> NSH-over-UDP-tunnel GSO crash this commit claims to fix is still…
> - [Low] In virtio_net_l4_start() (include/linux/virtio_net.h), the
> comment before the ipv6_find_hdr() call says "Find the transport…
>
> --
>
> net: virtio: validate outer protocol of UDP tunnel GSO packets
>
> This commit makes virtio_net_hdr_tnl_to_skb() validate the outer headers
> of UDP tunnel GSO packets. The frame must carry an outer IPv4 or IPv6
> header that matches the declared tunnel type, with UDP at
> outer_th_offset. The L3 start is taken from the device type: either an
> Ethernet/VLAN header, or the network header the caller set.
>
> 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
>
> [ ... ]
>
> > @@ -48,6 +51,146 @@ static inline int virtio_net_hdr_set_proto(struct
> > sk_buff *skb,
>
> [ ... ]
>
> > +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 owner push this function into the wrong branch, so the NSH
> crash from the commit message is still reachable?
>
> The branch is chosen from skb->dev->type. In tun, the real framing and the
> later eth_type_trans() call both follow tun->flags. The tap owner can set
> dev->type to any value with TUNSETLINK while the device is down:
>
> drivers/net/tun.c:__tun_chr_ioctl() {
> ...
> tun->dev->type = (int) arg;
> ...
> }
>
> Take an IFF_TAP device with dev->type set to, for example, ARPHRD_NONE.
> tun_get_user() does:
>
> skb_set_network_header(skb, ETH_HLEN);
> ...
> 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);
>
> In that case virtio_net_l3_start() takes the else branch with
> nhoff = ETH_HLEN and only reads the IP version nibble at byte 14.
>
> The skb->protocol cross-check is skipped because skb->protocol is still 0
> at this point. h_proto in bytes 12-13 is never read.
>
> With a valid IPv4 header at offset 14 and UDP at offset 34,
> virtio_net_tnl_offsets_ok() returns true. eth_type_trans() then sets
> skb->protocol from the unchecked h_proto, for example ETH_P_NSH, while
> SKB_GSO_UDP_TUNNEL is set. tun_xdp_one() has the same order: validation
> first, then an unconditional eth_type_trans().
>
> If that skb is redirected with tc mirred to a gretap, does it reach the
> same crash shown in the commit message?
>
> validate_xmit_skb()
> skb_mac_gso_segment()
> nsh_gso_segment()
> ...
> gre_tap_xmit()
> __gre_xmit()
> skb_push()
> skb_under_panic()
>
> Compared with the original reproducer, the only extra step seems to be
> one TUNSETLINK ioctl before bringing the tap up.
>
> There is a second variant: an IFF_TUN device with a packet-info header
> and dev->type set to ARPHRD_ETHER.
>
> With pi.proto = 0x8100 and bytes 12-13 = 0x8100, the cross-check passes.
> The validator then walks a VLAN tag at 14 to IPv4 at 18 and UDP at 38.
>
> The stack handles the packet as raw L3, though. skb_vlan_untag() reads
> the VLAN header at data[0], so attacker-chosen bytes 2-3 (for example
> NSH) become skb->protocol.
>
> The skb->protocol cross-check cannot catch either case. In the first it
> is 0, and in the second the sender controls both values.
>
> vhost-net guests cannot change dev->type, so the guest path looks closed.
> The tap writer path named in the commit message still seems open.
Right, both variants are real. They need CAP_NET_ADMIN over the
device: TUNSETLINK only works while the device is down, and it has to
be brought up again. A vhost-net guest cannot use them, but a tap
writer in a user namespace can.
v3 follows your suggestion: virtio_net_hdr_tnl_to_skb() gets a has_eth
argument from its callers, and virtio_net_l3_start() no longer looks
at the device type. tun passes whether the device is IFF_TAP (and true
from tun_xdp_one(), which always calls eth_type_trans()), virtio_net
always passes true. v3 also adds tests for both variants (tap set to
ARPHRD_NONE, IFF_TUN set to ARPHRD_ETHER).
>
> A smaller point: dev->type is read here without READ_ONCE() or a lock,
> while TUNSETLINK writes it under rtnl. The bypass above does not need
> that race, though, since a stable mismatched type is enough.
>
Since tun_get_user() only drops packets of a down device after the
checks, a check of dev->type in tun alone would have been racy too.
With has_eth, dev->type is no longer read on this path.
> Could the caller tell the validator whether the frame starts with an
> Ethernet header instead? tun knows this from tun->flags, and virtio_net
> always has one. Another option would be to always cross-check h_proto
> whenever eth_type_trans() will be applied afterwards.
>
> [ ... ]
>
> > + if (proto == htons(ETH_P_IPV6)) {
> > + const struct ipv6hdr *iph;
> > + int flags = IP6_FH_F_AUTH;
> > +
> > + if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
> > + return false;
> > + iph = (const void *)(skb->data + nhoff);
> > + if (iph->version != 6)
> > + return false;
> > + *thoff = nhoff + sizeof(*iph);
> > + *ip_proto = iph->nexthdr;
> > + if (*ip_proto != IPPROTO_TCP && *ip_proto != IPPROTO_UDP) {
> > + *thoff = nhoff;
> > + /* Find the transport header behind any
> > + * extension headers, AH included.
> > + */
> > + *ip_proto = ipv6_find_hdr(skb, thoff, -1, NULL,
> > + &flags);
>
> [Severity: Low]
>
> This isn't a bug, but the comment says the walk goes past AH. With
> flags = IP6_FH_F_AUTH and target = -1, ipv6_find_hdr() actually stops at
> the AH header:
Indeed, the comment comes from Michael's helper, which walked past
AH. I will fix it in v3.
pw-bot: cr
>
> net/ipv6/exthdrs_core.c:ipv6_find_hdr() {
> ...
> } else if (nexthdr == NEXTHDR_AUTH) {
> if (flags && (*flags & IP6_FH_F_AUTH) && (target < 0))
> break;
> ...
> }
>
> It returns NEXTHDR_AUTH, so an outer IPv6+AH+UDP frame is rejected. That
> matches the commit message ("ipv6_find_hdr() stops at an AH header").
>
> Could the comment say that the walk stops at AH? As written, a reader
> might take it as a reason to drop IP6_FH_F_AUTH later.
>
> > + if (*ip_proto < 0 || flags & IP6_FH_F_FRAG)
> > + return false;
> > + }
> > + return true;
> > + }
>
> [ ... ]
>
> --
> Sashiko AI review ·
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104050131.629967-1-edumazet%40kernel.org
