[PATCH v1] drm/kmb: Fix device reference leaks in probe

[PATCH v1] drm/kmb: Fix device reference leaks in probe

[Yuho Choi]

kmb_probe() takes a reference to its own platform device only to keep a
local dev pointer, and passes an extra reference to the DSI platform
device into kmb_dsi_host_bridge_init(). Neither reference is released.

The DSI platform device returned by of_find_device_by_node() also carries
a reference that is not put on probe failure or after successful setup.

Use borrowed device pointers where no ownership is needed. Keep an owned
reference only for the registered DSI host device, and drop it when the
host is unregistered. Release the temporary of_find_device_by_node()
reference on all probe exits.

Fixes: 7f7b96a8a0a1 ("drm/kmb: Add support for KeemBay Display")
Signed-off-by: Yuho Choi <dbgh9129@gmail.com>
---
 drivers/gpu/drm/kmb/kmb_drv.c | 32 ++++++++++++++++++++------------
 drivers/gpu/drm/kmb/kmb_dsi.c | 31 ++++++++++++++++++++++++++-----
 2 files changed, 46 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
index 7c2eb1152fc2..f8ef618e3a81 100644
--- a/drivers/gpu/drm/kmb/kmb_drv.c
+++ b/drivers/gpu/drm/kmb/kmb_drv.c
@@ -479,7 +479,7 @@ static void kmb_remove(struct platform_device *pdev)
 
 static int kmb_probe(struct platform_device *pdev)
 {
-	struct device *dev = get_device(&pdev->dev);
+	struct device *dev = &pdev->dev;
 	struct kmb_drm_private *kmb;
 	int ret = 0;
 	struct device_node *dsi_in;
@@ -515,20 +515,24 @@ static int kmb_probe(struct platform_device *pdev)
 
 	of_node_put(dsi_in);
 	of_node_put(dsi_node);
-	ret = kmb_dsi_host_bridge_init(get_device(&dsi_pdev->dev));
+	ret = kmb_dsi_host_bridge_init(&dsi_pdev->dev);
 
-	if (ret == -EPROBE_DEFER) {
-		return -EPROBE_DEFER;
-	} else if (ret) {
-		DRM_ERROR("probe failed to initialize DSI host bridge\n");
-		return ret;
+	if (ret) {
+		if (ret != -EPROBE_DEFER) {
+			DRM_ERROR("probe failed to initialize DSI host bridge\n");
+			kmb_dsi_host_unregister(NULL);
+		}
+		goto err_put_dsi_pdev;
 	}
 
 	/* Create DRM device */
 	kmb = devm_drm_dev_alloc(dev, &kmb_driver,
 				 struct kmb_drm_private, drm);
-	if (IS_ERR(kmb))
-		return PTR_ERR(kmb);
+	if (IS_ERR(kmb)) {
+		ret = PTR_ERR(kmb);
+		kmb_dsi_host_unregister(NULL);
+		goto err_put_dsi_pdev;
+	}
 
 	dev_set_drvdata(dev, &kmb->drm);
 
@@ -537,11 +541,11 @@ static int kmb_probe(struct platform_device *pdev)
 	if (IS_ERR(kmb->kmb_dsi)) {
 		drm_err(&kmb->drm, "failed to initialize DSI\n");
 		ret = PTR_ERR(kmb->kmb_dsi);
-		goto err_free1;
+		dev_set_drvdata(dev, NULL);
+		kmb_dsi_host_unregister(NULL);
+		goto err_put_dsi_pdev;
 	}
 
-	kmb->kmb_dsi->dev = &dsi_pdev->dev;
-	kmb->kmb_dsi->pdev = dsi_pdev;
 	ret = kmb_hw_init(&kmb->drm, 0);
 	if (ret)
 		goto err_free1;
@@ -565,6 +569,7 @@ static int kmb_probe(struct platform_device *pdev)
 
 	drm_client_setup(&kmb->drm, NULL);
 
+	put_device(&dsi_pdev->dev);
 	return 0;
 
  err_register:
@@ -578,6 +583,9 @@ static int kmb_probe(struct platform_device *pdev)
 	dev_set_drvdata(dev, NULL);
 	kmb_dsi_host_unregister(kmb->kmb_dsi);
 
+ err_put_dsi_pdev:
+	put_device(&dsi_pdev->dev);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c
index aeb2f9f98f23..025384a6fb69 100644
--- a/drivers/gpu/drm/kmb/kmb_dsi.c
+++ b/drivers/gpu/drm/kmb/kmb_dsi.c
@@ -182,8 +182,19 @@ static void kmb_dsi_clk_disable(struct kmb_dsi *kmb_dsi)
 
 void kmb_dsi_host_unregister(struct kmb_dsi *kmb_dsi)
 {
-	kmb_dsi_clk_disable(kmb_dsi);
-	mipi_dsi_host_unregister(kmb_dsi->host);
+	struct device *dev;
+
+	if (kmb_dsi)
+		kmb_dsi_clk_disable(kmb_dsi);
+
+	if (!dsi_host || !dsi_host->dev)
+		return;
+
+	mipi_dsi_host_unregister(dsi_host);
+
+	dev = dsi_host->dev;
+	dsi_host->dev = NULL;
+	put_device(dev);
 }
 
 /*
@@ -217,6 +228,7 @@ static const struct mipi_dsi_host_ops kmb_dsi_host_ops = {
 int kmb_dsi_host_bridge_init(struct device *dev)
 {
 	struct device_node *encoder_node, *dsi_out;
+	int ret;
 
 	/* Create and register MIPI DSI host */
 	if (!dsi_host) {
@@ -230,12 +242,20 @@ int kmb_dsi_host_bridge_init(struct device *dev)
 			dsi_device = kzalloc_obj(*dsi_device);
 			if (!dsi_device) {
 				kfree(dsi_host);
+				dsi_host = NULL;
 				return -ENOMEM;
 			}
 		}
+	}
 
-		dsi_host->dev = dev;
-		mipi_dsi_host_register(dsi_host);
+	if (!dsi_host->dev) {
+		dsi_host->dev = get_device(dev);
+		ret = mipi_dsi_host_register(dsi_host);
+		if (ret) {
+			put_device(dsi_host->dev);
+			dsi_host->dev = NULL;
+			return ret;
+		}
 	}
 
 	/* Find ADV7535 node and initialize it */
@@ -1410,7 +1430,6 @@ int kmb_dsi_mode_set(struct kmb_dsi *kmb_dsi, struct drm_display_mode *mode,
 struct kmb_dsi *kmb_dsi_init(struct platform_device *pdev)
 {
 	struct kmb_dsi *kmb_dsi;
-	struct device *dev = get_device(&pdev->dev);
 
 	kmb_dsi = devm_kzalloc(dev, sizeof(*kmb_dsi), GFP_KERNEL);
 	if (!kmb_dsi) {
@@ -1420,6 +1439,8 @@ struct kmb_dsi *kmb_dsi_init(struct platform_device *pdev)
 
 	kmb_dsi->host = dsi_host;
 	kmb_dsi->host->ops = &kmb_dsi_host_ops;
+	kmb_dsi->dev = &pdev->dev;
+	kmb_dsi->pdev = pdev;
 
 	dsi_device->host = kmb_dsi->host;
 	kmb_dsi->device = dsi_device;
-- 
2.43.0

Re: [PATCH v1] drm/kmb: Fix device reference leaks in probe

[sashiko-bot]

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

New issues:
- [Low] Undeclared identifier `dev` in `kmb_dsi_init()` breaks the build.

Pre-existing issues:
- [High] Devres-managed resources are bound to the wrong device, causing memory leaks on probe deferrals.
- [High] The MIPI DSI host remains registered indefinitely if the DRM probe defers, leading to a Use-After-Free upon module unload.
- [High] Calling `drm_crtc_cleanup()` on an uninitialized CRTC causes a NULL pointer dereference and kernel panic.
- [High] Ignored return value from `kmb_dsi_encoder_init()` leads to incomplete DRM initialization and crashes.
- [High] Ignored return value from `kmb_initialize_clocks()` leads to hardware operations without enabled clocks.
--

commit fe279368e5a0418668dc2dae4e274724b0887c0b
Author: Yuho Choi <dbgh9129@gmail.com>

drm/kmb: Fix device reference leaks in probe

kmb_probe() takes a reference to its own platform device only to keep a
local dev pointer, and passes an extra reference to the DSI platform
device into kmb_dsi_host_bridge_init(). Neither reference is released.

> diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
> index 7c2eb1152fc2f..f8ef618e3a810 100644
> --- a/drivers/gpu/drm/kmb/kmb_drv.c
> +++ b/drivers/gpu/drm/kmb/kmb_drv.c
[ ... ]
> @@ -515,20 +515,24 @@ static int kmb_probe(struct platform_device *pdev)
>  
>  	of_node_put(dsi_in);
>  	of_node_put(dsi_node);
> -	ret = kmb_dsi_host_bridge_init(get_device(&dsi_pdev->dev));
> +	ret = kmb_dsi_host_bridge_init(&dsi_pdev->dev);
>  
> -	if (ret == -EPROBE_DEFER) {
> -		return -EPROBE_DEFER;
> -	} else if (ret) {
> -		DRM_ERROR("probe failed to initialize DSI host bridge\n");
> -		return ret;
> +	if (ret) {
> +		if (ret != -EPROBE_DEFER) {
> +			DRM_ERROR("probe failed to initialize DSI host bridge\n");
> +			kmb_dsi_host_unregister(NULL);
> +		}
> +		goto err_put_dsi_pdev;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but if kmb_probe() returns
-EPROBE_DEFER, it explicitly skips calling kmb_dsi_host_unregister(NULL).

If the module is subsequently unloaded while still in this deferred state,
could this cause a use-after-free since the globally allocated dsi_host
would remain registered in the MIPI DSI core's internal list pointing to
unloaded memory?

[ ... ]
> diff --git a/drivers/gpu/drm/kmb/kmb_dsi.c b/drivers/gpu/drm/kmb/kmb_dsi.c
> index 59d0e856392fc..a7df845088842 100644
> --- a/drivers/gpu/drm/kmb/kmb_dsi.c
> +++ b/drivers/gpu/drm/kmb/kmb_dsi.c
[ ... ]
> @@ -1410,7 +1430,6 @@ int kmb_dsi_mode_set(struct kmb_dsi *kmb_dsi, struct drm_display_mode *mode,
>  struct kmb_dsi *kmb_dsi_init(struct platform_device *pdev)
>  {
>  	struct kmb_dsi *kmb_dsi;
> -	struct device *dev = get_device(&pdev->dev);
>  
>  	kmb_dsi = devm_kzalloc(dev, sizeof(*kmb_dsi), GFP_KERNEL);

[Severity: Low]
Does the removal of the dev variable declaration cause a build failure?

It looks like dev is still being used here in devm_kzalloc() without
being declared in kmb_dsi_init().

[Severity: High]
This is a pre-existing issue, but is the memory allocation tied to the
wrong platform device?

The devm_kzalloc() call resolves dev to dsi_pdev->dev, which is the device
returned by of_find_device_by_node(), rather than the device actively
being probed.

Since dsi_pdev isn't cleaned up when kmb_probe() returns -EPROBE_DEFER,
won't this leak a new kmb_dsi structure into dsi_pdev's devres list on
every probe deferral?

[Severity: High]
This is a pre-existing issue, but does the error path in kmb_probe()
cause a NULL pointer dereference?

If kmb_setup_mode_config() fails before kmb->crtc is initialized,
the error path unconditionally jumps here:

drivers/gpu/drm/kmb/kmb_drv.c:kmb_probe() {
    ...
 err_free:
    drm_crtc_cleanup(&kmb->crtc);
    ...
}

Since devm_drm_dev_alloc() zeroes the memory, kmb->crtc.dev is NULL.
Calling drm_crtc_cleanup() on this uninitialized CRTC would dereference
this NULL pointer.

[Severity: High]
This is a pre-existing issue, but are we ignoring the return value
from kmb_dsi_encoder_init() here?

drivers/gpu/drm/kmb/kmb_drv.c:kmb_setup_mode_config() {
    ...
    ret = kmb_dsi_encoder_init(drm, kmb->kmb_dsi);
    /* Set the CRTC's port so that the encoder component can find it */
    kmb->crtc.port = of_graph_get_port_by_id(drm->dev->of_node, 0);
    ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
    ...
}

The result in ret is immediately overwritten by drm_vblank_init().
If encoder initialization fails, the driver proceeds to register the DRM
device with an incomplete display pipeline, which could lead to crashes
during subsequent modeset operations.

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the error code
returned from kmb_initialize_clocks()?

drivers/gpu/drm/kmb/kmb_drv.c:kmb_hw_init() {
    ...
    /* Enable display clocks */
    kmb_initialize_clocks(kmb, &pdev->dev);

    /* Allocate LCD interrupt resources */
    irq_lcd = platform_get_irq(pdev, 0);
    ...
}

If clock initialization fails, the driver silently continues execution
and attempts to access hardware MMIO registers. Accessing unclocked
MMIO registers usually triggers a synchronous external abort and hangs
the system.

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