Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] net: devmem: UAF when netdev_nl_bind_tx_doit() races with phys_dev unregister --
commit ccf551ffb7f8200a4b1a6abbb90f90e5db975e35 Author: Mina Almasry <[email protected]> net: devmem: detach TX bindings on NETDEV_UNREGISTER This commit extends net_devmem_dev_unregister() and __net_devmem_dmabuf_binding_free() to handle TX devmem bindings when a physical or virtual network device unregisters. When binding->dev unregisters, the binding is unmarked live, its dma_buf is unmapped, and both binding->vdev and binding->dev are cleared; when only binding->vdev unregisters, only binding->vdev is cleared. > diff --git a/net/core/devmem.c b/net/core/devmem.c > index 4610de805ef08..5c7d9640e9d14 100644 > --- a/net/core/devmem.c > +++ b/net/core/devmem.c > @@ -149,7 +149,7 @@ void __net_devmem_dmabuf_binding_free(struct work_struct > *wq) > { > struct net_devmem_dmabuf_binding *binding = container_of(wq, > typeof(*binding), unbind_w); > netdevice_tracker dev_tracker; > - struct net_device *dev = NULL; > + struct net_device *dev; > > if (binding->freelist) > WARN(binding->free_count != binding->area.num_niovs, > @@ -157,8 +157,7 @@ void __net_devmem_dmabuf_binding_free(struct work_struct > *wq) > binding->area.num_niovs, binding->free_count); > > kvfree(binding->area.niovs); > - if (binding->direction == DMA_FROM_DEVICE) > - dev = net_devmem_lock_dev(binding, &dev_tracker); > + dev = net_devmem_lock_dev(binding, &dev_tracker); > net_devmem_dmabuf_binding_unmap(binding); > xa_erase(&net_devmem_dmabuf_bindings, binding->id); > net_devmem_unlock_dev(dev, &dev_tracker); > @@ -252,11 +251,20 @@ void net_devmem_dev_unregister(struct net_device *dev) > > xa_lock(&net_devmem_dmabuf_bindings); > xa_for_each(&net_devmem_dmabuf_bindings, xa_idx, binding) { > - if (binding->direction != DMA_FROM_DEVICE || > - READ_ONCE(binding->dev) != dev || > - xa_get_mark(&net_devmem_dmabuf_bindings, xa_idx, > - NET_DEVMEM_BINDING_LIVE)) > - continue; > + if (binding->direction == DMA_FROM_DEVICE) { > + if (READ_ONCE(binding->dev) != dev || > + xa_get_mark(&net_devmem_dmabuf_bindings, xa_idx, > + NET_DEVMEM_BINDING_LIVE)) > + continue; > + } else { > + if (READ_ONCE(binding->vdev) == dev) > + WRITE_ONCE(binding->vdev, NULL); > + if (READ_ONCE(binding->dev) != dev) > + continue; [Severity: High] Can binding->dev be left pointing to a freed physical net_device if netdev_nl_bind_tx_doit() on a virtual device races with physical device unregistration? When a NETMEM_TX_NO_DMA virtual device (vdev, such as netkit) leases an RX queue from a physical device (phys_dev), phys_dev is pinned via rxq_src->lease_tracker until vdev is unregistered. When phys_dev unregisters in unregister_netdevice_many_notify(), its reg_state is set to NETREG_UNREGISTERING under netdev_lock(phys_dev) while __LINK_STATE_PRESENT remains set. During call_netdevice_notifiers() for NETDEV_UNREGISTER on phys_dev, netdev_genl_netdevice_event() runs first, calling net_devmem_dev_unregister(phys_dev) under netdev_lock(phys_dev). Only after that notifier completes does netkit_notifier() invoke netkit_check_lease_unregister(phys_dev) -> unregister_netdevice_many() to acquire netdev_lock(vdev) and set vdev->reg_state to NETREG_UNREGISTERING. Because netdev_nl_family has parallel_ops enabled and NETDEV_CMD_BIND_TX does not hold rtnl_lock(), a concurrent netdev_nl_bind_tx_doit() on vdev can grab netdev_lock(vdev) via netdev_get_by_index_lock() while vdev->reg_state is still NETREG_REGISTERED, look up phys_dev via netdev_find_netmem_tx_dev(), and then acquire netdev_lock(bind_dev) after net_devmem_dev_unregister() has already finished on phys_dev: net/core/netdev-genl.c:netdev_find_netmem_tx_dev() { ... for (i = 0; i < dev->real_num_rx_queues; i++) { lease_rxq = READ_ONCE(__netif_get_rx_queue(dev, i)->lease); if (!lease_rxq) continue; phys_dev = lease_rxq->dev; if (netif_device_present(phys_dev) && phys_dev->netmem_tx == NETMEM_TX_DMA) return phys_dev; } } net/core/netdev-genl.c:netdev_nl_bind_tx_doit() { ... if (bind_dev != netdev) netdev_lock(bind_dev); dma_dev = netdev_queue_get_dma_dev(bind_dev, 0, NETDEV_QUEUE_TYPE_TX); binding = net_devmem_bind_dmabuf(bind_dev, bind_dev != netdev ? netdev : NULL, dma_dev, DMA_TO_DEVICE, dmabuf_fd, PAGE_SHIFT, priv, NETLINK_CB(skb).sk, info->extack); } Since netdev_nl_bind_tx_doit() does not check that bind_dev->reg_state == NETREG_REGISTERED after taking netdev_lock(bind_dev), it creates the binding with binding->dev = phys_dev and binding->vdev = vdev. When vdev then runs net_devmem_dev_unregister(vdev), this branch sees binding->vdev == vdev and binding->dev == phys_dev != vdev, so it only clears binding->vdev to NULL and continues without unmapping the dma_buf or clearing binding->dev. Because net_devmem_dev_unregister(phys_dev) already ran, binding->dev is never cleared. Once netkit_uninit() drops the lease reference and netdev_run_todo() frees phys_dev, won't netdev_nl_sock_priv_destroy() or __net_devmem_dmabuf_binding_free() dereference the freed phys_dev in net_devmem_lock_dev()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
