Re: [PATCH net v5 2/4] net: devmem: detach TX bindings on NETDEV_UNREGISTER

[sashiko-bot@kernel.org]

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 <almasrymina@google.com>

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/20261030083539.777877-1-almasrymina@google.com?part=2