Re: [PATCH v2 08/64] dmaengine: imx-dma: flip per-chan tasklet to dma_chan BH

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

Pre-existing issues:
- [High] The per-channel watchdog timer is not synchronously canceled during driver teardown, leading to a Use-After-Free.
- [High] Pending BH works are not canceled on the probe error path, causing a Use-After-Free.
- [High] Early interrupts cause a NULL pointer dereference because IRQs are requested before channel lists are initialized.
- [High] Descriptor state is accessed after being placed on the free list and unlocking the spinlock, leading to a TOCTOU race with prep functions.
- [High] Channel state and lists are read and modified without proper lock protection in descriptor preparation functions.
- [High] The driver frees DMA descriptors in `imxdma_free_chan_resources` without synchronizing with the completion BH, causing a Use-After-Free.
--

--- Patch [8]: [PATCH v2 08/64] dmaengine: imx-dma: flip per-chan tasklet to dma_chan BH ---
commit 6c1578dd7a98a9ceebdc129c266b46d0c2bdf6e3
Author: Allen Pais <[email protected]>

dmaengine: imx-dma: flip per-chan tasklet to dma_chan BH

Replace the per-channel tasklet with the shared dma_chan BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/imx-dma.c b/drivers/dma/imx-dma.c
> index 81c6276436f81..3ff0bfbfa5ffa 100644
> --- a/drivers/dma/imx-dma.c
> +++ b/drivers/dma/imx-dma.c

[ ... ]

> @@ -592,9 +591,10 @@ static int imxdma_xfer_desc(struct imxdma_desc *d)
>  	return 0;
>  }
>  
> -static void imxdma_tasklet(struct tasklet_struct *t)
> +static void imxdma_tasklet(struct dma_chan *chan)
>  {
> -	struct imxdma_channel *imxdmac = from_tasklet(imxdmac, t, dma_tasklet);
> +	struct imxdma_channel *imxdmac = container_of(chan, struct imxdma_channel,
> +						      chan);
>  	struct imxdma_engine *imxdma = imxdmac->imxdma;
>  	struct imxdma_desc *desc, *next_desc;
>  	unsigned long flags;

[Severity: High]
This is a pre-existing issue, but is it safe to invoke the callback after
releasing the spinlock and moving the descriptor to the free list?

At the end of imxdma_tasklet(), the lock is released and the callback is
subsequently invoked directly from the descriptor:

out:
        spin_unlock_irqrestore(&imxdma->lock, flags);

        dmaengine_desc_get_callback_invoke(&desc->desc, NULL);
}

Since the descriptor is already on the free list, couldn't a concurrent
call to imxdma_prep_dma_memcpy() pick this exact descriptor from ld_free
and clear desc->desc.callback = NULL, leading to execution of a NULL or
corrupted callback?

[Severity: High]
This isn't a bug introduced by this patch, but how are the channel lists
synchronized against concurrent preparation?

The completion BH actively splices and modifies ld_free and ld_active while
holding imxdma->lock. However, prep functions like imxdma_prep_dma_memcpy()
read ld_free and call list_first_entry() without acquiring the channel lock:

drivers/dma/imx-dma.c:imxdma_prep_dma_memcpy() {
        ...
        if (list_empty(&imxdmac->ld_free) ||
            imxdma_chan_is_doing_cyclic(imxdmac))
                return NULL;

        desc = list_first_entry(&imxdmac->ld_free, struct imxdma_desc, node);
        ...
}

Could this lead to linked list corruption or NULL pointer dereferences if
clients actively submit transfers while completions fire?

[ ... ]

> @@ -1142,7 +1142,7 @@ static int __init imxdma_probe(struct platform_device *pdev)
>  		INIT_LIST_HEAD(&imxdmac->ld_free);
>  		INIT_LIST_HEAD(&imxdmac->ld_active);
>  
> -		tasklet_setup(&imxdmac->dma_tasklet, imxdma_tasklet);
> +		dma_chan_init_bh(&imxdmac->chan, imxdma_tasklet);
>  		imxdmac->chan.device = &imxdma->dma_device;
>  		dma_cookie_init(&imxdmac->chan);
>  		imxdmac->channel = i;

[Severity: High]
This is a pre-existing issue, but what happens if the probe fails after
this point?

Looking at the error path later in imxdma_probe():

err_of_dma_controller:
        dma_async_device_unregister(&imxdma->dma_device);
disable_dma_ahb_clk:
        clk_disable_unprepare(imxdma->dma_ahb);
disable_dma_ipg_clk:
        clk_disable_unprepare(imxdma->dma_ipg);
        return ret;

It exits without calling dma_chan_kill_bh() or imxdma_free_irq(). If a
spurious or lingering hardware interrupt triggers before
dma_async_device_register() completes, dma_irq_handler() schedules the BH.
If a subsequent initialization step fails, devres frees the device memory
but the scheduled BH is never canceled, which might lead to a use-after-free.

[Severity: High]
This isn't a bug introduced by this patch, but does the early interrupt
setup risk a NULL pointer dereference?

The IRQ is requested earlier in imxdma_probe():

        ret = devm_request_irq(&pdev->dev, irq,
                               dma_irq_handler, 0, "DMA", imxdma);

This happens well before the list heads like ld_active are initialized in
the loop shown in the diff above. If an interrupt fires early,
dma_irq_handle_channel() checks the list:

        if (list_empty(&imxdmac->ld_active)) {
                spin_unlock_irqrestore(&imxdma->lock, flags);
                goto out;
        }

Since memory is 0-initialized, ld_active.next is NULL. This makes
list_empty() evaluate to false, leading directly to a crash via
list_first_entry() right below it.

[ ... ]

> @@ -1211,7 +1211,7 @@ static void imxdma_free_irq(struct platform_device *pdev, struct imxdma_engine *
>  		if (!is_imx1_dma(imxdma))
>  			disable_irq(imxdmac->irq);
>  
> -		tasklet_kill(&imxdmac->dma_tasklet);
> +		dma_chan_kill_bh(&imxdmac->chan);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but is it safe to kill the BH here without
also stopping the watchdog timer?

This function stops the BH but omits timer_shutdown_sync(&imxdmac->watchdog),
leaving it active. If the driver is unbound via sysfs while a transfer is
active, imxdma_remove() invokes imxdma_free_irq() and then unregisters
and frees the engine. Couldn't the still-armed timer fire on freed memory
and schedule the destroyed BH workqueue?

[Severity: High]
This isn't a bug introduced by this patch, but does
imxdma_free_chan_resources() safely synchronize with this BH?

When a channel is released, descriptors are indiscriminately freed without
stopping or waiting for the BH to finish:

drivers/dma/imx-dma.c:imxdma_free_chan_resources() {
        ...
        list_for_each_entry_safe(desc, _desc, &imxdmac->ld_free, node) {
                kfree(desc);
                imxdmac->descs_allocated--;
        }
        ...
}

Since the driver lacks a device_synchronize callback and does not call
dma_chan_kill_bh() during channel release, could the completion BH be
actively running on another CPU trying to invoke the callback for a
descriptor that is being freed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8