Re: [PATCH net-next v2 4/7] net: resolve uncommon partial checksums at ingress

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: OOB read in virtio_net_l3_start due to uninitialized mac_header
--

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

net: resolve uncommon partial checksums at ingress

This commit updates virtio net header parsing to validate checksum offsets
and only retain CHECKSUM_PARTIAL when they match parsed TCP or UDP headers.
It also introduces early tunnel offset validation.

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index d6466f96cdd00..357115ff0ec2d 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h

[ ... ]

> @@ -332,10 +550,13 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  	    inner_th < inner_nh + inner_l3min)
>  		return -EINVAL;
>  
> +	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
> +		return -EINVAL;
> +

[Severity: High]
Does this sequence result in accessing an uninitialized MAC header?

Tracing the call sequence starting here:
virtio_net_hdr_tnl_to_skb()
  -> virtio_net_tnl_offsets_ok()
    -> virtio_net_l3_start()

Inside virtio_net_l3_start(), if the device type is ARPHRD_ETHER (which is
true for IFF_TAP and virtio_net), the code evaluates eth_hdr(skb)->h_proto:

include/linux/virtio_net.h:virtio_net_l3_start() {
	if (skb->dev->type == ARPHRD_ETHER) {
		if (!pskb_may_pull(skb, ETH_HLEN))
			return false;
		*nhoff = ETH_HLEN;
		*proto = eth_hdr(skb)->h_proto;
...
}

However, eth_hdr(skb) strictly evaluates to skb->head + skb->mac_header.
Callers like tun_get_user() and virtnet_receive_done() do not appear to
initialize the MAC header before calling virtio_net_hdr_tnl_to_skb().

The MAC header is normally initialized via skb_reset_mac_header(skb), which
happens in __virtio_net_hdr_to_skb() located just below this check.

Because virtio_net_tnl_offsets_ok() is called before __virtio_net_hdr_to_skb(),
skb->mac_header retains its default allocation value of 0xFFFF. Could this
cause an out-of-bounds memory read (skb->head + 0xFFFF + offsetof(h_proto))
and a potential kernel panic?

>  	/* 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;

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