Re: [PATCH] dmaengine: tegra: initialize channel state before requesting IRQ

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Uninitialized `tdc->isr_handler` causes a NULL pointer dereference on early interrupt.
- [High] Uninitialized `tdc->dma_chan.dev` pointer dereference in the ISR error logging path and tracepoints.
- [High] Missing `tasklet_kill()` on error and remove paths leads to a Use-After-Free if an interrupt schedules the tasklet.
- [High] Hardware registers are accessed from the ISR while the device clock is disabled, leading to a bus hang.
--

commit bf493fa22f6f1d380e6e364bca736ddeb8ce99f4
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

dmaengine: tegra: initialize channel state before requesting IRQ
    
tegra_dma_probe() registers a channel IRQ before initializing the
channel lock, tasklet, waitqueue, and descriptor lists used by the
interrupt path. A pending interrupt can therefore access uninitialized
channel state. Prepare the channel state before requesting its IRQ so
the handler and tasklet always see initialized synchronization and list
objects.

> diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c
> index 640b8a218c9ad..23fd294ab00d5 100644
> --- a/drivers/dma/tegra20-apb-dma.c
> +++ b/drivers/dma/tegra20-apb-dma.c
> @@ -1494,6 +1494,19 @@ static int tegra_dma_probe(struct platform_device *pdev)
>  		}
>  
>  		snprintf(tdc->name, sizeof(tdc->name), "apbdma.%d", i);
> +		tdc->tdma = tdma;
> +		tdc->id = i;
> +		tdc->slave_id = TEGRA_APBDMA_SLAVE_ID_INVALID;
> +
> +		tasklet_setup(&tdc->tasklet, tegra_dma_tasklet);
> +		spin_lock_init(&tdc->lock);
> +		init_waitqueue_head(&tdc->wq);
> +
> +		INIT_LIST_HEAD(&tdc->pending_sg_req);
> +		INIT_LIST_HEAD(&tdc->free_sg_req);
> +		INIT_LIST_HEAD(&tdc->free_dma_desc);
> +		INIT_LIST_HEAD(&tdc->cb_desc);
> +
>  		ret = devm_request_irq(&pdev->dev, irq, tegra_dma_isr, 0,
>  				       tdc->name, tdc);

[Severity: High]
This isn't a bug introduced by this patch, but moving the initialization here
fails to initialize tdc->isr_handler.

If a spurious interrupt fires before tegra_dma_prep_slave_sg() initializes the
handler, and tdc_read() returns a value with TEGRA_APBDMA_STATUS_ISE_EOC set
(such as 0xFFFFFFFF resulting from an abort on an unclocked APB read), will
tegra_dma_isr() unconditionally execute the NULL tdc->isr_handler pointer?

tegra_dma_isr() {
	...
	if (status & TEGRA_APBDMA_STATUS_ISE_EOC) {
		...
		tdc->isr_handler(tdc, false);
	...
}

[Severity: High]
This isn't a bug introduced by this patch, but is tdc->dma_chan.dev
uninitialized at this point?

tdc->dma_chan.dev is dynamically allocated later in
dma_async_device_register(). If a spurious interrupt fires and the interrupt
status does not have TEGRA_APBDMA_STATUS_ISE_EOC set, the ISR falls back to
the error logging path which uses tdc2dev():

tegra_dma_isr() {
	...
	dev_info(tdc2dev(tdc), "Interrupt already served status 0x%08x\n", status);
	...
}

Will tdc2dev() unconditionally dereference the NULL dma_chan.dev pointer?

[Severity: High]
This is a pre-existing issue, but does requesting the IRQ here lead to a
use-after-free if probe fails?

If a spurious interrupt schedules tdc->tasklet and a subsequent probe step like
dma_async_device_register() fails, the error path returns without calling
tasklet_kill():

tegra_dma_probe() {
	...
err_pm_disable:
	pm_runtime_disable(&pdev->dev);
	return ret;
}

Devres will then free the tdma and tdc memory. When the scheduled softirq
runs, will it access the freed channel state? The driver unbind path in
tegra_dma_remove() appears to have the same issue.

[Severity: High]
This is a pre-existing issue, but does requesting the IRQ here allow hardware
registers to be accessed from the ISR while the device clock is disabled?

tegra_dma_init_hw() deliberately disables the DMA clock prior to this loop:

tegra_dma_init_hw() {
	...
	clk_disable(tdma->dma_clk);
}

When devm_request_irq() unmasks the interrupt, an early or spurious interrupt
will invoke tegra_dma_isr(), which performs an APB register read:

tegra_dma_isr() {
	...
	status = tdc_read(tdc, TEGRA_APBDMA_CHAN_STATUS);
	...
}

Since reading from an unclocked APB register typically hangs the bus on this
hardware, can this lead to a system lockup?

>  		if (ret) {
>  			dev_err(&pdev->dev,
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830061213.2209331-1-runyu.xiao@seu.edu.cn?part=1