Re: [PATCH net v5 4/7] 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: virtio_net_l3_start() allows skb->protocol and eth_hdr() mismatch
- [High] net: virtio_net_hdr_tnl_to_skb() mishandles non-GSO tunnel packets
- [High] net: virtio_net_l3_start() IPv6 inference overwritten by set_proto
- [Medium] net: virtio_net_l4_start() skips IPv6 AH headers without IP6_FH_F_AUTH
--

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

net: resolve uncommon partial checksums at ingress

This commit validates virtio_net_hdr checksum offsets at ingress so that
CHECKSUM_PARTIAL is only retained when csum_start and csum_offset match a
parsed TCP or UDP header. Other non-GSO partial checksums are completed in
software immediately, while unrecognized GSO packets are dropped.

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 013c6c7e2dea0..52852a33a2db7 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -48,6 +52,205 @@ static inline int virtio_net_hdr_set_proto(struct sk_buff *skb,

[ ... ]

> +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;
> +
> +	/* A preset protocol is what the stack will use, except on
> +	 * loopback which re-parses the frame: loopback_xmit() calls
> +	 * eth_type_trans() on it.
> +	 */
> +	if (skb->dev->type == ARPHRD_ETHER ||
> +	    skb->dev->type == ARPHRD_LOOPBACK) {
> +		if (!pskb_may_pull(skb, ETH_HLEN))
> +			return false;
> +		*nhoff = ETH_HLEN;
> +		*proto = skb->protocol;
> +		if (!*proto || skb->dev->type == ARPHRD_LOOPBACK)
> +			*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 when skb->protocol and eth_hdr(skb)->h_proto disagree on an
AF_PACKET socket?

In net/packet/af_packet.c, packet_snd() and tpacket_snd() set skb->protocol
from the user-supplied sll_protocol or socket protocol before calling
virtio_net_hdr_to_skb():

	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());

On an ARPHRD_ETHER device, if userspace presets skb->protocol to
htons(ETH_P_8021Q) while eth_hdr(skb)->h_proto in the frame is
htons(ETH_P_IP), virtio_net_l3_start() ignores eth_hdr(skb)->h_proto and
calls __vlan_get_protocol(), treating the first 4 bytes of the IPv4 header
as a VLAN tag and advancing *nhoff by 4 bytes into a fake header inside the
packet. Conversely, if skb->protocol is htons(ETH_P_IP) while
eth_hdr(skb)->h_proto is htons(ETH_P_8021Q), virtio_net_ip4_parse() parses
the VLAN tag as the start of an IPv4 header.

When the frame is transmitted across virtual ARPHRD_ETHER devices that
forward via __dev_forward_skb2() in net/core/dev.c (such as veth_xmit() ->
veth_forward_skb() -> __dev_forward_skb(), macvlan, ipvlan, or netkit),
skb->protocol is overwritten from eth_hdr(skb)->h_proto:

	if (likely(!ret)) {
		skb->protocol = eth_type_trans(skb, dev);
		skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);
	}

On the other hand, on ARPHRD_LOOPBACK, virtio_net_l3_start() ignores a
preset skb->protocol and validates against eth_hdr(skb)->h_proto, even
though the transmit path prior to loopback_xmit() (such as TC egress and
validate_xmit_skb() -> skb_mac_gso_segment() -> skb_network_protocol())
uses skb->protocol.

Could this allow a packet with mismatched skb->protocol and
eth_hdr(skb)->h_proto to bypass the CHECKSUM_PARTIAL offset validation in
virtio_net_csum_partial_ok()?

> +	} 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) {
> +			const struct iphdr *iph;
> +
> +			/* A packet socket bound to ETH_P_ALL leaves the
> +			 * protocol unset: infer it 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);
> +		}
> +	}
> +	return true;
> +}

[Severity: High]
Should virtio_net_l3_start() also update skb->protocol when it infers the
protocol from the IP version, or should virtio_net_hdr_set_proto() inspect
the IP version as well?

When a packet socket bound to ETH_P_ALL sends an IPv6 packet with
VIRTIO_NET_HDR_F_NEEDS_CSUM and VIRTIO_NET_HDR_GSO_UDP_L4 (or
VIRTIO_NET_HDR_GSO_UDP) on a non-Ethernet device without
header_ops->parse_protocol, __virtio_net_hdr_to_skb() takes the
VIRTIO_NET_HDR_F_NEEDS_CSUM branch and leaves skb->protocol as 0.

virtio_net_l3_start() then infers *proto = htons(ETH_P_IPV6) into its local
variable, and virtio_net_csum_partial_ok_at_offset() validates the packet
as IPv6. However, after virtio_net_hdr_to_skb() returns, packet_snd() and
tpacket_snd() call virtio_net_hdr_set_proto() in include/linux/virtio_net.h:

	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;

Wouldn't this overwrite skb->protocol with htons(ETH_P_IP) after the packet
was validated as IPv6, causing skb_mac_gso_segment() to dispatch the IPv6
packet to inet_gso_segment()?

> +
> +/* 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;
> +			/* Find the transport header behind any
> +			 * extension headers, AH included.
> +			 */
> +			*ip_proto = ipv6_find_hdr(skb, thoff, -1, NULL,
> +						  &flags);
> +			if (*ip_proto < 0 || flags & IP6_FH_F_FRAG)
> +				return false;
> +		}

[Severity: Medium]
Should flags be initialized to IP6_FH_F_AUTH before calling ipv6_find_hdr()
in virtio_net_l4_start()?

When ipv6_find_hdr() in net/ipv6/exthdrs_core.c is called with flags = 0, it
skips NEXTHDR_AUTH headers by ipv6_authlen(hp) bytes. By contrast, packet
dissectors and netfilter helpers such as nft_set_pktinfo_ipv6() in
include/net/netfilter/nf_tables_ipv6.h stop at NEXTHDR_AUTH:

	unsigned int flags = IP6_FH_F_AUTH;
	int protohdr, thoff = 0;
	unsigned short frag_off;

	protohdr = ipv6_find_hdr(pkt->skb, &thoff, -1, &frag_off, &flags);

Because ipv6_find_hdr() only checks the first 2 bytes of the AH header, a
packet carrying NEXTHDR_AUTH can advance *thoff to any 4-byte-aligned offset
past the IPv6 header and keep CHECKSUM_PARTIAL armed across rules that stop
at NEXTHDR_AUTH.

In addition, on local IPv6 receive, ah6_input() in net/ipv6/ah6.c resets
skb->ip_summed = CHECKSUM_NONE without completing CHECKSUM_PARTIAL before
verifying the AH ICV over the inner transport header, and IPv6 GSO offload
in ipv6_gso_pull_exthdrs() does not support NEXTHDR_AUTH. Wouldn't stopping
at NEXTHDR_AUTH via IP6_FH_F_AUTH allow non-GSO AH packets to have their
checksum completed in software at ingress and reject AH GSO packets,
matching the IPv4 behavior?

[ ... ]

> @@ -368,7 +628,22 @@ 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.
> +	 */
> +	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 __virtio_net_resolve_csum_partial_mismatch(skb);
>  }

[Severity: High]
How are non-GSO packets with a tunnel header handled in
virtio_net_hdr_tnl_to_skb()?

In __virtio_net_hdr_to_skb(), if skb->len - nh_off <= gso_size,
shinfo->gso_size is not set, so skb_is_gso(skb) is false when control
returns to virtio_net_hdr_tnl_to_skb().

If !skb_is_gso(skb) and virtio_net_csum_partial_ok_at_offset() returns true,
virtio_net_hdr_tnl_to_skb() returns 0 while keeping CHECKSUM_PARTIAL at the
caller-chosen vhdr->inner_nh_offset + inner_th inside the UDP payload (for
example, pointing at a fake IPv4 header placed inside the UDP payload or
overlapping a VXLAN inner Ethernet header), even though GSO segmentation
will not run. If VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM was set, clearing
SKB_GSO_UDP_TUNNEL_CSUM also leaves the outer UDP checksum uncomputed.

On the other hand, if !skb_is_gso(skb) and
virtio_net_csum_partial_ok_at_offset() returns false, the comment states
that a tunnel header implies GSO so the packet is dropped, but
__virtio_net_resolve_csum_partial_mismatch() only returns -EINVAL when
skb_is_gso(skb) is true:

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;
}

When !skb_is_gso(skb), __virtio_net_resolve_csum_partial_mismatch()
completes the checksum via skb_checksum_help() and returns 1, leaving
skb->encapsulation = 1 and the unvalidated skb->inner_transport_header,
skb->inner_network_header, and skb->inner_mac_header fields set on the skb.
Could this leave bogus encapsulation metadata or arbitrary inner
CHECKSUM_PARTIAL offsets on non-GSO packets?

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