Re: [PATCH v5 2/4] dmaengine: Fix device kref underflow in dma_chan_put()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but dropping the last device reference in `dma_chan_put()` triggers device deallocation, leading to a Use-After-Free access in `dma_release_channel()`. While a subsequent patch in the series ('commit "dmaengine: fix use-after-free in dma_chan_put() and dma_release_channel()"') fixes a UAF inside `dma_chan_put()`, it fails to address the subsequent UAF in `dma_release_channel()` itself.
--

--- Patch [2]: [PATCH v5 2/4] dmaengine: Fix device kref underflow in dma_chan_put() ---
commit 3c7cc7c5c7f5082cf18d425005421e76b4f2aada
Author: Shivank Garg <[email protected]>

dmaengine: Fix device kref underflow in dma_chan_put()
    
dma_chan_get() takes chan->device->ref only on the slow path:
    
        /* no kref on fast path */
        if (chan->client_count) {
                __module_get(owner);
                chan->client_count++;
                return 0;
        }
        if (!try_module_get(owner))
                return -ENODEV;
        if (!dma_device_get(chan->device)) { // calls kref_get_unless_zero()

[ ... ]
    
Fixes: 8ad342a86359 ("dmaengine: Add reference counting to dma_device struct")
Reviewed-by: Frank Li <[email protected]>
Reviewed-by: Logan Gunthorpe <[email protected]>
Signed-off-by: Shivank Garg <[email protected]>

> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index cb3b7bccce1f4..f2760b039d932 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -520,7 +520,9 @@ static void dma_chan_put(struct dma_chan *chan)
>  		chan->route_data = NULL;
>  	}
>  
> -	dma_device_put(chan->device);
> +	/* This channel is not in use anymore, drop the device ref */
> +	if (!chan->client_count)
> +		dma_device_put(chan->device);
[Severity: High]
This is a pre-existing issue, but dropping the last device reference in
dma_chan_put() can trigger device deallocation, leading to a use-after-free
access in dma_release_channel().

When a DMA provider device is unregistered while a client still holds a channel,
the provider's dma_async_device_unregister() drops its initial reference.
Later, when the client releases the channel by calling dma_release_channel(),
it calls dma_chan_put() which drops the final reference to the dma_device.
This causes the device and its embedded dma_chan structures to be freed.

Immediately after dma_chan_put() returns, dma_release_channel() dereferences
the freed memory:

drivers/dma/dmaengine.c:dma_release_channel() {
	...
	dma_chan_put(chan);

	if (chan->slave) {
		sysfs_remove_link(&chan->dev->device.kobj, DMA_SLAVE_NAME);
	...
}

While a subsequent patch in this series fixes a use-after-free inside
dma_chan_put(), it appears to leave this subsequent use-after-free in
dma_release_channel() unaddressed.

Could the channel sysfs cleanup and debugfs frees happen before we drop the
final device reference?

>  	module_put(dma_chan_to_owner(chan));
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2