Re: [PATCH] dmaengine: pl330: Fix NULL pointer dereference in tasklet during channel release

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Newly introduced non-recursive spinlock self-deadlock in pl330_free_chan_resources() when aborting pending descriptors.

Pre-existing issues:
- [Critical] Concurrent modifications to the shared `desc_pool` without acquiring `pool_lock` causes linked-list corruption.
- [Critical] Client callbacks for aborted descriptors can be invoked asynchronously after the client has freed the channel.
- [Critical] Out-of-bounds array access in pl330_update() upon spurious hardware events.
- [High] Missing pch->thread validation in fill_queue() leaves a NULL pointer dereference vulnerability, rendering the patch's fix incomplete.
--

--- Patch [1]: [PATCH] dmaengine: pl330: Fix NULL pointer dereference in tasklet during channel release ---
commit 46db9642cb9437f521f1604962399f1fd25810d2
Author: Mayank Mishra <mayankmishraa@google.com>

dmaengine: pl330: Fix NULL pointer dereference in tasklet during channel release

When a DMA transfer aborts, the interrupt handler schedules completion
callbacks and marks descriptors as DONE. Subsequent channel release via
pl330_free_chan_resources() clears the hardware thread by calling
pl330_release_channel(), which sets pch->thread to NULL. This commit
adds pch->lock inside pl330_free_chan_resources() to avoid a TOCTOU race
condition where pl330_tasklet() dereferences pch->thread after it is set
to NULL.

> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 25ba84b187041..acb9949f4a626 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2086,12 +2086,23 @@ static void pl330_tasklet(struct tasklet_struct *t)
>  	fill_queue(pch);

[Severity: High]
This is a pre-existing issue, but might the missing pch->thread validation
in fill_queue() result in a NULL pointer dereference, leaving the patch's fix
incomplete?

The patch correctly identifies that pch->thread can be set to NULL
asynchronously, but it only adds validation checks after fill_queue() is
executed inside pl330_tasklet().

If the tasklet runs after pch->thread is cleared, fill_queue() iterates over
pch->work_list and passes the NULL thread to pl330_submit_req():

drivers/dma/pl330.c:fill_queue() {
    ...
    ret = pl330_submit_req(pch->thread, desc);
    ...
}

Could passing a NULL pch->thread directly lead to a crash when it is
dereferenced?

>  	if (list_empty(&pch->work_list)) {
> -		spin_lock(&pch->thread->dmac->lock);
> -		_stop(pch->thread);
> -		spin_unlock(&pch->thread->dmac->lock);
> -		power_down = true;
> -		pch->active = false;
> -	} else {
> +		/*
> +		 * Verify pch->thread is still valid before dereferencing
> +		 * it, as it could be set to NULL asynchronously during
> +		 * channel release after transfer aborts.
> +		 */
> +		if (pch->thread) {

[ ... ]

> @@ -2357,7 +2369,14 @@ static void pl330_free_chan_resources(struct dma_chan *chan)
>  	tasklet_kill(&pch->task);

[Severity: Critical]
This isn't a bug introduced by this patch, but could client callbacks for
aborted descriptors be invoked asynchronously after the client has freed the
channel?

During channel release, pl330_free_chan_resources() invokes tasklet_kill()
early to synchronize. However, pl330_release_channel() subsequently aborts
active descriptors and calls dma_pl330_rqcb(), which reschedules the tasklet:

drivers/dma/pl330.c:dma_pl330_rqcb() {
    ...
    tasklet_schedule(&pch->task);
    ...
}

pl330_free_chan_resources() then returns, prompting the client driver to
fully free its DMA context. Will the scheduled tasklet execute later and
invoke dmaengine_desc_callback_invoke() on the freed client memory?

>  	pm_runtime_get_sync(pch->dmac->ddma.dev);
> -	spin_lock_irqsave(&pl330->lock, flags);
> +	/*
> +	 * Acquire pch->lock before pl330->lock to respect the locking hierarchy
> +	 * (pch->lock -> pl330->lock) used inside the tasklet. This ensures
> +	 * that setting pch->thread to NULL and checking it inside the tasklet
> +	 * is fully synchronized, preventing TOCTOU race conditions.
> +	 */
> +	spin_lock_irqsave(&pch->lock, flags);
> +	spin_lock(&pl330->lock);
>  
>  	pl330_release_channel(pch->thread);

[Severity: Critical]
Will acquiring pch->lock here result in a non-recursive spinlock
self-deadlock when aborting pending descriptors?

If a client releases a channel with active descriptors,
pl330_free_chan_resources() now acquires pch->lock and calls
pl330_release_channel(), which attempts to abort active descriptors by
calling dma_pl330_rqcb().

Looking at dma_pl330_rqcb(), it unconditionally attempts to acquire the exact
same non-recursive lock:

drivers/dma/pl330.c:dma_pl330_rqcb() {
    ...
    spin_lock_irqsave(&pch->lock, flags);
    ...
}

Will this cause the executing CPU to hang?

>  	pch->thread = NULL;
> @@ -2365,7 +2384,8 @@ static void pl330_free_chan_resources(struct dma_chan *chan)
>  	if (pch->cyclic)
>  		list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool);

[Severity: Critical]
This is a pre-existing issue, but is it safe to splice work_list into
desc_pool here without acquiring pool_lock?

In pl330_free_chan_resources(), pl330_tasklet(), and pl330_terminate_all(),
items are spliced or moved into the shared pl330->desc_pool while only
holding pch->lock or pl330->lock.

The pool_lock appears explicitly required to protect desc_pool, as seen in
pluck_desc().

Can multiple channels terminating simultaneously corrupt the shared list?

> -	spin_unlock_irqrestore(&pl330->lock, flags);
> +	spin_unlock(&pl330->lock);
> +	spin_unlock_irqrestore(&pch->lock, flags);
>  	pm_runtime_put_autosuspend(pch->dmac->ddma.dev);
>  	pl330_unprep_slave_fifo(pch);
>  }

[Severity: Critical]
This is a pre-existing issue, but could an unhandled event index trigger an
out-of-bounds array access in pl330_update()?

If a hardware event fires for an event index 'ev' that is currently
unallocated, pl330->events[ev] is set to -1. pl330_update() reads this and
uses it directly as an array index without validation:

drivers/dma/pl330.c:pl330_update() {
    ...
    id = pl330->events[ev];
    thrd = &pl330->channels[id];
    ...
}

Does this out-of-bounds memory read and subsequent memory write corrupt
adjacent kernel structures when thrd->req_running is evaluated?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709125914.1079328-1-mayankmishraa@google.com?part=1