Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] net: devmem: UAF in dmabuf unmap due to skipped unmap on physical device unregister --
commit 9aa963a98903abc88d8d047a795dac2c2807eebc Author: Mina Almasry <[email protected]> net: devmem: unmap dma_buf synchronously on queue uninstall This commit adds synchronous unmapping of a dma_buf when the last bound RX queue is uninstalled or the bound physical device is unregistered. The intention is to prevent a use-after-free crash in dma_unmap_sg_attrs() that happens when the device is removed (freeing dma_dev) before the binding is completely freed on socket close. > diff --git a/net/core/devmem.c b/net/core/devmem.c > index f4d60654ce7fd..93cc2815d935a 100644 > --- a/net/core/devmem.c > +++ b/net/core/devmem.c [ ... ] > @@ -153,6 +176,38 @@ 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] When a virtual device leases an RX queue from a physical device, netdev_nl_bind_rx_doit() maps a dmabuf using the physical device's dma_dev but sets binding->dev to the virtual device. If the physical device is unregistered, will this condition cause the unmap to be skipped because binding->dev does not match the physical device? > + > + mutex_lock(&binding->lock); > + WRITE_ONCE(binding->dev, NULL); > + if (!netif_is_queue_leasee(dev)) > + net_devmem_dmabuf_binding_unmap(binding); [Severity: High] If the virtual device unregisters (either directly or due to the physical device unregistering), this explicit check skips the unmapping step for leased queues. Does this leave the dmabuf mapped to the physical device's soon-to-be-freed dma_dev, and ultimately lead to the same use-after-free crash during socket close? > + mutex_unlock(&binding->lock); > +} [ ... ] > @@ -529,16 +588,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); [Severity: High] Similar to the issue above, does explicitly skipping the unmap here for leased queues prevent synchronous unmapping and trigger a use-after-free in __net_devmem_dmabuf_binding_free() when it uses the freed dma_dev? > + mutex_unlock(&binding->lock); > } > + break; > } > } > -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
