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 <[email protected]> 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/[email protected]?part=5
