Re: [PATCH net v6 4/5] net: resolve uncommon partial checksums at ingress

[sashiko-bot@kernel.org]

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 <mst@redhat.com>

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/cover.1792768096.git.mst@redhat.com?part=4