Re: [PATCH v3 2/3] dmaengine: fix use-after-free in dma_chan_put() and dma_release_channel()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Clearing `chan->slave` before stopping the hardware introduces a NULL pointer dereference in `stm32_dma3_pm_suspend()`.
--

--- Patch [2]: [PATCH v3 2/3] dmaengine: fix use-after-free in dma_chan_put() and dma_release_channel() ---
commit c0bca60dc6977cca7352bacd1095081d89c88211
Author: Shivank Garg <[email protected]>

dmaengine: fix use-after-free in dma_chan_put() and dma_release_channel()

When dma_device_put() drops the last reference on chan->device->ref,
dma_device_release() may free the dma_device. This commit caches the
module owner and delays dma_chan_put() to prevent reading freed memory.

Link: https://sashiko.dev/#/patchset/[email protected]

> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
> @@ -916,8 +919,6 @@ void dma_release_channel(struct dma_chan *chan)
>  	if (--chan->device->privatecnt == 0)
>  		dma_cap_clear(DMA_PRIVATE, chan->device->cap_mask);
>  
> -	dma_chan_put(chan);
> -
>  	if (chan->slave) {
>  		sysfs_remove_link(&chan->dev->device.kobj, DMA_SLAVE_NAME);
>  		sysfs_remove_link(&chan->slave->kobj, chan->name);
>  		kfree(chan->name);
>  		chan->name = NULL;
>  		chan->slave = NULL;
>  	}
>  
>  #ifdef CONFIG_DEBUG_FS
>  	kfree(chan->dbg_client_name);
>  	chan->dbg_client_name = NULL;
>  #endif
> +	dma_chan_put(chan);

[Severity: High]
Could moving dma_chan_put() here introduce a NULL pointer dereference
during system suspend?

By moving dma_chan_put() after the block that clears chan->slave, the
hardware channel may still be active while chan->slave is NULL. If a
system suspend occurs concurrently, drivers like stm32-dma3 might check
the hardware state and attempt to print the slave device name:

drivers/dma/stm32/stm32-dma3.c:stm32_dma3_pm_suspend() {
    ...
	ccr = readl_relaxed(ddata->base + STM32_DMA3_CCR(chan->id));
	if (ccr & CCR_EN) {
		dev_warn(dev, "Suspend is prevented: %s still in use by %s\n",
			 dma_chan_name(c), dev_name(c->slave));
    ...
}

Since dma_chan_put() (which stops the hardware) hasn't been called yet,
CCR_EN would still be active, and dev_name(c->slave) would dereference
the NULL pointer we just set. 

Is there a way to stop the channel before clearing the slave pointer
without triggering the original use-after-free?

>  	mutex_unlock(&dma_list_mutex);
>  }

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