Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] net: virtio_net_hdr_to_skb() runs skb_checksum_help() before skb_zcopy_init() - [High] net: skb->protocol ignored on Ethernet/loopback in virtio_net_l3_start() - [High] virtio_net: csum validation bypass on non-GSO skb in virtio_net_hdr_tnl_to_skb() --
commit 69b2836e804af5188d86e52f3cfbb8777bb548d1 Author: Michael S. Tsirkin <[email protected]> net: resolve uncommon partial checksums at ingress This patch validates csum_start and csum_offset on incoming packets with virtio net headers, retaining CHECKSUM_PARTIAL only when the offsets match a parsed TCP or UDP header. Non-GSO packets with unrecognized partial checksum offsets have their checksums 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..ec6f47ce1c974 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h > @@ -48,6 +52,202 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff > *skb, > return 0; > } > > +/* TODO: nothing virtio specific here, move to ip.h */ > + > +/* > + * Pull and minimally validate the IPv4 header at @nhoff. > + * Returns the header and stores the transport header offset in > + * @thoff, or NULL if truncated or malformed. Only the fixed header > + * is guaranteed linear; callers that need the options or the > + * transport header must pull further. > + */ > +static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb, > + unsigned int nhoff, > + unsigned int *thoff) > +{ > + struct iphdr *iph; > + > + if (!pskb_may_pull(skb, nhoff + sizeof(struct iphdr))) > + return NULL; > + > + iph = (struct iphdr *)(skb->data + nhoff); > + if (iph->version != 4 || > + iph->ihl < sizeof(struct iphdr) / sizeof(u32)) > + return NULL; > + > + *thoff = nhoff + iph->ihl * sizeof(u32); > + return iph; > +} > + > +/* 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; > + > + /* Loopback carries ethernet frames, too: > + * loopback_xmit() calls eth_type_trans() on them. > + */ > + if (skb->dev->type == ARPHRD_ETHER || > + skb->dev->type == ARPHRD_LOOPBACK) { > + 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; > + } [Severity: High] What happens here when an AF_PACKET SOCK_RAW socket with PACKET_VNET_HDR sends a frame whose skb->protocol disagrees with eth_hdr(skb)->h_proto? In net/packet/af_packet.c, both packet_snd() and tpacket_snd() (via tpacket_fill_skb()) set skb->protocol from the socket or sll_protocol and place skb->network_header at ETH_HLEN before calling virtio_net_hdr_to_skb(), and then call packet_parse_headers(): net/packet/af_packet.c:packet_snd() { ... 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()); if (err < 0) goto out_free; len += vnet_hdr_sz; } packet_parse_headers(skb, sock); ... } In packet_parse_headers(), a pre-set skb->protocol is preserved, and skb->network_header is only advanced past VLAN tags when skb->dev->type is ARPHRD_ETHER and 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); } Because virtio_net_l3_start() ignores a pre-set skb->protocol on ARPHRD_ETHER and ARPHRD_LOOPBACK devices and unconditionally parses eth_hdr(skb)->h_proto and VLAN tags via __vlan_get_protocol(), a sender can set sll_protocol to htons(ETH_P_IP) while placing htons(ETH_P_8021Q) in eth_hdr(skb)->h_proto (or send a VLAN-tagged frame on ARPHRD_LOOPBACK). In that case, virtio_net_l3_start() advances *nhoff to 18 and validates csum_start and csum_offset against a crafted IPv4 and UDP/TCP header embedded inside the real IPv4 header at offset 14, keeping CHECKSUM_PARTIAL on the skb. Downstream paths that rely on skb->protocol and skb_network_offset(skb) at ETH_HLEN (such as nf_hook_egress(), sch_handle_egress(), ipvlan_process_outbound(), and skb_mac_gso_segment()) will then inspect or segment the packet at offset 14 before checksum completion overwrites the two bytes at csum_start + csum_offset. Should virtio_net_l3_start() honor a pre-set skb->protocol (and avoid advancing past VLAN headers on ARPHRD_LOOPBACK when skb->network_header will remain at ETH_HLEN) so that checksum validation parses the same L3 header and offset as the rest of the stack? [ ... ] > +/* > + * 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. > + */ > +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; > +} > + > static inline int __virtio_net_hdr_to_skb(struct sk_buff *skb, > const struct virtio_net_hdr *hdr, > bool little_endian, u8 hdr_gso_type) > @@ -194,11 +394,35 @@ static inline int __virtio_net_hdr_to_skb(struct > sk_buff *skb, > return 0; > } > > +/* > + * Return value: > + * 0 - success > + * >0 - checksum was completed in software (slow path) > + * <0 - failure > + */ > static inline int virtio_net_hdr_to_skb(struct sk_buff *skb, > const struct virtio_net_hdr *hdr, > bool little_endian) > { > - return __virtio_net_hdr_to_skb(skb, hdr, little_endian, hdr->gso_type); > + typeof(skb->transport_header) old_transport = skb->transport_header; > + int ret = __virtio_net_hdr_to_skb(skb, hdr, little_endian, > + hdr->gso_type); > + > + if (ret < 0) > + return ret; > + > + if (skb->ip_summed != CHECKSUM_PARTIAL || > + virtio_net_csum_partial_ok(skb, hdr->gso_type)) > + return 0; > + > + /* > + * Here we do not trust the checksum metadata. > + * __virtio_net_hdr_to_skb set the transport header to csum_start > + * from virtio hdr, restore it to a known good value. > + */ > + skb->transport_header = old_transport; > + > + return __virtio_net_resolve_csum_partial_mismatch(skb); > } [Severity: High] Can calling skb_checksum_help() inside virtio_net_hdr_to_skb() bypass the shared fragment linearization check on zerocopy SKBs? In net/core/dev.c, skb_checksum_help() checks skb_has_shared_frag(skb) (which tests SKBFL_SHARED_FRAG in skb_shinfo(skb)->flags) and linearizes the skb so that userspace-mapped pages cannot be mutated during or after checksum calculation: 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; } ... *(__sum16 *)(skb->data + offset) = csum_fold(csum) ?: CSUM_MANGLED_0; out_set_summed: skb->ip_summed = CHECKSUM_NONE; } However, in tap_get_user() (drivers/net/tap.c), tun_get_user() (drivers/net/tun.c), and tpacket_snd() (net/packet/af_packet.c), userspace pages are attached to skb_shinfo(skb)->frags[] first (via zerocopy_sg_from_iter() or tpacket_fill_skb()), then virtio_net_hdr_to_skb() is called, and skb_zcopy_init() (which sets SKBFL_ZEROCOPY_FRAG, i.e., SKBFL_ZEROCOPY_ENABLE | SKBFL_SHARED_FRAG) is only called afterwards: drivers/net/tap.c:tap_get_user() { ... if (zerocopy) err = zerocopy_sg_from_iter(skb, from); ... 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); } } When a non-GSO zerocopy packet takes the slow path in virtio_net_hdr_to_skb() -> __virtio_net_resolve_csum_partial_mismatch() -> skb_checksum_help(), SKBFL_SHARED_FRAG is not yet set on the skb. As a result, skb_has_shared_frag(skb) evaluates to false, __skb_linearize(skb) is skipped, skb_checksum_help() computes the checksum directly over writable userspace pages and sets skb->ip_summed = CHECKSUM_NONE, and skb_zcopy_init() then marks the fragments as shared after the checksum has already been finalized. [ ... ] > @@ -335,9 +589,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 +610,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 +625,18 @@ 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; > + > + /* A tunnel header implies GSO, so drop. */ > + return -EINVAL; > } [Severity: High] Does virtio_net_hdr_tnl_to_skb() need to verify that skb_is_gso(skb) is true after calling __virtio_net_hdr_to_skb()? In __virtio_net_hdr_to_skb(), when skb->len - nh_off <= gso_size, the packet is treated as non-GSO and shinfo->gso_size and shinfo->gso_type are left as 0, while skb->ip_summed remains CHECKSUM_PARTIAL at hdr->csum_start: 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; } } return 0; } If a sender on tun (tun_get_user() or tun_xdp_one()) or virtio_net (virtnet_receive_done()) submits a small UDP packet with a tunnel GSO type, VIRTIO_NET_HDR_F_NEEDS_CSUM, and gso_size >= skb->len - nh_off, __virtio_net_hdr_to_skb() returns 0 with skb_is_gso(skb) false. virtio_net_hdr_tnl_to_skb() then sets SKB_GSO_UDP_TUNNEL (or SKB_GSO_UDP_TUNNEL_CSUM) on the non-GSO skb and validates csum_start against the untrusted vhdr->inner_nh_offset inside the UDP payload via virtio_net_csum_partial_ok_at_offset(). By placing a fake 20-byte IPv4 header stub at inner_nh_offset inside the UDP payload and setting csum_start to inner_nh + 20, the packet passes validation and enters the stack as a non-GSO UDP packet with CHECKSUM_PARTIAL pointing inside the UDP payload (and with shinfo->gso_type non-zero while shinfo->gso_size is 0, which also triggers WARN_ON_ONCE(1) in tun_vnet_hdr_tnl_from_skb() if forwarded to a tun device without NETIF_F_GSO_UDP_TUNNEL). [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
