Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] net: virtio_net_hdr_tnl_to_skb() retains untrusted inner state and csum --
commit fe9720049eff45b73b2666232bd03b5f28b7c81f Author: Michael S. Tsirkin <[email protected]> net: resolve uncommon partial checksums at ingress This commit validates virtio_net_hdr checksum offsets against parsed L3 and L4 headers on ingress, completing non-GSO checksums in software and dropping GSO packets when the checksum metadata does not match a known TCP or UDP header. > diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h > index 013c6c7e2dea0..14120710d0753 100644 > --- a/include/linux/virtio_net.h > +++ b/include/linux/virtio_net.h [ ... ] > @@ -352,8 +621,8 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb, > return -EINVAL; > } > > - skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) : > - htons(ETH_P_IP)); > + inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP); > + skb_set_inner_protocol(skb, inner_proto); > if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) { > if (!tnl_csum_negotiated) > return -EINVAL; > @@ -368,7 +637,20 @@ 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. > + */ > + 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] When a packet arrives through tun_get_user() -> tun_vnet_hdr_tnl_to_skb() or virtnet_receive_done() -> virtio_net_hdr_tnl_to_skb() with VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 or VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 and a gso_size large enough that __virtio_net_hdr_to_skb() does not mark the skb as GSO (skb->len - nh_off <= gso_size, so !skb_is_gso(skb)), can this handling in virtio_net_hdr_tnl_to_skb() leave untrusted inner header state and partial checksums active on non-GSO packets? First, when !skb_is_gso(skb) and virtio_net_csum_partial_ok_at_offset() returns false because the inner offsets or headers are invalid, __virtio_net_resolve_csum_partial_mismatch() completes the checksum via skb_checksum_help() and returns 1. However, virtio_net_hdr_tnl_to_skb() has already set skb->encapsulation = 1, skb->inner_protocol, skb->inner_transport_header, skb->inner_network_header, and skb->inner_mac_header from the unvalidated vhdr->inner_nh_offset and hdr->csum_start. If that non-GSO packet is later forwarded into an IP tunnel, iptunnel_handle_offloads() in net/ipv4/ip_tunnel_core.c checks: if (likely(!skb->encapsulation)) { skb_reset_inner_headers(skb); skb->encapsulation = 1; } Because skb->encapsulation is already 1, skb_reset_inner_headers() is skipped and ip_tunnel_xmit() / ip_md_tunnel_xmit() read skb_inner_network_header(skb) at the unvalidated inner_nh offset. Drivers and stack paths that inspect skb->encapsulation on non-GSO packets, such as mlx4_en_xmit() in drivers/net/ethernet/mellanox/mlx4/en_tx.c: if (skb->encapsulation) { ... ip.hdr = skb_inner_network_header(skb); proto = (ip.v4->version == 4) ? ip.v4->protocol : ip.v6->nexthdr; as well as netif_skb_features(), i40e_features_check(), ice_features_check(), udp_gro_receive(), xfrm_get_inner_ipproto(), geneve_rx(), skb_mpls_push(), and bpf_skb_adjust_room(), will also act on the unvalidated inner header offsets. Second, virtio_net_tnl_offsets_ok() only verifies that the outer L3 header is followed by UDP at outer_th, while inner_nh is taken directly from the untrusted vhdr->inner_nh_offset (requiring only inner_nh >= outer_th + sizeof(struct udphdr)). On a non-GSO UDP packet (!skb_is_gso(skb)), can a sender place fake IP and UDP headers at a chosen offset inner_nh inside the UDP payload (or inside an inner Ethernet/VXLAN header) so that virtio_net_csum_partial_ok_at_offset() succeeds and returns 0, keeping CHECKSUM_PARTIAL at an arbitrary offset inside the UDP payload where later checksum completion modifies the packet after protocol or firewall inspection? Third, when !skb_is_gso(skb) and virtio_net_csum_partial_ok_at_offset() returns true, clearing SKB_GSO_UDP_TUNNEL_CSUM drops the outer UDP tunnel checksum request while leaving skb->ip_summed == CHECKSUM_PARTIAL pointing to inner_th. If the packet is delivered locally to udp_rcv() -> udp4_csum_init(), skb_csum_unnecessary() in include/linux/skbuff.h: return ((skb->ip_summed == CHECKSUM_UNNECESSARY) || skb->csum_valid || (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_start_offset(skb) >= 0)); evaluates to true at the outer UDP layer while udp_unexpected_gso() is false, skipping outer UDP checksum validation and leaving the inner checksum uncompleted. Should virtio_net_hdr_tnl_to_skb() reject or avoid setting tunnel encapsulation and inner CHECKSUM_PARTIAL state when !skb_is_gso(skb), and only populate skb->encapsulation and skb->inner_* after the inner headers have been validated? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
