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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: skb->protocol mismatch in virtio_net_l3_start() when unset
- [High] net: uncompleted csum in virtio_net_hdr_tnl_to_skb() for non-GSO
--

commit e848d2c54d731e54131a7b7019dc30d4e0367ad9
Author: Michael S. Tsirkin <mst@redhat.com>

net: resolve uncommon partial checksums at ingress

This patch validates csum_start and csum_offset in virtio_net_hdr_to_skb() and
virtio_net_hdr_tnl_to_skb() against parsed TCP and UDP headers, completing
partial checksums in software for non-GSO packets when validation fails and
dropping unrecognized GSO packets.

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 013c6c7e2dea0..54048759cc6ec 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h

[ ... ]

> +static inline bool
> +virtio_net_l3_start(struct sk_buff *skb, int *nhoff, __be16 *proto)
> +{
> +	/* skb->dev should always be set here already,
> +	 * but let's not crash when it is not.
> +	 */
> +	if (WARN_ON_ONCE(!skb->dev))
> +		return false;
> +
> +	if (skb->dev->type == ARPHRD_ETHER ||
> +	    skb->dev->type == ARPHRD_LOOPBACK) {
> +		__be16 h_proto;
> +
> +		if (!pskb_may_pull(skb, ETH_HLEN))
> +			return false;
> +		h_proto = eth_hdr(skb)->h_proto;
> +		/* skb->protocol is a sender claim, and parts of the
> +		 * stack parse the packet per it: when it disagrees with
> +		 * the frame we do not know which view the packet will
> +		 * be parsed as, so do not validate.
> +		 */
> +		if (skb->protocol && skb->protocol != h_proto)
> +			return false;
> +		*nhoff = ETH_HLEN;
> +		*proto = h_proto;
> +		if (eth_type_vlan(*proto)) {
> +			*proto = __vlan_get_protocol(skb, *proto, nhoff);
> +			if (!*proto)
> +				return false;
> +		}
> +	} else {
> +		const struct iphdr *iph;
> +
> +		*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;
> +		/* No L2 header to cross-check with: take the protocol
> +		 * 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);
> +		else
> +			return false;
> +		if (skb->protocol && skb->protocol != *proto)
> +			return false;
> +	}
> +	return true;
> +}

[Severity: High]
Can virtio_net_l3_start() validate a packet as ETH_P_IPV6 when skb->protocol
is unset (0), only for the caller to later set skb->protocol to ETH_P_IP or
leave it as 0?

In net/packet/af_packet.c, both packet_snd() and tpacket_snd() invoke
virtio_net_hdr_to_skb() before setting skb->protocol:

net/packet/af_packet.c:packet_snd() {
	...
	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);

	if (vnet_hdr_sz)
		virtio_net_hdr_set_proto(skb, &vnet_hdr);
	...
}

When a packet is sent with proto == 0 (or htons(ETH_P_ALL)) on a
non-ARPHRD_ETHER, non-ARPHRD_LOOPBACK device (such as tun in IFF_TUN mode,
sit, ipip, gre, or wg), skb->protocol is 0 when virtio_net_hdr_to_skb() runs.

If the L3 header has iph->version == 6, virtio_net_l3_start() sets
*proto = htons(ETH_P_IPV6) and allows skb->protocol == 0. Then
virtio_net_csum_partial_ok_at_offset() walks the IPv6 header and any IPv6
extension headers, and for VIRTIO_NET_HDR_GSO_UDP or
VIRTIO_NET_HDR_GSO_UDP_L4 it only checks ip_proto == IPPROTO_UDP:

include/linux/virtio_net.h:virtio_net_csum_partial_ok_at_offset() {
	...
	case VIRTIO_NET_HDR_GSO_UDP:
	case VIRTIO_NET_HDR_GSO_UDP_L4:
		return ip_proto == IPPROTO_UDP;
	...
}

After virtio_net_hdr_to_skb() succeeds and retains CHECKSUM_PARTIAL at the
IPv6 transport offset, packet_parse_headers() leaves skb->protocol as 0 on
devices without header_ops->parse_protocol (or SOCK_DGRAM), and
virtio_net_hdr_set_proto() unconditionally overwrites skb->protocol with
ETH_P_IP:

include/linux/virtio_net.h:virtio_net_hdr_set_proto() {
	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;
	...
}

The packet then enters packet_xmit() with skb->protocol == htons(ETH_P_IP)
despite having been validated as IPv6, so inet_gso_segment() and flow
dissection parse the IPv6 header as IPv4 while csum_start points to the IPv6
extension-header-derived offset. Similarly, when hdr->gso_type is
VIRTIO_NET_HDR_GSO_NONE in packet_snd() / tpacket_snd(), or when pi.proto == 0
in tun_get_user() on an IFF_TUN device without IFF_NO_PI, skb->protocol
remains 0 after validation.

Should virtio_net_l3_start() reject skb->protocol == 0 (or ensure
skb->protocol is populated with the validated protocol)?

[ ... ]

> @@ -335,9 +604,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
> @@ -352,9 +624,29 @@ 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);
> +
> +	/*
> +	 * We know VIRTIO_NET_HDR_F_NEEDS_CSUM was set so __virtio_net_hdr_to_skb
> +	 * set CHECKSUM_PARTIAL.
> +	 */
> +	if (unlikely(!virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type,
> +							  inner_nh,
> +							  inner_proto))) {
> +		/* __virtio_net_hdr_to_skb set the transport header to
> +		 * csum_start, restore it to a known good value.
> +		 */
> +		skb->transport_header = old_transport;
> +
> +		return __virtio_net_resolve_csum_partial_mismatch(skb);
> +	}
> +
> +	skb_set_inner_protocol(skb, inner_proto);
>  	if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
> +		/* Checked here, after the validation: a packet completed
> +		 * in software never uses the feature, so an unnegotiated
> +		 * request only matters for metadata good enough to act on.
> +		 */
>  		if (!tnl_csum_negotiated)
>  			return -EINVAL;
>  
> @@ -368,6 +660,21 @@ 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;
> +
> +	if (!skb_is_gso(skb)) {
> +		/* Too small to segment: complete the checksum in place
> +		 * instead of dropping - senders do tag small packets
> +		 * with GSO headers, e.g. packetdrill sets gso_type and
> +		 * gso_size with --mss, whatever the packet size.
> +		 * The outer UDP checksum is not calculated, but a zero
> +		 * one is fine: UDP over IPv4 allows a disabled
> +		 * checksum, and so do tunnels over IPv6 where the
> +		 * receiver opts in (RFC 6935, RFC 6936).
> +		 */
> +		skb_shinfo(skb)->gso_type &= ~(SKB_GSO_UDP_TUNNEL_CSUM |
> +					       SKB_GSO_UDP_TUNNEL);
> +	}
> +
>  	return 0;
>  }

[Severity: High]
In virtio_net_hdr_tnl_to_skb(), when a packet tagged with a tunnel GSO type is
too small to segment (!skb_is_gso(skb)), the comment states "Too small to
segment: complete the checksum in place instead of dropping", but doesn't this
block only clear the tunnel GSO flags while leaving CHECKSUM_PARTIAL
uncompleted?

When __virtio_net_hdr_to_skb() sees skb->len - nh_off <= gso_size, it does not
set shinfo->gso_size, so skb_is_gso(skb) is false. If
virtio_net_csum_partial_ok_at_offset() succeeds, virtio_net_hdr_tnl_to_skb()
reaches the !skb_is_gso(skb) block without calling skb_checksum_help() (or
__virtio_net_resolve_csum_partial_mismatch()):

1. Because inner_nh (from vhdr->inner_nh_offset) is an untrusted offset inside
   the outer UDP payload and no tunnel header between outer_th and inner_nh is
   verified, a sender can place a fake 20-byte IPv4 header inside the UDP
   payload of a non-GSO UDP packet with csum_start = inner_nh + 20. Both
   virtio_net_tnl_offsets_ok() and virtio_net_csum_partial_ok_at_offset()
   succeed, SKB_GSO_UDP_TUNNEL is cleared, and the non-GSO UDP packet retains
   CHECKSUM_PARTIAL at an arbitrary offset inside its UDP payload (which can
   later overwrite 2 bytes of payload after netfilter/BPF inspection and causes
   udp4_csum_init() / udp6_csum_init() to treat the outer UDP checksum as
   unnecessary).

2. If VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM was set, clearing
   SKB_GSO_UDP_TUNNEL_CSUM without completing the inner checksum leaves the
   outer UDP header with an uncompleted non-zero pseudo-header checksum while
   CHECKSUM_PARTIAL still points to the inner transport header.

3. skb->encapsulation remains set to 1 even though SKB_GSO_UDP_TUNNEL and
   SKB_GSO_UDP_TUNNEL_CSUM are cleared.

4. Conversely, if a !skb_is_gso(skb) packet fails virtio_net_tnl_offsets_ok()
   or the inner_th < inner_nh + inner_l3min check earlier in
   virtio_net_hdr_tnl_to_skb(), it returns -EINVAL instead of restoring
   skb->transport_header = old_transport and completing the checksum via
   __virtio_net_resolve_csum_partial_mismatch(skb).

Should virtio_net_hdr_tnl_to_skb() resolve !skb_is_gso(skb) packets via
__virtio_net_resolve_csum_partial_mismatch(skb) after restoring
skb->transport_header = old_transport?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/305bcadb19f4a539a318c6b87c829fbe43d0ffdb.1793276116.git.mst@redhat.com?part=4