Re: [PATCH v2 36/64] dmaengine: sprd-dma: kill vchan BH on teardown

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver prematurely removes channels from `sdev->dma_dev.channels` before calling `dma_async_device_unregister()`, causing the core DMA framework to silently skip unregistering them, which leaks per-CPU memory, IDAs, and sysfs nodes, leading to a Use-After-Free.
- [High] IRQ is requested before the MMIO base is mapped and clocks are enabled, leading to a guaranteed NULL pointer dereference or bus hang if an interrupt is pending.
--

--- Patch [36]: [PATCH v2 36/64] dmaengine: sprd-dma: kill vchan BH on teardown ---
commit 90cb440f02a76b9f95f830c9b7cd74c3b47c9d5f
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: sprd-dma: kill vchan BH on teardown
    
Use dma_chan_kill_bh() for virt-dma cleanup.

> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 087fea3af2e41..42a0c1c34337b 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -1253,7 +1253,7 @@ static void sprd_dma_remove(struct platform_device *pdev)
>  	list_for_each_entry_safe(c, cn, &sdev->dma_dev.channels,
>  				 vc.chan.device_node) {
>  		list_del(&c->vc.chan.device_node);

[Severity: High]
This isn't a bug introduced by this patch, but removing the channels from
sdev->dma_dev.channels here causes the core dma framework to skip
unregistering them when dma_async_device_unregister() is called later in
this function:

drivers/dma/dmaengine.c:dma_async_device_unregister() {
    ...
    list_for_each_entry_safe(chan, n, &device->channels, device_node)
        __dma_async_device_channel_unregister(device, chan);
    ...
}

By emptying the channel list early, the sysfs nodes for the channels are
never unregistered.

Since the driver data is devres-managed and freed after unbind, does this
leave orphaned sysfs nodes pointing to freed memory, leading to a
use-after-free if a user reads the sysfs files?

> -		tasklet_kill(&c->vc.task);
> +		dma_chan_kill_bh(&c->vc.chan);
>  	}
>  
>  	of_dma_controller_free(pdev->dev.of_node);

[Severity: High]
This is another pre-existing issue, but there appears to be a null pointer
dereference risk during probe in this driver.

The interrupt is requested before the mmio base is mapped:

drivers/dma/sprd-dma.c:sprd_dma_probe() {
    ...
    ret = devm_request_irq(&pdev->dev, sdev->irq, dma_irq_handle,
                           0, "sprd_dma", (void *)sdev);
    ...
    sdev->glb_base = devm_platform_ioremap_resource(pdev, 0);
    ...
}

If an interrupt is already pending when devm_request_irq() unmasks it,
the interrupt handler will fire immediately and dereference the uninitialized
glb_base pointer:

drivers/dma/sprd-dma.c:dma_irq_handle() {
    ...
    u32 irq_status = readl(sdev->glb_base + SPRD_DMA_GLB_INT_MSK_STS);
    ...
}

Should the memory mapping and clock enablement be done before requesting
the interrupt to avoid this panic during boot or module load?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785183549.git.allen.lkml@gmail.com?part=36