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

[sashiko-bot@kernel.org]

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