Re: [PATCH v2 43/64] dmaengine: pxa_dma: kill vchan BH on teardown

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

Pre-existing issues:
- [High] Prematurely deleting channels from `dmadev->channels` during driver removal bypasses the dmaengine core's unregistration logic, permanently breaking module reload.
- [High] Critical Use-After-Free vulnerability on driver unbind due to improper devres cleanup ordering and failure to quiesce the hardware.
- [High] Missing `of_dma_controller_free()` on driver removal leaves a dangling pointer in the OF DMA list.
- [High] Potential Use-After-Free on the probe error path due to missing synchronization of bottom-half work.
--

--- Patch [43]: [PATCH v2 43/64] dmaengine: pxa_dma: kill vchan BH on teardown ---
commit 542f54f4e01925d8e48b9a9c649593d23bb50a91
Author: Allen Pais <[email protected]>

dmaengine: pxa_dma: kill vchan BH on teardown

Use dma_chan_kill_bh() for virt-dma cleanup.

> diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c
> index fa2ee0b3e09f8..d57e5e0c422cb 100644
> --- a/drivers/dma/pxa_dma.c
> +++ b/drivers/dma/pxa_dma.c
> @@ -1215,7 +1215,7 @@ static void pxad_free_channels(struct dma_device *dmadev)
>  	list_for_each_entry_safe(c, cn, &dmadev->channels,
>  				 vc.chan.device_node) {
>  		list_del(&c->vc.chan.device_node);

[Severity: High]
This is a pre-existing issue, but does this premature list deletion bypass the
dmaengine core's unregistration logic?

In pxad_remove(), the driver calls pxad_free_channels(), which uses list_del()
to manually remove each channel from dmadev->channels. The core framework's
dma_async_device_unregister() is executed subsequently as a devres action and
relies on traversing dmadev->channels to unregister each channel.

Because the list has been artificially emptied,
__dma_async_device_channel_unregister() is never invoked, skipping
device_unregister(), ida_free(), and free_percpu(). Will this leak sysfs
nodes (e.g., "dma0chan0") and cause permanent sysfs name collisions on
subsequent module loads?

> -		tasklet_kill(&c->vc.task);
> +		dma_chan_kill_bh(&c->vc.chan);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but is there a use-after-free vulnerability on
driver unbind due to improper devres cleanup ordering?

During initialization, pxad_init_phys() requests IRQs via devm_request_irq(),
and subsequently pxad_init_dmadev() allocates channel memory via
devm_kzalloc(). Because devres releases resources in reverse order, the channel
memory is freed before the IRQ is released.

Since pxad_remove() fails to mask interrupts or explicitly stop the DMA
hardware, if the hardware asserts an interrupt during this window:

drivers/dma/pxa_dma.c:pxad_chan_handler() {
    ...
    struct pxad_chan *chan = phy->vchan;
    ...
    spin_lock(&chan->vc.lock);
    ...
}

Will this dereference the already freed phy->vchan pointer and attempt to
acquire a freed spinlock if an interrupt fires during teardown?

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

In pxad_probe(), the driver registers its DMA controller using
of_dma_controller_register(), adding an of_dma structure to the global
of_dma_list. However, neither pxad_remove() nor any error path calls
of_dma_controller_free().

When the driver is unbound, will the of_dma structure remain in the global
list, containing pointers to the freed pdev memory and the unloaded
pxad_dma_xlate code, leading to a use-after-free if subsequent DMA requests
from other devices traverse the DT?

[Severity: High]
This is a pre-existing issue, but is there a potential use-after-free on the
probe error path due to missing synchronization of bottom-half work?

In pxad_probe(), if of_dma_controller_register() fails, the function returns
an error without flushing bottom-half work:

drivers/dma/pxa_dma.c:pxad_probe() {
    ...
    if (ret < 0) {
        dev_err(pdev->slave.dev,
                "of_dma_controller_register failed\n");
        return ret;
    }
    ...
}

Because dmaenginem_async_device_register() executes prior to this failure, the
DMA controller is briefly globally visible. If an external client requests a
channel and queues bh_work during this window, will the devres cleanup free
the channel memory before the asynchronous work executes?

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