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