[PATCH] drm/exynos: fimc: disable LCLK during cleanup

[PATCH] drm/exynos: fimc: disable LCLK during cleanup

[Myeonghun Pak]

fimc_setup_clocks() prepares and enables FIMC_CLK_LCLK after acquiring
the clocks.  The later component_add() failure path and normal remove path
only put the clock references, leaving LCLK prepared and enabled.

The runtime PM callbacks manage only FIMC_CLK_GATE, and component bind and
unbind do not change LCLK ownership.  Disable LCLK at the cleanup sites
that are reached only after fimc_setup_clocks() succeeds.  Keep the setup
failure path unchanged because it can put the clocks before LCLK has been
enabled.

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

Fixes: e5f8683923af ("drm/exynos: rework fimc clocks handling")
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/gpu/drm/exynos/exynos_drm_fimc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
index 09e33a26c..58214abb5 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
@@ -1362,6 +1362,7 @@ static int fimc_probe(struct platform_device *pdev)
 err_pm_dis:
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
+	clk_disable_unprepare(ctx->clocks[FIMC_CLK_LCLK]);
 	fimc_put_clocks(ctx);
 
 	return ret;
@@ -1376,6 +1377,7 @@ static void fimc_remove(struct platform_device *pdev)
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 
+	clk_disable_unprepare(ctx->clocks[FIMC_CLK_LCLK]);
 	fimc_put_clocks(ctx);
 }
 
-- 
2.47.1

Re: [PATCH] drm/exynos: fimc: disable LCLK during cleanup

[sashiko-bot]

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

New issues:
- [High] Unconditionally disabling LCLK in fimc_remove and the probe error path races with devm_request_irq cleanup, allowing the IRQ handler to execute while clocks are disabled.

Pre-existing issues:
- [High] The IRQ handler is registered before clocks are set up and the context spinlock is initialized, risking a bus hang and spinlock BUG.
- [High] Concurrent execution of fimc_irq_handler and fimc_abort causes a data race on ctx->task, leading to workqueue corruption.
- [High] Active tasks are not cancelled and pending async cleanup works are not flushed during module unbind in fimc_remove(), leading to memory leaks and Use-After-Free.
--

commit 71fc119244ac2354764b21e9e75759a36ca6bc94
Author: Myeonghun Pak <mhun512@gmail.com>
Subject: drm/exynos: fimc: disable LCLK during cleanup

The commit disables LCLK at the cleanup sites that are reached only after
fimc_setup_clocks() succeeds, because the later failure path and normal
remove path only put the clock references.

> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimc.c b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> index 09e33a26caaff..58214abb5a8e6 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimc.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimc.c
[ ... ]
> @@ -1362,6 +1362,7 @@ static int fimc_probe(struct platform_device *pdev)
>  err_pm_dis:
>  	pm_runtime_dont_use_autosuspend(dev);
>  	pm_runtime_disable(dev);
> +	clk_disable_unprepare(ctx->clocks[FIMC_CLK_LCLK]);

[Severity: High]
Does disabling the clock here race with the devm-managed IRQ handler?

Because the IRQ handler is managed by devm_request_irq(), it remains
registered and unmasked at the GIC until devres cleans it up after
fimc_probe() returns.

If a spurious interrupt occurs during this window, fimc_irq_handler()
could fire and access unclocked registers, which causes an external abort.

>  	fimc_put_clocks(ctx);
>  
>  	return ret;
> @@ -1376,6 +1377,7 @@ static void fimc_remove(struct platform_device *pdev)
>  	pm_runtime_dont_use_autosuspend(dev);
>  	pm_runtime_disable(dev);
>  

[Severity: High]
This isn't a bug introduced by this patch, but could removing the component
here without stopping hardware or flushing background tasks lead to a
use-after-free?

Active tasks are not cancelled and pending async cleanup works are not
flushed during module unbind in fimc_remove().

If the driver is unbound while an async task is running, devres frees the
fimc_context (ctx). The hardware completing the task triggers the IRQ, or
the pending work executes, both of which dereference the freed ctx.

> +	clk_disable_unprepare(ctx->clocks[FIMC_CLK_LCLK]);

[Severity: High]
Can manually disabling the clock here also race with devm_request_irq()
cleanup?

Similar to the probe error path, devm_request_irq() won't free the IRQ
until after fimc_remove() returns.

If the hardware is still active, or an interrupt occurs during this
window, fimc_irq_handler() would execute while clocks are disabled,
crashing the kernel.

>  	fimc_put_clocks(ctx);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but could the IRQ registration
order in fimc_probe() cause a bus hang or spinlock BUG?

The IRQ is requested and unmasked before hardware is clocked and the
spinlock is initialized:

	ret = devm_request_irq(dev, ret, fimc_irq_handler,
			       0, dev_name(dev), ctx);
	if (ret < 0) {
		dev_err(dev, "failed to request irq.\n");
		return ret;
	}

	ret = fimc_setup_clocks(ctx);
	if (ret < 0)
		return ret;

	spin_lock_init(&ctx->lock);

If the bootloader leaves the FIMC hardware active with a pending interrupt,
calling devm_request_irq() unmasks the interrupt immediately.

The handler fimc_irq_handler() will execute, which calls fimc_clear_irq()
to access unclocked registers and attempts to acquire ctx->lock before it
is initialized.

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of locking
when accessing ctx->task cause a data race between fimc_irq_handler() and
fimc_abort()?

fimc_abort() executes in process context and accesses ctx->task without
disabling interrupts:

	if (ctx->task) {
		struct exynos_drm_ipp_task *task = ctx->task;
		ctx->task = NULL;

Meanwhile, fimc_irq_handler() does the same:

	if (ctx->task) {
		struct exynos_drm_ipp_task *task = ctx->task;
		ctx->task = NULL;

If fimc_abort() runs concurrently with fimc_irq_handler() (e.g. userspace
aborts the task just as hardware completes it), both paths could see a
valid task pointer, set it to NULL, and call exynos_drm_ipp_task_done().

This leads to double scheduling of task->cleanup_work on the same
work_struct, corrupting the workqueue linked lists.

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