[PATCH] drm/logicvc: keep the IRQ within the display resource lifetime

[PATCH] drm/logicvc: keep the IRQ within the display resource lifetime

[Myeonghun Pak]

The managed IRQ is requested before the managed CRTC allocation. Probe
unwind can therefore free the CRTC while the handler remains installed.
Probe errors and remove also disable clocks before devres frees the IRQ.

Request the IRQ after display and vblank initialization. Release it on
registration failure and removal before disabling the clocks. Keep it
installed through atomic shutdown so pending commits can still receive
vblank completion.

This issue was found by static analysis.

Fixes: efeeaefe9be5 ("drm: Add support for the LogiCVC display controller")
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
Compile-tested with Clang, W=1 (x86_64): logicvc_drm.o and
logicvc_crtc.o. No runtime or hardware testing.

 drivers/gpu/drm/logicvc/logicvc_drm.c | 21 +++++++++++++--------
 drivers/gpu/drm/logicvc/logicvc_drm.h |  1 +
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/logicvc/logicvc_drm.c b/drivers/gpu/drm/logicvc/logicvc_drm.c
index bbebf4fc7f51..cf60bbb9800c 100644
--- a/drivers/gpu/drm/logicvc/logicvc_drm.c
+++ b/drivers/gpu/drm/logicvc/logicvc_drm.c
@@ -366,6 +366,7 @@ static int logicvc_drm_probe(struct platform_device *pdev)
 	drm_dev = &logicvc->drm_dev;
 
 	logicvc->regmap = regmap;
+	logicvc->irq = irq;
 	INIT_LIST_HEAD(&logicvc->layers_list);
 
 	caps = logicvc_drm_caps_match(logicvc);
@@ -385,13 +386,6 @@ static int logicvc_drm_probe(struct platform_device *pdev)
 		goto error_reserved_mem;
 	}
 
-	ret = devm_request_irq(dev, irq, logicvc_drm_irq_handler, 0,
-			       dev_name(dev), logicvc);
-	if (ret) {
-		drm_err(drm_dev, "Failed to request IRQ\n");
-		goto error_clocks;
-	}
-
 	ret = logicvc_drm_config_parse(logicvc);
 	if (ret && ret != -ENODEV) {
 		drm_err(drm_dev, "Failed to parse config\n");
@@ -434,16 +428,26 @@ static int logicvc_drm_probe(struct platform_device *pdev)
 		goto error_clocks;
 	}
 
+	ret = devm_request_irq(dev, irq, logicvc_drm_irq_handler, 0,
+			       dev_name(dev), logicvc);
+	if (ret) {
+		drm_err(drm_dev, "Failed to request IRQ\n");
+		goto error_mode;
+	}
+
 	ret = drm_dev_register(drm_dev, 0);
 	if (ret) {
 		drm_err(drm_dev, "Failed to register DRM device\n");
-		goto error_mode;
+		goto error_irq;
 	}
 
 	drm_client_setup(drm_dev, NULL);
 
 	return 0;
 
+error_irq:
+	devm_free_irq(dev, irq, logicvc);
+
 error_mode:
 	logicvc_mode_fini(logicvc);
 
@@ -466,6 +470,7 @@ static void logicvc_drm_remove(struct platform_device *pdev)
 	drm_dev_unregister(drm_dev);
 	drm_atomic_helper_shutdown(drm_dev);
 
+	devm_free_irq(dev, logicvc->irq, logicvc);
 	logicvc_mode_fini(logicvc);
 
 	logicvc_clocks_unprepare(logicvc);
diff --git a/drivers/gpu/drm/logicvc/logicvc_drm.h b/drivers/gpu/drm/logicvc/logicvc_drm.h
index e0f4787c69f9..f69538e6c0e9 100644
--- a/drivers/gpu/drm/logicvc/logicvc_drm.h
+++ b/drivers/gpu/drm/logicvc/logicvc_drm.h
@@ -53,6 +53,7 @@ struct logicvc_drm {
 	struct drm_device drm_dev;
 	phys_addr_t reserved_mem_base;
 	struct regmap *regmap;
+	int irq;
 
 	struct clk *vclk;
 	struct clk *vclk2;

[PATCH] drm/logicvc: keep the IRQ within the display resource lifetime

[Myeonghun Pak]

The interrupt handler accesses the CRTC and the register map. Its
device-managed release currently happens after remove has unprepared
the clocks, and the CRTC is allocated after the interrupt is requested.
Consequently a running handler can outlive the resources it uses, both
on removal and when probe unwinds.

Request the interrupt after initializing the display objects, and
release it explicitly before tearing down the mode state and clocks.
Keep it available through drm_atomic_helper_shutdown(), which may need
vblank completion. Also release it on drm_dev_register() failure before
unwinding the display resources.

This issue was identified as part of our ongoing research while
reviewing kernel code.

Fixes: efeeaefe9be5 ("drm: Add support for the LogiCVC display controller")
Assisted-by: LLM
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
Compile-tested with GCC, W=1 (arm64). No runtime testing.

 drivers/gpu/drm/logicvc/logicvc_drm.c | 21 +++++++++++++--------
 drivers/gpu/drm/logicvc/logicvc_drm.h |  1 +
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/logicvc/logicvc_drm.c b/drivers/gpu/drm/logicvc/logicvc_drm.c
index bbebf4f..cf60bbb 100644
--- a/drivers/gpu/drm/logicvc/logicvc_drm.c
+++ b/drivers/gpu/drm/logicvc/logicvc_drm.c
@@ -366,6 +366,7 @@ static int logicvc_drm_probe(struct platform_device *pdev)
 	drm_dev = &logicvc->drm_dev;
 
 	logicvc->regmap = regmap;
+	logicvc->irq = irq;
 	INIT_LIST_HEAD(&logicvc->layers_list);
 
 	caps = logicvc_drm_caps_match(logicvc);
@@ -385,13 +386,6 @@ static int logicvc_drm_probe(struct platform_device *pdev)
 		goto error_reserved_mem;
 	}
 
-	ret = devm_request_irq(dev, irq, logicvc_drm_irq_handler, 0,
-			       dev_name(dev), logicvc);
-	if (ret) {
-		drm_err(drm_dev, "Failed to request IRQ\n");
-		goto error_clocks;
-	}
-
 	ret = logicvc_drm_config_parse(logicvc);
 	if (ret && ret != -ENODEV) {
 		drm_err(drm_dev, "Failed to parse config\n");
@@ -434,16 +428,26 @@ static int logicvc_drm_probe(struct platform_device *pdev)
 		goto error_clocks;
 	}
 
+	ret = devm_request_irq(dev, irq, logicvc_drm_irq_handler, 0,
+			       dev_name(dev), logicvc);
+	if (ret) {
+		drm_err(drm_dev, "Failed to request IRQ\n");
+		goto error_mode;
+	}
+
 	ret = drm_dev_register(drm_dev, 0);
 	if (ret) {
 		drm_err(drm_dev, "Failed to register DRM device\n");
-		goto error_mode;
+		goto error_irq;
 	}
 
 	drm_client_setup(drm_dev, NULL);
 
 	return 0;
 
+error_irq:
+	devm_free_irq(dev, irq, logicvc);
+
 error_mode:
 	logicvc_mode_fini(logicvc);
 
@@ -466,6 +470,7 @@ static void logicvc_drm_remove(struct platform_device *pdev)
 	drm_dev_unregister(drm_dev);
 	drm_atomic_helper_shutdown(drm_dev);
 
+	devm_free_irq(dev, logicvc->irq, logicvc);
 	logicvc_mode_fini(logicvc);
 
 	logicvc_clocks_unprepare(logicvc);
diff --git a/drivers/gpu/drm/logicvc/logicvc_drm.h b/drivers/gpu/drm/logicvc/logicvc_drm.h
index e0f4787..f69538e 100644
--- a/drivers/gpu/drm/logicvc/logicvc_drm.h
+++ b/drivers/gpu/drm/logicvc/logicvc_drm.h
@@ -53,6 +53,7 @@ struct logicvc_drm {
 	struct drm_device drm_dev;
 	phys_addr_t reserved_mem_base;
 	struct regmap *regmap;
+	int irq;
 
 	struct clk *vclk;
 	struct clk *vclk2;

Re: [PATCH] drm/logicvc: keep the IRQ within the display resource lifetime

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] drm/logicvc: UAF of drm_plane during probe failure or unbind
--

commit 81e314862f436f8aee3b4c3a043bbab040bd125b
Author: Myeonghun Pak <mhun512@gmail.com>

drm/logicvc: keep the IRQ within the display resource lifetime

The interrupt handler accesses the CRTC and the register map. Its
device-managed release currently happens after remove has unprepared
the clocks, and the CRTC is allocated after the interrupt is requested.
Consequently a running handler can outlive the resources it uses, both
on removal and when probe unwinds.

Request the interrupt after initializing the display objects, and
release it explicitly before tearing down the mode state and clocks.
Keep it available through drm_atomic_helper_shutdown(), which may need
vblank completion. Also release it on drm_dev_register() failure before
unwinding the display resources.

> diff --git a/drivers/gpu/drm/logicvc/logicvc_drm.c b/drivers/gpu/drm/logicvc/logicvc_drm.c
> index bbebf4fc7f51a..cf60bbb9800c5 100644
> --- a/drivers/gpu/drm/logicvc/logicvc_drm.c
> +++ b/drivers/gpu/drm/logicvc/logicvc_drm.c
[ ... ]
> @@ -434,16 +428,26 @@ static int logicvc_drm_probe(struct platform_device *pdev)
>  		goto error_clocks;
>  	}
>  
> +	ret = devm_request_irq(dev, irq, logicvc_drm_irq_handler, 0,
> +			       dev_name(dev), logicvc);
> +	if (ret) {
> +		drm_err(drm_dev, "Failed to request IRQ\n");
> +		goto error_mode;
> +	}
> +

[Severity: Critical]
Does this error path (and other probe failures or device unbinds) trigger a
use-after-free of the drm_plane structures?

During probe failure unwinding or device unbind, devres will automatically
free the layer memory (which contains the drm_plane) because it was allocated
with devm_kzalloc(). 

However, neither devres nor logicvc_layer_fini() calls drm_plane_cleanup()
before the memory is freed:

drivers/gpu/drm/logicvc/logicvc_layer.c:logicvc_layer_fini() {
	struct device *dev = logicvc->drm_dev.dev;

	list_del(&layer->list);
	devm_kfree(dev, layer);
}

Because the plane is still linked in the DRM core plane list, the later call
to drm_mode_config_cleanup() traverses the list and dereferences the already
freed memory:

drivers/gpu/drm/drm_mode_config.c:drm_mode_config_cleanup() {
	...
	list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list, head) {
		plane->funcs->destroy(plane);
	}
	...
}

Should logicvc_layer_fini() call drm_plane_cleanup() before explicitly freeing
the layer structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179203400409.2736079.4502160101423826217.G31.logicvc-irq@gmail.com?part=1