Re: [PATCH net v5 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: use-after-free in virtio_net_hdr_validate_disabled() due to missing RCU
--

commit 50d6e871df0af42e23190bbc8209691b9cfd76e4
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 network namespaces to opt out of virtio
net header checksum validation on ingress.

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 52852a33a2db7..cea70ccf0ed4b 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -5,6 +5,7 @@
>  #include <linux/if_vlan.h>
>  #include <linux/ip.h>
>  #include <linux/ipv6.h>
> +#include <linux/jump_label.h>
>  #include <linux/netdevice.h>
>  #include <linux/udp.h>
>  #include <net/gso.h>
> @@ -78,6 +79,20 @@ 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.
> + * The caller must keep the netns of skb->dev alive.
> + */
> +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]
Can dereferencing dev_net(skb->dev) here without rcu_read_lock() and
dev_net_rcu() cause a use-after-free on struct net when
virtio_net_hdr_validate_disabled_key is enabled?

Callers such as tun_get_user() in drivers/net/tun.c, as well as
tpacket_snd() and packet_snd() in net/packet/af_packet.c, invoke
virtio_net_hdr_tnl_to_skb() or virtio_net_hdr_to_skb() from preemptible
process context without holding rcu_read_lock() or rtnl_lock():

drivers/net/tun.c:tun_get_user() {
    ...
    err = tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr);
    ...
}

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

In these paths, the caller only holds dev_hold(skb->dev) and a reference
to the socket's netns via sock_net(sk). However, struct net_device does
not hold a reference to its struct net, and skb->dev may reside in a
different network namespace from sock_net(sk) (for instance, when a tun
device or target net_device has been moved to another namespace).

Because __dev_change_net_namespace() in net/core/dev.c switches
dev->nd_net via dev_net_set(dev, net) without waiting for existing
dev_hold(dev) references to drop (synchronizing only via
synchronize_net() and rcu_barrier()), a preempted task in tun_get_user(),
tpacket_snd(), or packet_snd() can load dev_net(skb->dev) just before the
device is moved out and that network namespace is destroyed via
cleanup_net() -> net_complete_free(), and then dereference the freed
struct net upon resuming.

For comparison, __skb_flow_dissect() in net/core/flow_dissector.c (which
is called from __virtio_net_hdr_to_skb() in the same paths) takes
rcu_read_lock() and uses dev_net_rcu(skb->dev) (since commit afec62cd0a41).

Should virtio_net_hdr_validate_disabled() likewise guard the lookup with
rcu_read_lock() / rcu_read_unlock() (or scoped_guard(rcu)) and use
dev_net_rcu(skb->dev)?

[ ... ]

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