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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missing `tasklet_kill()` on the `mtk_cqdma_probe()` error path results in a Use-After-Free if a tasklet is scheduled before the failure occurs.

Pre-existing issues:
- [High] The interrupt handler `mtk_cqdma_irq()` can crash with a NULL pointer dereference if an interrupt triggers before all physical channels are allocated during probe.
- [High] Requesting IRQs and registering the DMA device before enabling power domains and clocks causes asynchronous or synchronous external aborts.
- [High] Missing `of_dma_controller_free()` on the probe error path leaves a dangling pointer in the OF DMA list.
- [High] Incorrect teardown order in `mtk_cqdma_remove()` allows tasklets to be re-scheduled after being killed.
- [High] Error path in `mtk_cqdma_prep_dma_memcpy()` frees descriptors that are already linked into the virtual channel's list, causing list corruption and use-after-free.
- [High] A zero-length DMA transfer request causes an out-of-bounds access and crash in `mtk_cqdma_prep_dma_memcpy()`.
- [High] When issuing multiple pending descriptors, the CPU is forced to synchronously wait for hardware completion for all descriptors, stalling the system.
--

commit 0bc1ea9cc66d3b94f9b7b69d7f95a506a7e4bd12
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
schedule an uninitialized tasklet while probe is still running.

> diff --git a/drivers/dma/mediatek/mtk-cqdma.c b/drivers/dma/mediatek/mtk-cqdma.c
> index 80791e30aec24..8038f14d1afb4 100644
> --- a/drivers/dma/mediatek/mtk-cqdma.c
> +++ b/drivers/dma/mediatek/mtk-cqdma.c
> @@ -815,6 +815,7 @@ static int mtk_cqdma_probe(struct platform_device *pdev)
>  		INIT_LIST_HEAD(&cqdma->pc[i]->queue);
>  		spin_lock_init(&cqdma->pc[i]->lock);
>  		refcount_set(&cqdma->pc[i]->refcnt, 0);
> +		tasklet_setup(&cqdma->pc[i]->tasklet, mtk_cqdma_tasklet_cb);

[Severity: High]
Does this patch introduce a use-after-free risk on the probe error path?

By moving tasklet_setup() before devm_request_irq(), the tasklet becomes
active early. If a subsequent probe step (like mtk_cqdma_hw_init) fails,
the error path returns without killing the scheduled tasklets:

drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_probe() {
    ...
err_unregister:
    dma_async_device_unregister(dd);
    
    return err;
}

If an early interrupt has already scheduled the tasklet, and devres frees
the channel memory upon probe failure, will the tasklet subsequently execute
on freed memory?


[Severity: High]
This is a pre-existing issue, but can the interrupt handler crash with a
NULL pointer dereference if an interrupt triggers before all physical channels
are allocated during probe?

During mtk_cqdma_probe(), channels are allocated and their IRQs requested
sequentially in a loop. If an IRQ fires for channel 0 before channel 1 is
allocated, mtk_cqdma_irq() iterates over all channels:

drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_irq() {
    ...
    for (i = 0; i < cqdma->dma_channels; ++i, schedule_tasklet = false) {
        spin_lock(&cqdma->pc[i]->lock);
    ...
}

Would this dereference the unallocated cqdma->pc[1] and cause a crash via
spin_lock()?


[Severity: High]
This isn't a bug introduced by this patch, but does requesting IRQs and
registering the DMA device before enabling power domains and clocks risk an
external abort?

In mtk_cqdma_probe(), devm_request_irq() and dma_async_device_register()
are called before mtk_cqdma_hw_init() enables power domains and clocks. 

If a spurious interrupt or an immediate client transaction occurs, it will
attempt to read or write unclocked hardware registers:

drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_irq() {
    ...
    if (mtk_dma_read(cqdma->pc[i], MTK_CQDMA_INT_FLAG) ...
    ...
}

Could this lead to a synchronous external abort and a kernel panic?


[Severity: High]
This is a pre-existing issue, but does the probe error path leave a dangling
pointer in the OF DMA list?

In mtk_cqdma_probe(), the device structure pointer is added to the global
OF DMA list:

    err = of_dma_controller_register(pdev->dev.of_node,
                                     of_dma_xlate_by_chan_id, cqdma);

If mtk_cqdma_hw_init() fails, the error path jumps to err_unregister, which
fails to call of_dma_controller_free():

err_unregister:
    dma_async_device_unregister(dd);

    return err;

When devres frees the cqdma device structure, will this leave a dangling
pointer that can be accessed by subsequent DMA requests?

>  		cqdma->pc[i]->base = devm_platform_ioremap_resource(pdev, i);
>  		if (IS_ERR(cqdma->pc[i]->base))
>  			return PTR_ERR(cqdma->pc[i]->base);
> @@ -869,10 +870,6 @@ static int mtk_cqdma_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, cqdma);
>  
> -	/* initialize tasklet for each PC */
> -	for (i = 0; i < cqdma->dma_channels; ++i)
> -		tasklet_setup(&cqdma->pc[i]->tasklet, mtk_cqdma_tasklet_cb);
> -
>  	dev_info(&pdev->dev, "MediaTek CQDMA driver registered\n");
>  
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown order in
mtk_cqdma_remove() allow tasklets to be re-scheduled after being killed?

VC tasklets are killed before PC hardware interrupts are disabled and
PC tasklets are killed:

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

If an interrupt fires in this window and schedules a PC tasklet, couldn't
the PC tasklet call vchan_cookie_complete() and reschedule the VC tasklet
after it was already killed, leading to a use-after-free when devres frees
the tasklet memory?


[Severity: High]
This is a pre-existing issue, but does the error path in
mtk_cqdma_prep_dma_memcpy() free descriptors that are already linked into
the virtual channel's list?

Descriptors are added to the active vc->desc_allocated list:

drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_prep_dma_memcpy() {
    ...
    tx = vchan_tx_prep(to_virt_chan(c), &cvd[i]->vd, flags);
    ...
}

If a subsequent memory allocation for cvd[i] fails, the error path frees
cvd[i - 1] without unlinking it:

    ...
    for (; i > 0; --i)
        kfree(cvd[i - 1]);
    return NULL;
}

Will the DMA framework later access this freed memory and cause a
use-after-free?


[Severity: High]
This isn't a bug introduced by this patch, but does a zero-length DMA
transfer request cause an out-of-bounds access?

If mtk_cqdma_prep_dma_memcpy() is called with len == 0, the allocation
count becomes 0:

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);
    ...
}

kzalloc_objs() will return the ZERO_SIZE_PTR error pointer. The
initialization loop is skipped, and the code attempts to dereference
it out-of-bounds:

    ...
    return &cvd[0]->vd.tx;
}

Could this trigger a kernel panic when a driver submits a 0-length DMA
request?


[Severity: High]
This is a pre-existing issue, but does issuing multiple pending
descriptors force the CPU to synchronously wait for hardware completion?

In mtk_cqdma_issue_vchan_pending(), the trigger_engine variable is scoped
outside the loop and never reset back to false:

drivers/dma/mediatek/mtk-cqdma.c:mtk_cqdma_issue_vchan_pending() {
    ...
    bool trigger_engine = false;
    ...
    if (trigger_engine)
        mtk_cqdma_start(pc, cvd);
    ...
}

For all subsequent queued descriptors, will this repeatedly execute
mtk_cqdma_start(), which busy-waits with spinlocks held and interrupts
disabled for the previous transaction to finish, potentially stalling
the system?

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