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

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

[Mayank Mishra]

When a DMA transfer aborts (e.g., due to a translation fault), the
interrupt handler stops the channel, schedules completion callbacks,
and marks the descriptors as DONE with an error status.

Subsequent channel release via pl330_free_chan_resources() clears the
hardware thread by calling pl330_release_channel(), which reschedules
the tasklet, and then sets pch->thread to NULL.

Because pl330_free_chan_resources() and pl330_tasklet() were not
synchronized on the setting of pch->thread (holding pl330->lock and
pch->lock respectively), a TOCTOU race condition occurred:

1. pl330_tasklet() verified that pch->thread was non-NULL.
2. pl330_free_chan_resources() concurrently set pch->thread to NULL.
3. pl330_tasklet() resumed and dereferenced pch->thread->dmac->lock,
   resulting in a kernel NULL pointer dereference at offset 0x10.

Resolve this by acquiring the channel lock pch->lock before the global
pl330->lock inside pl330_free_chan_resources() when setting the thread
pointer to NULL, enforcing the driver's existing locking hierarchy
(pch->lock -> pl330->lock). Additionally, add safety checks inside
pl330_tasklet() to verify that pch->thread is valid before dereferencing.

Fixes: b3040e40675e ("DMA: PL330: Add dma api driver")
Signed-off-by: Mayank Mishra <mayankmishraa@google.com>
---
 drivers/dma/pl330.c | 38 +++++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
index 25ba84b18704..acb9949f4a62 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);
 
 	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) {
+			spin_lock(&pch->thread->dmac->lock);
+			_stop(pch->thread);
+			spin_unlock(&pch->thread->dmac->lock);
+			power_down = true;
+			pch->active = false;
+		}
+	} else if (pch->thread) {
+		/*
+		 * Verify pch->thread is valid before starting it, ensuring
+		 * safe abort cleanups when channel resources are released.
+		 */
 		/* Make sure the PL330 Channel thread is active */
 		spin_lock(&pch->thread->dmac->lock);
 		pl330_start_thread(pch->thread);
@@ -2109,7 +2120,8 @@ static void pl330_tasklet(struct tasklet_struct *t)
 		if (pch->cyclic) {
 			desc->status = PREP;
 			list_move_tail(&desc->node, &pch->work_list);
-			if (power_down) {
+			/* Verify thread validity before restarting cyclic channel */
+			if (power_down && pch->thread) {
 				pch->active = true;
 				spin_lock(&pch->thread->dmac->lock);
 				pl330_start_thread(pch->thread);
@@ -2357,7 +2369,14 @@ static void pl330_free_chan_resources(struct dma_chan *chan)
 	tasklet_kill(&pch->task);
 
 	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);
 	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);
 
-	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);
 }
-- 
2.55.0.795.g602f6c329a-goog

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

[sashiko-bot]

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