Re: [PATCHv3 04/15] dmaengine: fsldma: provide device_release callback

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The patch introduces a double-free of `fdev` and multiple use-after-free (UAF) vulnerabilities during device teardown by leaving explicit cleanup operations in `fsldma_of_remove` after the device might have been freed.

Pre-existing issues:
- [High] Individual DMA channel structures (`struct fsldma_chan`) are unconditionally destroyed in `fsldma_of_remove()`, causing use-after-free and invalid hardware access if dmaengine clients still hold references.
- [High] The driver frees the DMA channel structure without ensuring its associated tasklet is killed, which can lead to a use-after-free if the tasklet executes after the channel is freed.
--

commit f8012bef1fb7e396bca2eeb8a77b04e6cdee7e7c
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: fsldma: provide device_release callback

The DMA core requires drivers to set dma_device.device_release so that
the container structure is only freed after all references to it have
been dropped. This patch allocates fdev with kzalloc_obj, provides the
device_release callback, and saves channel pointers to avoid UAF during
removal.

> diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
> index 22d62d958abd9..06a6996a53229 100644
> --- a/drivers/dma/fsldma.c
> +++ b/drivers/dma/fsldma.c
> @@ -1211,6 +1211,8 @@ static void fsl_dma_chan_remove(struct fsldma_chan *chan)
>  	kfree(chan);

[Severity: High]
This isn't a bug introduced by this patch, but does this code free the DMA
channel structure without ensuring its associated tasklet is stopped?

While fsldma_free_irqs() prevents new hardware interrupts, an interrupt
that fired previously could have scheduled the tasklet. If the tasklet is
pending in the softirq queue, it will execute dma_do_tasklet() after chan
has been freed, resulting in a use-after-free.

Would it make sense to call tasklet_kill(&chan->tasklet) before freeing
the channel?

>  }
>  

[ ... ]

> @@ -1308,19 +1311,33 @@ static int fsldma_of_probe(struct platform_device *op)
>  	return err;
>  }
>  
> +static void fsldma_device_release(struct dma_device *dma_dev)
> +{
> +	struct fsldma_device *fdev = container_of(dma_dev, struct fsldma_device,
> +						  common);
> +	kfree(fdev);
> +}
> +
>  static void fsldma_of_remove(struct platform_device *op)
>  {
> -	struct fsldma_device *fdev;
> +	struct fsldma_device *fdev = platform_get_drvdata(op);
> +	struct fsldma_chan *chans[FSL_DMA_MAX_CHANS_PER_DEVICE];
>  	unsigned int i;
>  
> -	fdev = platform_get_drvdata(op);
> -	dma_async_device_unregister(&fdev->common);
> +	for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++)
> +		chans[i] = fdev->chan[i];
>  
>  	fsldma_free_irqs(fdev);
>  
> +	/*
> +	 * fdev may be freed by fsldma_device_release inside this call;
> +	 * use saved copies of the channel pointers afterwards.
> +	 */
> +	dma_async_device_unregister(&fdev->common);
> +
>  	for (i = 0; i < FSL_DMA_MAX_CHANS_PER_DEVICE; i++) {
> -		if (fdev->chan[i])
> -			fsl_dma_chan_remove(fdev->chan[i]);
> +		if (chans[i])
> +			fsl_dma_chan_remove(chans[i]);

[Severity: Critical]
If dma_async_device_unregister() drops the last reference and triggers
fsldma_device_release() to free fdev, does fsl_dma_chan_remove() cause
a use-after-free?

Inside fsl_dma_chan_remove(), list_del(&chan->common.device_node) modifies
the list head linked to fdev->common.channels, which belongs to the newly
freed fdev.

[Severity: High]
This is a pre-existing issue, but does destroying the channel structures
synchronously in fsldma_of_remove() lead to invalid hardware access and
use-after-free if dmaengine clients still hold references?

Since the channel lifetime needs to match the DMA device container, memory
is freed and I/O mapped registers are unmapped here while clients might
still have active references. Could the channel teardown be moved to the
fsldma_device_release() callback instead?

>  	}
>  	irq_dispose_mapping(fdev->irq);

[Severity: Critical]
Does accessing fdev->irq here cause a use-after-free if fdev was already
freed by dma_async_device_unregister() earlier in this function?

The unquoted code immediately following this in fsldma_of_remove() also
proceeds to call iounmap(fdev->regs) and kfree(fdev), which appears to
cause a double-free if the release callback already ran.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609221926.35538-5-rosenp@gmail.com?part=1