Re: [PATCH net v5 7/7] virtio_net: cleanup protocol calculation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: use-after-free read of iph->ihl after pskb_may_pull() in ip4_parse()
- [High] virtio_net: failed drop of invalid non-GSO tunnel packets in virtio_net_hdr_tnl_to_skb()
--

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

virtio_net: cleanup protocol calculation

This commit removes a duplicate assignment of inner_proto in
virtio_net_hdr_tnl_to_skb() by computing it once before calling
skb_set_inner_protocol().

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 063cc116e51a1..e984e446ba44a 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -613,8 +613,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;
> @@ -638,7 +638,6 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  	 * 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;

[Severity: High]
When virtio_net_csum_partial_ok_at_offset() validates an IPv4 header via
virtio_net_l4_start() -> ip4_parse() (introduced earlier in this series in
commit f0109d51e8b0 ("net: add ip4_parse()")), can ip4_parse() in
include/linux/ip.h perform a use-after-free read of iph->ihl?

In include/linux/ip.h:ip4_parse():

	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;

	/* Pull the options as well: the transport header offset
	 * returned below must be within the linear data.
	 */
	if (!pskb_may_pull(skb, nhoff + iph->ihl * sizeof(u32)))
		return NULL;

	*thoff = nhoff + iph->ihl * sizeof(u32);
	return (struct iphdr *)(skb->data + nhoff);

When an IPv4 packet has options (iph->ihl > 5) that reside in nonlinear
paged data (so nhoff + iph->ihl * sizeof(u32) > skb_headlen(skb)), or when
the skb is cloned, the second pskb_may_pull() call invokes
__pskb_pull_tail() -> pskb_expand_head(), which allocates a new skb->head
buffer and frees the old buffer still pointed to by iph.

Dereferencing iph->ihl to compute *thoff after the second pskb_may_pull()
then reads from the freed buffer before iph is reloaded from
skb->data + nhoff. This affects all callers of ip4_parse(), including
ip_rcv_core(), virtio_net_l4_start(), br_validate_ipv4(), and
nf_reject_iphdr_validate().

Should ip4_parse() cache iph->ihl * sizeof(u32) in a local variable before
the second pskb_may_pull() call, or reload iph from skb->data + nhoff first?

[Severity: High]
Also in include/linux/virtio_net.h:virtio_net_hdr_tnl_to_skb(), what happens
when virtio_net_csum_partial_ok_at_offset() returns false for a non-gso
packet with a tunnel header?

Earlier in virtio_net_hdr_tnl_to_skb(), __virtio_net_hdr_to_skb() only sets
shinfo->gso_size and shinfo->gso_type when skb->len - nh_off > gso_size. If
a caller provides gso_size >= skb->len - nh_off, skb_is_gso(skb) remains
false, which virtio_net_hdr_tnl_to_skb() explicitly accounts for after
setting the inner header offsets and skb->encapsulation:

	skb->inner_transport_header = inner_th + skb_headroom(skb);
	skb->inner_network_header = inner_nh + skb_headroom(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))
		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;

	/* A tunnel header implies GSO, so drop. */
	return __virtio_net_resolve_csum_partial_mismatch(skb);

However, in include/linux/virtio_net.h:__virtio_net_resolve_csum_partial_mismatch():

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) and virtio_net_csum_partial_ok_at_offset() fails
because the inner headers are malformed or do not match inner_th,
__virtio_net_resolve_csum_partial_mismatch(skb) does not drop the packet.
Instead, it completes the inner checksum via skb_checksum_help(skb) and
returns 1, which callers treat as success.

Does this allow a non-gso tunnel packet with invalid inner headers to enter
the networking stack with skb->encapsulation = 1, unvalidated inner header
offsets, and any requested outer tunnel checksum
(VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) lost when skb_checksum_help() sets
ip_summed to CHECKSUM_NONE? Should virtio_net_hdr_tnl_to_skb() return
-EINVAL directly when virtio_net_csum_partial_ok_at_offset() returns false?

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