Re: [PATCH net v2 1/4] net: devmem: unmap dma_buf synchronously on queue uninstall

[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 dma_unmap_sg_attrs() for devmem leased RX bindings
--

commit 0978772617865658c49d5d8aff7d5a2c206deaf4
Author: Mina Almasry <almasrymina@google.com>
net: devmem: unmap dma_buf synchronously on queue uninstall

This commit changes the unmapping of the dma_buf attachment to happen
synchronously when the queue is uninstalled. It introduces
net_devmem_dmabuf_binding_unmap() to clear the attachment, preventing a
use-after-free if the physical device is unregistered while references
are still held.

> diff --git a/net/core/devmem.c b/net/core/devmem.c
> index f4d60654ce7fd..02d607cc87217 100644
> --- a/net/core/devmem.c
> +++ b/net/core/devmem.c

[ ... ]

> @@ -153,6 +176,35 @@ void net_devmem_unbind_dmabuf(struct net_devmem_dmabuf_binding *binding)
>  	percpu_ref_kill(&binding->ref);
>  }
>  
> +static void
> +net_devmem_dev_unregister_rx(struct net_devmem_dmabuf_binding *binding,
> +			     unsigned long xa_idx, struct net_device *dev)
> +{
> +	if (xa_get_mark(&net_devmem_dmabuf_bindings, xa_idx,
> +			NET_DEVMEM_BINDING_LIVE) ||
> +	    READ_ONCE(binding->dev) != dev)
> +		return;

[Severity: High]
For leased RX queues, if the physical device unregisters, could binding->dev
still point to the virtual device instead of the physical device?

If the physical device unregisters without prior unleasing, or if the queue
was previously uninstalled (setting binding->dev to NULL), this check would
evaluate to true and skip the unmapping. This would leave the dma_buf
mapped to the physical device's dma_dev, which is subsequently freed.

When the binding is ultimately destroyed via
__net_devmem_dmabuf_binding_free(), could this trigger a use-after-free in
dma_buf_unmap_attachment_unlocked() when it accesses the freed dma_dev?

> +
> +	mutex_lock(&binding->lock);
> +	WRITE_ONCE(binding->dev, NULL);
> +	if (!netif_is_queue_leasee(dev))
> +		net_devmem_dmabuf_binding_unmap(binding);
> +	mutex_unlock(&binding->lock);
> +}

[ ... ]

> @@ -529,16 +585,19 @@ static void mp_dmabuf_devmem_uninstall(void *mp_priv,
>  	unsigned long xa_idx;
>  
>  	xa_for_each(&binding->bound_rxqs, xa_idx, bound_rxq) {
> -		if (bound_rxq == rxq) {
> -			xa_erase(&binding->bound_rxqs, xa_idx);
> -			if (xa_empty(&binding->bound_rxqs)) {
> -				mutex_lock(&binding->lock);
> -				ASSERT_EXCLUSIVE_WRITER(binding->dev);
> -				WRITE_ONCE(binding->dev, NULL);
> -				mutex_unlock(&binding->lock);
> -			}
> -			break;
> +		if (bound_rxq != rxq)
> +			continue;
> +
> +		xa_erase(&binding->bound_rxqs, xa_idx);
> +		if (xa_empty(&binding->bound_rxqs)) {
> +			mutex_lock(&binding->lock);
> +			ASSERT_EXCLUSIVE_WRITER(binding->dev);
> +			WRITE_ONCE(binding->dev, NULL);
> +			if (!rxq->lease)
> +				net_devmem_dmabuf_binding_unmap(binding);
> +			mutex_unlock(&binding->lock);

[Severity: High]
Here binding->dev is unconditionally set to NULL, but for leased queues,
net_devmem_dmabuf_binding_unmap() is explicitly skipped.

If the physical device is later unregistered, won't
net_devmem_dev_unregister_rx() fail to unmap the binding because
binding->dev was already set to NULL?

Could this result in the dma_buf remaining mapped to the physical device
until the binding is eventually freed, causing a use-after-free at that
time?

>  		}
> +		break;
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012174300.2338784-1-almasrymina@google.com?part=1