Re: [PATCH net v4 4/7] net: resolve uncommon partial checksums at ingress

[sashiko-bot@kernel.org]

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

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