Re: [PATCH net v3 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: devmem: UAF in dmabuf unmap due to skipped unmap on physical device unregister
--

commit 9aa963a98903abc88d8d047a795dac2c2807eebc
Author: Mina Almasry <almasrymina@google.com>

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