Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] net: virtio_net_l3_start() allows skb->protocol and eth_hdr() mismatch - [High] net: virtio_net_hdr_tnl_to_skb() mishandles non-GSO tunnel packets - [High] net: virtio_net_l3_start() IPv6 inference overwritten by set_proto - [Medium] net: virtio_net_l4_start() skips IPv6 AH headers without IP6_FH_F_AUTH --
commit 1c3b02f56c070f59dd306e2b6555bc2eacaea7e4 Author: Michael S. Tsirkin <[email protected]> net: resolve uncommon partial checksums at ingress This commit validates virtio_net_hdr checksum offsets at ingress so that CHECKSUM_PARTIAL is only retained when csum_start and csum_offset match a parsed TCP or UDP header. Other non-GSO partial checksums are completed in software immediately, while unrecognized GSO packets are dropped. > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > index 013c6c7e2dea0..52852a33a2db7 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h > @@ -48,6 +52,205 @@ 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 is only guaranteed to be set since 44378d02c5aa > + * ("net: always dissect GSO packets in > + * __virtio_net_hdr_to_skb()"), do not crash when it is not. > + */ > + if (WARN_ON_ONCE(!skb->dev)) > + return false; > + > + /* A preset protocol is what the stack will use, except on > + * loopback which re-parses the frame: loopback_xmit() calls > + * eth_type_trans() on it. > + */ > + if (skb->dev->type == ARPHRD_ETHER || > + skb->dev->type == ARPHRD_LOOPBACK) { > + if (!pskb_may_pull(skb, ETH_HLEN)) > + return false; > + *nhoff = ETH_HLEN; > + *proto = skb->protocol; > + if (!*proto || skb->dev->type == ARPHRD_LOOPBACK) > + *proto = eth_hdr(skb)->h_proto; > + if (eth_type_vlan(*proto)) { > + *proto = __vlan_get_protocol(skb, *proto, nhoff); > + if (!*proto) > + return false; > + } [Severity: High] What happens when skb->protocol and eth_hdr(skb)->h_proto disagree on an AF_PACKET socket? In net/packet/af_packet.c, packet_snd() and tpacket_snd() set skb->protocol from the user-supplied sll_protocol or socket protocol before calling virtio_net_hdr_to_skb(): if (proto != htons(ETH_P_ALL)) skb->protocol = proto; skb->dev = dev; ... if (vnet_hdr_sz) { err = virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()); On an ARPHRD_ETHER device, if userspace presets skb->protocol to htons(ETH_P_8021Q) while eth_hdr(skb)->h_proto in the frame is htons(ETH_P_IP), virtio_net_l3_start() ignores eth_hdr(skb)->h_proto and calls __vlan_get_protocol(), treating the first 4 bytes of the IPv4 header as a VLAN tag and advancing *nhoff by 4 bytes into a fake header inside the packet. Conversely, if skb->protocol is htons(ETH_P_IP) while eth_hdr(skb)->h_proto is htons(ETH_P_8021Q), virtio_net_ip4_parse() parses the VLAN tag as the start of an IPv4 header. When the frame is transmitted across virtual ARPHRD_ETHER devices that forward via __dev_forward_skb2() in net/core/dev.c (such as veth_xmit() -> veth_forward_skb() -> __dev_forward_skb(), macvlan, ipvlan, or netkit), skb->protocol is overwritten from eth_hdr(skb)->h_proto: if (likely(!ret)) { skb->protocol = eth_type_trans(skb, dev); skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN); } On the other hand, on ARPHRD_LOOPBACK, virtio_net_l3_start() ignores a preset skb->protocol and validates against eth_hdr(skb)->h_proto, even though the transmit path prior to loopback_xmit() (such as TC egress and validate_xmit_skb() -> skb_mac_gso_segment() -> skb_network_protocol()) uses skb->protocol. Could this allow a packet with mismatched skb->protocol and eth_hdr(skb)->h_proto to bypass the CHECKSUM_PARTIAL offset validation in virtio_net_csum_partial_ok()? > + } else { > + *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; > + *proto = skb->protocol; > + if (!*proto) { > + const struct iphdr *iph; > + > + /* A packet socket bound to ETH_P_ALL leaves the > + * protocol unset: infer it 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); > + } > + } > + return true; > +} [Severity: High] Should virtio_net_l3_start() also update skb->protocol when it infers the protocol from the IP version, or should virtio_net_hdr_set_proto() inspect the IP version as well? When a packet socket bound to ETH_P_ALL sends an IPv6 packet with VIRTIO_NET_HDR_F_NEEDS_CSUM and VIRTIO_NET_HDR_GSO_UDP_L4 (or VIRTIO_NET_HDR_GSO_UDP) on a non-Ethernet device without header_ops->parse_protocol, __virtio_net_hdr_to_skb() takes the VIRTIO_NET_HDR_F_NEEDS_CSUM branch and leaves skb->protocol as 0. virtio_net_l3_start() then infers *proto = htons(ETH_P_IPV6) into its local variable, and virtio_net_csum_partial_ok_at_offset() validates the packet as IPv6. However, after virtio_net_hdr_to_skb() returns, packet_snd() and tpacket_snd() call virtio_net_hdr_set_proto() in include/linux/virtio_net.h: if (skb->protocol) return 0; switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) { case VIRTIO_NET_HDR_GSO_TCPV4: case VIRTIO_NET_HDR_GSO_UDP: case VIRTIO_NET_HDR_GSO_UDP_L4: skb->protocol = cpu_to_be16(ETH_P_IP); break; Wouldn't this overwrite skb->protocol with htons(ETH_P_IP) after the packet was validated as IPv6, causing skb_mac_gso_segment() to dispatch the IPv6 packet to inet_gso_segment()? > + > +/* Walk the L3 header at @nhoff: where does its transport header > + * start, and which protocol is it? Returns false if the header > + * is truncated, malformed or fragmented. > + */ > +static inline bool > +virtio_net_l4_start(struct sk_buff *skb, unsigned int nhoff, __be16 proto, > + unsigned int *thoff, int *ip_proto) > +{ > + if (proto == htons(ETH_P_IP)) { > + const struct iphdr *iph = virtio_net_ip4_parse(skb, nhoff, > thoff); > + > + if (!iph || ip_is_fragment(iph)) > + return false; > + *ip_proto = iph->protocol; > + return true; > + } > + > + if (proto == htons(ETH_P_IPV6)) { > + const struct ipv6hdr *iph; > + int flags = 0; > + > + 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); > + if (*ip_proto < 0 || flags & IP6_FH_F_FRAG) > + return false; > + } [Severity: Medium] Should flags be initialized to IP6_FH_F_AUTH before calling ipv6_find_hdr() in virtio_net_l4_start()? When ipv6_find_hdr() in net/ipv6/exthdrs_core.c is called with flags = 0, it skips NEXTHDR_AUTH headers by ipv6_authlen(hp) bytes. By contrast, packet dissectors and netfilter helpers such as nft_set_pktinfo_ipv6() in include/net/netfilter/nf_tables_ipv6.h stop at NEXTHDR_AUTH: unsigned int flags = IP6_FH_F_AUTH; int protohdr, thoff = 0; unsigned short frag_off; protohdr = ipv6_find_hdr(pkt->skb, &thoff, -1, &frag_off, &flags); Because ipv6_find_hdr() only checks the first 2 bytes of the AH header, a packet carrying NEXTHDR_AUTH can advance *thoff to any 4-byte-aligned offset past the IPv6 header and keep CHECKSUM_PARTIAL armed across rules that stop at NEXTHDR_AUTH. In addition, on local IPv6 receive, ah6_input() in net/ipv6/ah6.c resets skb->ip_summed = CHECKSUM_NONE without completing CHECKSUM_PARTIAL before verifying the AH ICV over the inner transport header, and IPv6 GSO offload in ipv6_gso_pull_exthdrs() does not support NEXTHDR_AUTH. Wouldn't stopping at NEXTHDR_AUTH via IP6_FH_F_AUTH allow non-GSO AH packets to have their checksum completed in software at ingress and reject AH GSO packets, matching the IPv4 behavior? [ ... ] > @@ -368,7 +628,22 @@ 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. > + */ > + inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP); > + if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh, > + inner_proto)) > + return 0; > + > + /* A tunnel header implies GSO, so drop. */ > + return __virtio_net_resolve_csum_partial_mismatch(skb); > } [Severity: High] How are non-GSO packets with a tunnel header handled in virtio_net_hdr_tnl_to_skb()? In __virtio_net_hdr_to_skb(), if skb->len - nh_off <= gso_size, shinfo->gso_size is not set, so skb_is_gso(skb) is false when control returns to virtio_net_hdr_tnl_to_skb(). If !skb_is_gso(skb) and virtio_net_csum_partial_ok_at_offset() returns true, virtio_net_hdr_tnl_to_skb() returns 0 while keeping CHECKSUM_PARTIAL at the caller-chosen vhdr->inner_nh_offset + inner_th inside the UDP payload (for example, pointing at a fake IPv4 header placed inside the UDP payload or overlapping a VXLAN inner Ethernet header), even though GSO segmentation will not run. If VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM was set, clearing SKB_GSO_UDP_TUNNEL_CSUM also leaves the outer UDP checksum uncomputed. On the other hand, if !skb_is_gso(skb) and virtio_net_csum_partial_ok_at_offset() returns false, the comment states that a tunnel header implies GSO so the packet is dropped, but __virtio_net_resolve_csum_partial_mismatch() only returns -EINVAL when skb_is_gso(skb) is true: static inline int __virtio_net_resolve_csum_partial_mismatch(struct sk_buff *skb) { if (skb_is_gso(skb)) return -EINVAL; return skb_checksum_help(skb) ?: 1; } When !skb_is_gso(skb), __virtio_net_resolve_csum_partial_mismatch() completes the checksum via skb_checksum_help() and returns 1, leaving skb->encapsulation = 1 and the unvalidated skb->inner_transport_header, skb->inner_network_header, and skb->inner_mac_header fields set on the skb. Could this leave bogus encapsulation metadata or arbitrary inner CHECKSUM_PARTIAL offsets on non-GSO packets? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
