[PATCH] dmaengine: stm32-dma3: disable the active clock on removal

[PATCH] dmaengine: stm32-dma3: disable the active clock on removal

[Myeonghun Pak]

Probe enables the controller clock and runtime suspend disables it.
The remove callback only disables runtime PM, which does not itself
invoke runtime suspend. If the controller is still active, its clock
prepare and enable references therefore remain held after removal.

Remove the OF DMA provider and disable runtime PM, then release the
channel IRQs before disabling the clock. The IRQ handler reads controller
registers, so wait for it to finish rather than leaving IRQ release to
the managed cleanup that runs after remove returns.

Disable the clock only if the device is not already suspended, then mark
it suspended. This avoids a second clock disable for a runtime-suspended
device and also covers CONFIG_PM=n.

This issue was identified during our ongoing static-analysis research while
reviewing kernel code.

Fixes: f561ec8b2b33 ("dmaengine: Add STM32 DMA3 support")
Assisted-by: OpenAI:GPT-5.6
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/dma/stm32/stm32-dma3.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/dma/stm32/stm32-dma3.c b/drivers/dma/stm32/stm32-dma3.c
index 4724e7fa0008e177160d085172cb89d6bc483c4f..b4d68be5a46b8aa1751dd32234d42f6e6b470335 100644
--- a/drivers/dma/stm32/stm32-dma3.c
+++ b/drivers/dma/stm32/stm32-dma3.c
@@ -1926,7 +1926,21 @@ static int stm32_dma3_probe(struct platform_device *pdev)
 
 static void stm32_dma3_remove(struct platform_device *pdev)
 {
+	struct stm32_dma3_ddata *ddata = platform_get_drvdata(pdev);
+	struct dma_chan *c;
+
+	of_dma_controller_free(pdev->dev.of_node);
 	pm_runtime_disable(&pdev->dev);
+
+	list_for_each_entry(c, &ddata->dma_dev.channels, device_node) {
+		struct stm32_dma3_chan *chan = to_stm32_dma3_chan(c);
+
+		devm_free_irq(&pdev->dev, chan->irq, chan);
+	}
+
+	if (!pm_runtime_status_suspended(&pdev->dev))
+		clk_disable_unprepare(ddata->clk);
+	pm_runtime_set_suspended(&pdev->dev);
 }
 
 static int stm32_dma3_runtime_suspend(struct device *dev)
-- 
2.53.0

Re: [PATCH] dmaengine: stm32-dma3: disable the active clock on removal

[sashiko-bot]

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

New issues:
- [High] Devres sequence inversion explicitly disables the active clock in `remove()` before the DMA device is unregistered, creating a race where clients can access unclocked hardware.

Pre-existing issues:
- [High] IRQ handler accesses hardware registers without runtime PM protection, risking a crash if the device is suspended concurrently.
- [High] Missing tasklet_kill() for vchan tasklets causes Use-After-Free and hardware races during device removal.
--

commit 267d53077eeb3e6ecf120df2e24f7e2dab91c06d
Author: Myeonghun Pak <mhun512@gmail.com>

dmaengine: stm32-dma3: disable the active clock on removal

Probe enables the controller clock and runtime suspend disables it.
The remove callback only disables runtime PM, which does not itself
invoke runtime suspend. If the controller is still active, its clock
prepare and enable references therefore remain held after removal.

> diff --git a/drivers/dma/stm32/stm32-dma3.c b/drivers/dma/stm32/stm32-dma3.c
> index 4724e7fa0008e..b4d68be5a46b8 100644
> --- a/drivers/dma/stm32/stm32-dma3.c
> +++ b/drivers/dma/stm32/stm32-dma3.c
> @@ -1926,7 +1926,21 @@ static int stm32_dma3_probe(struct platform_device *pdev)
>  
>  static void stm32_dma3_remove(struct platform_device *pdev)
>  {
> +	struct stm32_dma3_ddata *ddata = platform_get_drvdata(pdev);
> +	struct dma_chan *c;
> +
> +	of_dma_controller_free(pdev->dev.of_node);
>  	pm_runtime_disable(&pdev->dev);

[Severity: High]
This isn't a bug introduced by this patch, but can the interrupt handler
stm32_dma3_chan_irq() access unclocked hardware registers if a client
releases a channel concurrently?

The interrupt handler directly reads STM32_DMA3_MISR without acquiring a
runtime PM reference to guarantee the device is powered.

If a client releases a channel, stm32_dma3_free_chan_resources() executes,
dropping the channel lock and calling pm_runtime_put_sync(), which
synchronously suspends the device and disables its clock.

If an interrupt fires just before the lock is acquired in
stm32_dma3_free_chan_resources(), the handler spins. Once the lock is released,
the handler proceeds to read the register concurrently with the clock being
disabled by pm_runtime_put_sync(), which can trigger a crash.

> +
> +	list_for_each_entry(c, &ddata->dma_dev.channels, device_node) {
> +		struct stm32_dma3_chan *chan = to_stm32_dma3_chan(c);
> +
> +		devm_free_irq(&pdev->dev, chan->irq, chan);

[Severity: High]
This is a pre-existing issue, but does this loop also need to kill pending
vchan tasklets to prevent use-after-free and hardware races?

Virt-dma channels schedule a tasklet (chan->vchan.task) to process asynchronous
completions. If a tasklet was scheduled by an interrupt just before the IRQ was
freed, it remains pending and will execute asynchronously after remove() returns.

At that point, the clock is disabled by remove(), so any hardware access
initiated by the tasklet invoking client callbacks will crash the system. Once
devres frees the channel structures, the running tasklet will also access freed
memory.

> +	}
> +
> +	if (!pm_runtime_status_suspended(&pdev->dev))
> +		clk_disable_unprepare(ddata->clk);

[Severity: High]
Does disabling the clock here create a window where clients can access
unclocked hardware?

The driver registers the DMA device using dmaenginem_async_device_register().
Devres-managed cleanup runs after stm32_dma3_remove() returns. By manually
disabling the controller clock here, there is a window where the DMA device
remains registered and exposed to clients, but its hardware clock is disabled.

If a client issues a transfer or releases a channel during this window, the
driver might attempt to read or write hardware registers, causing a crash.

> +	pm_runtime_set_suspended(&pdev->dev);
>  }
>  
>  static int stm32_dma3_runtime_suspend(struct device *dev)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911184841.34222-1-mhun512@gmail.com?part=1