Re: [PATCH net v4 5/7] net: add a sysctl to opt out of virtio net hdr checksum validation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: RCU violation reading dev_net() in virtio_net_hdr_validate_disabled()
--

commit 69b1f5f4a234fc132ca5965bd53c81fd86975566
Author: Michael S. Tsirkin <mst@redhat.com>

net: add a sysctl to opt out of virtio net hdr checksum validation

This commit adds a per-netns sysctl, net.core.virtio_net_hdr_validate,
guarded by a static key to allow opting out of virtio_net_hdr checksum
offset validation.

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index ec6f47ce1c974..b3bbecbd023cb 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
[ ... ]
> @@ -78,6 +79,19 @@ static inline struct iphdr *virtio_net_ip4_parse(struct sk_buff *skb,
>  	*thoff = nhoff + iph->ihl * sizeof(u32);
>  	return iph;
>  }
> +/* Defined in net/core/dev.c, enabled while any network namespace
> + * has net.core.virtio_net_hdr_validate set to 0.
> + */
> +DECLARE_STATIC_KEY_FALSE(virtio_net_hdr_validate_disabled_key);
> +
> +/* Failsafe: a namespace can disable the check with
> + * net.core.virtio_net_hdr_validate.
> + */
> +static inline bool virtio_net_hdr_validate_disabled(struct sk_buff *skb)
> +{
> +	return static_branch_unlikely(&virtio_net_hdr_validate_disabled_key) &&
> +	       !READ_ONCE(dev_net(skb->dev)->core.sysctl_virtio_net_hdr_validate);
> +}

[Severity: High]
Does virtio_net_hdr_validate_disabled() need RCU read-side protection when
fetching dev_net(skb->dev) and reading core.sysctl_virtio_net_hdr_validate?

When net.core.virtio_net_hdr_validate is set to 0 in any network namespace
(enabling virtio_net_hdr_validate_disabled_key), several callers of
virtio_net_hdr_to_skb() and virtio_net_hdr_tnl_to_skb() invoke
virtio_net_hdr_validate_disabled(skb) from preemptible process context while
holding only dev_hold(skb->dev) and without holding rcu_read_lock().

Specifically, tun_get_user() calls tun_vnet_hdr_tnl_to_skb() ->
virtio_net_hdr_tnl_to_skb() before acquiring rcu_read_lock():

drivers/net/tun.c:tun_get_user() {
	...
	err = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr);
	if (err < 0) {
		atomic_long_inc(&tun->rx_frame_errors);
		err = -EINVAL;
		goto free_skb;
	}
	...
	rcu_read_lock();
	if (unlikely(!(tun->dev->flags & IFF_UP))) {
	...
}

Similarly, tpacket_snd() and packet_snd() in net/packet/af_packet.c call
virtio_net_hdr_to_skb() in process context without rcu_read_lock():

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

net/packet/af_packet.c:packet_snd() {
	...
	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;
	}
	...
}

Holding dev_hold(skb->dev) does not prevent __dev_change_net_namespace() from
moving skb->dev to another network namespace via dev_net_set(dev, net). If the
old network namespace then exits and is freed via cleanup_net(), a task
preempted after loading skb->dev->nd_net.net outside rcu_read_lock() could
dereference a freed struct net.

Should virtio_net_hdr_validate_disabled() use dev_net_rcu(skb->dev) inside
rcu_read_lock() and rcu_read_unlock(), similar to how commit afec62cd0a4191
("flow_dissector: use RCU protection to fetch dev_net()") fixed the same
pattern in __skb_flow_dissect()?

[ ... ]

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