Re: [PATCH net v3 5/8] net: resolve uncommon partial checksums at ingress

[sashiko-bot@kernel.org]

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

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