Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] net: skb_checksum_help() in virtio_net_hdr_to_skb() skips shared frag copy - [High] net: virtio_net_hdr_tnl_to_skb() corrupts non-GSO skb state on tunnel hdr - [High] net: virtio_net_l4_start() skips IPv6 NEXTHDR_AUTH by omitting IP6_FH_F_AUTH - [High] net: virtio_net_l3_start() L2/L3 offset and protocol mismatch on lo and AF_PACKET --
commit 338a1355fe27c4686a83ea9e73bf390729be2308 Author: Michael S. Tsirkin <[email protected]> net: resolve uncommon partial checksums at ingress This commit validates csum_start and csum_offset at ingress so that CHECKSUM_PARTIAL is only retained when matching a parsed TCP or UDP header. Non-GSO packets with unrecognized checksum offsets are completed immediately in software via skb_checksum_help(), while unrecognized GSO packets are dropped. > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > index 013c6c7e2dea0..97b554ef7123f 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h [ ... ] > @@ -48,6 +52,198 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff > *skb, [ ... ] > +/* 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 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; > + > + if (skb->dev->type == ARPHRD_ETHER) { > + if (!pskb_may_pull(skb, ETH_HLEN)) > + return false; > + *nhoff = ETH_HLEN; > + *proto = eth_hdr(skb)->h_proto; > + if (eth_type_vlan(*proto)) { > + *proto = __vlan_get_protocol(skb, *proto, nhoff); > + if (!*proto) > + return false; > + } > + } 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) { > + u8 version; > + > + /* A packet socket bound to ETH_P_ALL leaves the > + * protocol unset: infer it from the IP version > + * nibble at the network header. > + */ > + if (!pskb_may_pull(skb, *nhoff + 1)) > + return false; > + version = skb->data[*nhoff] >> 4; > + if (version == 4) > + *proto = htons(ETH_P_IP); > + else if (version == 6) > + *proto = htons(ETH_P_IPV6); > + } > + } > + return true; > +} [Severity: High] Can virtio_net_l3_start() disagree with the networking stack on the L3 offset and protocol when packets are sent via AF_PACKET (packet_snd() or tpacket_snd()) with PACKET_VNET_HDR enabled? First, on loopback (where dev->type is ARPHRD_LOOPBACK, but dev->hard_header_len is ETH_HLEN and loopback_xmit() calls eth_type_trans()), virtio_net_l3_start() takes the else branch and sets *nhoff = skb_network_offset(skb) (14) without inspecting eth_hdr(skb) or VLAN tags: drivers/net/loopback.c:loopback_xmit() { skb->protocol = eth_type_trans(skb, dev); } If a packet on lo has eth_hdr(skb)->h_proto = htons(ETH_P_8021Q) and a 4-byte 802.1P priority tag (VLAN ID 0) with h_vlan_TCI = htons(0x6000) and h_vlan_encapsulated_proto = htons(ETH_P_IP), virtio_net_l3_start() and virtio_net_l4_start() can validate a fake IPv6 header starting at the VLAN tag (offset 14), while loopback_xmit() -> __netif_receive_skb_core() -> skb_vlan_untag() strips the 4-byte tag and delivers the inner IPv4 packet at offset 18 with CHECKSUM_PARTIAL retained at the unvalidated offset. Second, on ARPHRD_ETHER devices, virtio_net_l3_start() always uses eth_hdr(skb)->h_proto and advances *nhoff past VLAN tags via __vlan_get_protocol(), whereas AF_PACKET SOCK_RAW preserves a user-supplied sll_protocol in skb->protocol and packet_parse_headers() only advances skb->network_header when eth_type_vlan(skb->protocol) is true: net/packet/af_packet.c:packet_parse_headers() { if (!skb->protocol && sock->type == SOCK_RAW) skb->protocol = dev_parse_header_protocol(skb); skb_probe_transport_header(skb); /* Move network header to the right position for VLAN tagged packets */ if (likely(skb->dev->type == ARPHRD_ETHER) && eth_type_vlan(skb->protocol) && vlan_get_protocol_and_depth(skb, skb->protocol, &depth) != 0) skb_set_network_header(skb, depth); } If sll_protocol is ETH_P_IP while eth_hdr(skb)->h_proto is ETH_P_8021Q (or vice versa), wouldn't virtio_net_l3_start() validate csum_start against a different protocol and network header offset than what skb->protocol and skb->network_header represent for downstream processing? > + > +/* 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; > + *ip_proto = ipv6_find_hdr(skb, thoff, -1, NULL, > + &flags); > + if (*ip_proto < 0 || flags & IP6_FH_F_FRAG) > + return false; > + } > + return true; > + } > + > + return false; > +} [Severity: High] Should flags in virtio_net_l4_start() be initialized to IP6_FH_F_AUTH before calling ipv6_find_hdr()? In ipv6_find_hdr(), NEXTHDR_AUTH is only treated as a terminal header when IP6_FH_F_AUTH is passed in flags; otherwise it skips ipv6_authlen(hp) bytes and continues walking into the following header: net/ipv6/exthdrs_core.c:ipv6_find_hdr() { } else if (nexthdr == NEXTHDR_AUTH) { if (flags && (*flags & IP6_FH_F_AUTH) && (target < 0)) break; hdrlen = ipv6_authlen(hp); } else } Downstream parsers such as nft_set_pktinfo_ipv6() (which sets flags = IP6_FH_F_AUTH) and __skb_flow_dissect() stop at NEXTHDR_AUTH rather than skipping it. With flags = 0, an IPv6 packet carrying NEXTHDR_AUTH followed by TCP or UDP passes virtio_net_csum_partial_ok_at_offset() and retains CHECKSUM_PARTIAL, allowing later checksum completion to overwrite the 2 bytes at csum_start + csum_offset after netfilter or flow-dissector inspection, and also allowing IPv6+AH GSO packets through even though ipv6_gso_pull_exthdrs() does not support IPPROTO_AH. [ ... ] > +/* > + * Handle a packet whose checksum metadata does not match the packet: > + * GSO packets are dropped, as segmentation needs one checksum per > + * segment; others are completed in software. > + * > + * Return 1 if the checksum was completed in software, else a negative > + * errno. > + */ > +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; > +} [Severity: High] When virtio_net_resolve_csum_partial_mismatch() calls skb_checksum_help() from virtio_net_hdr_to_skb() or virtio_net_hdr_tnl_to_skb(), does skb_checksum_help() miss copying shared zerocopy fragments because skb_zcopy_init() has not been called yet? In skb_checksum_help(), shared fragments are linearized before computing the checksum so that userspace cannot modify the payload pages after the checksum is calculated: net/core/dev.c:skb_checksum_help() { /* Before computing a checksum, we should make sure no frag could * be modified by an external entity : checksum could be wrong. */ if (skb_has_shared_frag(skb)) { ret = __skb_linearize(skb); if (ret) goto out; } } However, skb_has_shared_frag(skb) checks SKBFL_SHARED_FRAG in skb_shinfo(skb)->flags, which is only set when skb_zcopy_init() runs. In tun_get_user(), tap_get_user(), and tpacket_snd(), the userspace pages are attached to the skb first, then virtio_net_hdr_to_skb() (or tun_vnet_hdr_tnl_to_skb() / tun_vnet_hdr_to_skb()) is called, and skb_zcopy_init() is only called afterwards: drivers/net/tun.c:tun_get_user() { err = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr); ... /* copy skb_ubuf_info for callback when skb has no error */ if (zerocopy) { skb_zcopy_init(skb, msg_control); } } drivers/net/tap.c:tap_get_user() { if (vnet_hdr_len) { err = tun_vnet_hdr_to_skb(q->flags, skb, &vnet_hdr); ... } ... /* copy skb_ubuf_info for callback when skb has no error */ if (zerocopy) { skb_zcopy_init(skb, msg_control); } } net/packet/af_packet.c:tpacket_snd() { if (has_vnet_hdr) { if (virtio_net_hdr_to_skb(skb, &vnet_hdr, vio_le()) < 0) { tp_len = -EINVAL; goto tpacket_error; } } ... uarg->ubuf.flags = SKBFL_ZEROCOPY_FRAG; refcount_set(&uarg->ubuf.refcnt, 1); /* Hold a sk_wmem_alloc reference until completion */ refcount_inc(&po->sk.sk_wmem_alloc); skb_zcopy_init(skb, &uarg->ubuf); } Because SKBFL_SHARED_FRAG is not set yet when virtio_net_resolve_csum_partial_mismatch() calls skb_checksum_help(), skb_has_shared_frag(skb) returns false, __skb_linearize(skb) is skipped, and the packet proceeds with skb->ip_summed = CHECKSUM_NONE while its fragments remain backed by writable userspace pages. [ ... ] > @@ -335,9 +586,12 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, > /* Let the basic parsing deal with plain GSO features. */ > ret = __virtio_net_hdr_to_skb(skb, hdr, true, > hdr->gso_type & ~gso_tunnel_type); > - if (ret) > + if (ret < 0) > return ret; > > + if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th)) > + return -EINVAL; > + > /* In case of USO, the inner protocol is still unknown and > * `inner_isv6` is just a guess, additional parsing is needed. > * The previous validation ensures that accessing an ipv4 inner > @@ -353,7 +607,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, > } > > skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) : > - htons(ETH_P_IP)); > + htons(ETH_P_IP)); > if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) { > if (!tnl_csum_negotiated) > return -EINVAL; > @@ -368,7 +622,17 @@ 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; > + > + /* > + * 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; > + > + return virtio_net_resolve_csum_partial_mismatch(skb); > } [Severity: High] What happens in virtio_net_hdr_tnl_to_skb() when a packet carries a tunnel GSO type in hdr->gso_type, but its length is not greater than gso_size? In __virtio_net_hdr_to_skb(), small packets do not populate shinfo->gso_size or shinfo->gso_type: include/linux/virtio_net.h:__virtio_net_hdr_to_skb() { /* Too small packets are not really GSO ones. */ if (skb->len - nh_off > gso_size) { shinfo->gso_size = gso_size; shinfo->gso_type = gso_type; /* Header must be checked, and gso_segs computed. */ shinfo->gso_type |= SKB_GSO_DODGY; shinfo->gso_segs = 0; } } Because shinfo->gso_size remains 0, skb_is_gso(skb) is false, but virtio_net_hdr_tnl_to_skb() still sets shinfo->gso_type |= SKB_GSO_UDP_TUNNEL_CSUM (or SKB_GSO_UDP_TUNNEL), sets inner header offsets from vhdr->inner_nh_offset and hdr->csum_start, overwrites skb->transport_header, and sets skb->encapsulation = 1. If virtio_net_csum_partial_ok_at_offset() then returns false, virtio_net_resolve_csum_partial_mismatch(skb) sees !skb_is_gso(skb), completes the inner checksum via skb_checksum_help(), and returns 1 without cleaning up the tunnel state on the skb. That leaves the non-GSO skb with skb->encapsulation = 1, unvalidated inner header offsets, a non-zero shinfo->gso_type with gso_size == 0 (which triggers WARN_ON_ONCE(1) in tun_vnet_hdr_tnl_from_skb() if forwarded to another tun/virtio interface), and an uncomputed outer UDP checksum when VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM was set. Conversely, if the payload at the caller-chosen vhdr->inner_nh_offset happens to look like a valid IP + TCP/UDP header, virtio_net_csum_partial_ok_at_offset() returns true on this non-GSO UDP packet, allowing CHECKSUM_PARTIAL to be retained at an arbitrary offset inside the UDP payload. Should virtio_net_hdr_tnl_to_skb() reject packets where !skb_is_gso(skb) after __virtio_net_hdr_to_skb()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
