Re: [PATCH v2 13/64] dmaengine: mxs-dma: use dma_chan BH scheduling

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

Pre-existing issues:
- [High] Missing synchronization/flushing of bottom-half work during channel teardown leads to a use-after-free of client resources.
- [High] Requesting IRQ before clock enable and descriptor initialization causes a kernel panic on pending interrupt.
- [High] The driver allocates the DMA engine state with devm_kzalloc but lacks a device_release callback, risking UAF on unbind.
--

--- Patch [13]: [PATCH v2 13/64] dmaengine: mxs-dma: use dma_chan BH scheduling ---
commit cc33b3c421743dc24481c221e9955791a7689087
Author: Allen Pais <[email protected]>

dmaengine: mxs-dma: use dma_chan BH scheduling

Replace the per-channel tasklet with the shared dma_chan BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

> The handler continues to run in softirq context while dmaengine owns
> the common scheduling and teardown mechanism.

[Severity: High]
This is a pre-existing issue, but since the commit message notes that the
teardown mechanism is being updated, does mxs_dma_free_chan_resources()
need to call dma_chan_kill_bh() to prevent use-after-free conditions?

If a client releases the channel concurrently with an interrupt,
mxs_dma_free_chan_resources() currently returns without flushing pending
work:

drivers/dma/mxs-dma.c:mxs_dma_free_chan_resources() {
	...
	mxs_dma_disable_chan(chan);

	free_irq(mxs_chan->chan_irq, mxs_dma);
}

Could this allow the asynchronous work to outlive the channel allocation
and dereference a freed client callback when mxs_dma_tasklet() runs?

> diff --git a/drivers/dma/mxs-dma.c b/drivers/dma/mxs-dma.c
> index 7acb3d29dad30..8cecc0add82dc 100644
> --- a/drivers/dma/mxs-dma.c
> +++ b/drivers/dma/mxs-dma.c

[ ... ]

> @@ -385,8 +385,8 @@ static irqreturn_t mxs_dma_int_handler(int irq, void *dev_id)
>  		dma_cookie_complete(&mxs_chan->desc);
>  	}
>  
> -	/* schedule tasklet on this channel */
> -	tasklet_schedule(&mxs_chan->tasklet);
> +	/* schedule BH on this channel */
> +	dma_chan_schedule_bh(&mxs_chan->chan);
>  
>  	return IRQ_HANDLED;
>  }

[Severity: High]
This is a pre-existing issue, but can this handler be triggered before the
hardware clock is enabled and the descriptor is initialized?

Looking at mxs_dma_alloc_chan_resources(), the IRQ is requested before the
clock is enabled:

drivers/dma/mxs-dma.c:mxs_dma_alloc_chan_resources() {
	...
	ret = request_irq(mxs_chan->chan_irq, mxs_dma_int_handler,
			  0, "mxs-dma", mxs_dma);
	if (ret)
		goto err_irq;

	ret = clk_prepare_enable(mxs_dma->clk);
	...
}

If a spurious or bootloader-pending interrupt fires immediately, could
mxs_dma_int_handler() process a DMA_COMPLETE status and hit a BUG_ON when
it calls dma_cookie_complete(&mxs_chan->desc) because the cookie is
still 0?

[ ... ]

> @@ -780,7 +780,7 @@ static int mxs_dma_probe(struct platform_device *pdev)
>  		mxs_chan->chan.device = &mxs_dma->dma_device;
>  		dma_cookie_init(&mxs_chan->chan);
>  
> -		tasklet_setup(&mxs_chan->tasklet, mxs_dma_tasklet);
> +		dma_chan_init_bh(&mxs_chan->chan, mxs_dma_tasklet);
>  
>  
>  		/* Add the channel to mxs_chan list */

[Severity: High]
This is a pre-existing issue, but is the driver state vulnerable to a
use-after-free if the device is unbound?

The driver allocates mxs_dma using devm_kzalloc() but appears to register
the async device without providing a device_release callback:

drivers/dma/mxs-dma.c:mxs_dma_probe() {
	...
	mxs_dma = devm_kzalloc(dev, sizeof(*mxs_dma), GFP_KERNEL);
	if (!mxs_dma)
		return -ENOMEM;
	...
	ret = dmaenginem_async_device_register(&mxs_dma->dma_device);
	...
}

If the driver is unbound via sysfs while a client still holds a reference
to a DMA channel, will devres immediately free mxs_dma, causing a panic
when the client later accesses the channel?

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