Re: [PATCH v2] dmaengine: mediatek: initialize tasklets before requesting IRQs

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The DMA device, OF controller, and IRQ handlers are registered and exposed to the system before the hardware clocks and power domains are enabled, risking a fatal system crash.
- [High] Incorrect teardown ordering in `mtk_cqdma_remove` allows virtual channel tasklets to be rescheduled after they are killed, leading to use-after-free, and permits hardware access during de-initialization.
- [High] `mtk_cqdma_remove` manually removes channels from the device list, completely bypassing channel unregistration and leaking memory and ID allocations.
- [High] Lockless access of descriptor state in `mtk_cqdma_tx_status` leads to a TOCTOU use-after-free.
- [High] If `mtk_cqdma_prep_dma_memcpy` is called with a transfer length of 0, memory allocation returns `ZERO_SIZE_PTR`, bypassing the NULL check and causing a kernel Oops.
--

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

dmaengine: mediatek: initialize tasklets before requesting IRQs

The CQDMA probe registers each physical-channel IRQ before initializing
the tasklet scheduled by the interrupt handler. A pending interrupt can
access an uninitialized physical channel or schedule an uninitialized
tasklet while probe is still running.

Link: https://lore.kernel.org/all/20260830060422.2204459-1-runyu.xiao@seu.edu.cn/

> diff --git a/drivers/dma/mediatek/mtk-cqdma.c b/drivers/dma/mediatek/mtk-cqdma.c
> index 80791e30aec24..741b6325cdee4 100644
> --- a/drivers/dma/mediatek/mtk-cqdma.c
> +++ b/drivers/dma/mediatek/mtk-cqdma.c

[ ... ]

> @@ -805,41 +805,54 @@ static int mtk_cqdma_probe(struct platform_device *pdev)
>  		err = platform_get_irq(pdev, i);
>  		if (err < 0)
> -			return err;
> +			goto err_tasklets;
>  		cqdma->pc[i]->irq = err;
> +	}
>  
> +	/* Register IRQs only after all PCs are initialized. */
> +	for (i = 0; i < cqdma->dma_channels; ++i) {
>  		err = devm_request_irq(&pdev->dev, cqdma->pc[i]->irq,
>  				       mtk_cqdma_irq, 0, dev_name(&pdev->dev),
>  				       cqdma);

[Severity: High]
This is a pre-existing issue, but does requesting the IRQ here unmask interrupts
at the controller, allowing the ISR to execute unclocked register reads before
the hardware clocks are enabled?

>  		if (err) {
>  			dev_err(&pdev->dev,
>  				"request_irq failed with err %d\n", err);

[ ... ]

> @@ -850,7 +863,7 @@ static int mtk_cqdma_probe(struct platform_device *pdev)
>  
>  	err = dma_async_device_register(dd);
>  	if (err)
> -		return err;
> +		goto err_tasklets;

[Severity: High]
This is a pre-existing issue, but is the DMA device exposed to clients before
its hardware is initialized?

>  
>  	err = of_dma_controller_register(pdev->dev.of_node,
>  					 of_dma_xlate_by_chan_id, cqdma);
> @@ -864,22 +877,36 @@ static int mtk_cqdma_probe(struct platform_device *pdev)
>  	err = mtk_cqdma_hw_init(cqdma);
>  	if (err) {

[Severity: High]
This is a pre-existing issue, but hardware clocks and PM runtime are only
initialized here, which appears to be too late in the probe sequence. Could an
immediately pending interrupt or an immediate DMA request access the unclocked
hardware, leading to a fatal system crash?

[ ... ]

This is a pre-existing issue, but could the incorrect teardown ordering in
mtk_cqdma_remove() lead to use-after-free and hardware access during
de-initialization?

drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_remove() {
...
	/* kill VC task */
	for (i = 0; i < cqdma->dma_requests; i++) {
		vc = &cqdma->vc[i];

		list_del(&vc->vc.chan.device_node);
		tasklet_kill(&vc->vc.task);
	}
...
	for (i = 0; i < cqdma->dma_channels; i++) {
		spin_lock_irqsave(&cqdma->pc[i]->lock, flags);
		mtk_dma_clr(cqdma->pc[i], MTK_CQDMA_INT_EN,
			    MTK_CQDMA_INT_EN_BIT);
		spin_unlock_irqrestore(&cqdma->pc[i]->lock, flags);

		/* Waits for any pending IRQ handlers to complete */
		synchronize_irq(cqdma->pc[i]->irq);

		tasklet_kill(&cqdma->pc[i]->tasklet);
	}

	/* disable hardware */
	mtk_cqdma_hw_deinit(cqdma);

	dma_async_device_unregister(&cqdma->ddev);
...
}

[Severity: High]
VC tasklets are killed prematurely before PC producers and IRQs are fully
stopped. Furthermore, the teardown loop fails to call disable_irq(), leaving the
global IRQ handler vulnerable to cross-channel polling execution. Lastly, is it
safe that the DMA device is unregistered after hardware deinitialization, which
might allow late client requests to interact with unclocked hardware?


This is a pre-existing issue, but does mtk_cqdma_remove() manually remove
channels from the device list, completely bypassing channel unregistration
and leaking memory and ID allocations?

drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_remove() {
...
	for (i = 0; i < cqdma->dma_requests; i++) {
		vc = &cqdma->vc[i];

		list_del(&vc->vc.chan.device_node);
		tasklet_kill(&vc->vc.task);
	}
...
}

[Severity: High]
Does calling list_del(&vc->vc.chan.device_node) hide the channels from the DMA
core unregistration, leading to a leak of memory and ID allocations?


This is a pre-existing issue, but does lockless access of descriptor state in
mtk_cqdma_tx_status() lead to a TOCTOU use-after-free?

drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_tx_status() {
...
	spin_lock_irqsave(&cvc->pc->lock, flags);
	spin_lock(&cvc->vc.lock);
	vd = mtk_cqdma_find_active_desc(c, cookie);
	spin_unlock(&cvc->vc.lock);
	spin_unlock_irqrestore(&cvc->pc->lock, flags);

	if (vd) {
		cvd = to_cqdma_vdesc(vd);
		bytes = cvd->residue;
	}
...
}

[Severity: High]
Since the locks protecting the descriptor's lifecycle are dropped right before
its fields are accessed, could concurrent tasklets complete the transaction and
free the descriptor, resulting in a use-after-free?


This is a pre-existing issue, but if mtk_cqdma_prep_dma_memcpy() is called
with a transfer length of 0, could memory allocation return ZERO_SIZE_PTR and
bypass the NULL check?

drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_prep_dma_memcpy() {
...
	nr_vd = DIV_ROUND_UP(len, MTK_CQDMA_MAX_LEN);
	cvd = kzalloc_objs(*cvd, nr_vd, GFP_NOWAIT);
	if (!cvd)
		return NULL;

	for (i = 0; i < nr_vd; ++i) {
...
	return &cvd[0]->vd.tx;
}

[Severity: High]
When len is 0, nr_vd becomes 0 and kzalloc_objs() returns ZERO_SIZE_PTR.
Does the NULL check pass in this scenario, skipping the loop and causing a
kernel oops when &cvd[0]->vd.tx is returned and dereferenced?

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