Re: [PATCH v2 5/5] drm: release panel reference after panel bridge creation

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Calling `drm_panel_bridge_remove` with an `ERR_PTR` on the error path causes a kernel oops.
- [High] Missing `IS_ERR` check for `devm_drm_panel_bridge_add_typed` return value leads to `ERR_PTR` dereference in `drm_bridge_attach`.
- [High] Error handling failures in `rockchip_rgb_init()` lead to a dangling encoder in DRM core lists and a leaked panel bridge.
- [Medium] Hijacking `bridge->funcs` in `rockchip_lvds_bind` breaks panel bridge cleanup, causing memory and reference leaks.
- [Medium] The `mcde_drv` driver leaks the panel bridge and panel reference on error paths and during teardown.
--

--- Patch [5]: [PATCH v2 5/5] drm: release panel reference after panel bridge creation ---
commit 03ed59fea0a81ee259951270dc961716491dc0bd
Author: Albert Esteve <aesteve@redhat.com>
Subject: drm: release panel reference after panel bridge creation

This commit updates a second batch of drivers calling of_drm_find_panel() or
drm_of_find_panel_or_bridge() to release the lookup reference after wrapping
the panel in a bridge. It also handles paths where a panel is found but
cannot be used by dropping the reference immediately.

> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -1297,9 +1297,11 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>  			goto err_drvdata;
>  		}
>  
> -		if (panel)
> +		if (panel) {
>  			bridge = devm_drm_panel_bridge_add_typed(dev, panel,
>  								 DRM_MODE_CONNECTOR_DPI);
> +			drm_panel_put(panel);
> +		}
>  

[Severity: High]
This is a pre-existing issue, but I noticed devm_drm_panel_bridge_add_typed()
can return an ERR_PTR on failure. Since there is no IS_ERR() check here in
ingenic_drm_bind(), the error pointer gets assigned to ib->next_bridge a few
lines later and is passed indirectly to drm_bridge_attach(). 

Could this cause the DRM core to dereference the error pointer (e.g., accessing
bridge->dev) and crash during initialization?

[ ... ]

> diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c
> --- a/drivers/gpu/drm/mcde/mcde_drv.c
> +++ b/drivers/gpu/drm/mcde/mcde_drv.c
> @@ -153,6 +153,7 @@ static int mcde_modeset_init(struct drm_device *drm)
>  		if (panel) {
>  			bridge = drm_panel_bridge_add_typed(panel,
>  					DRM_MODE_CONNECTOR_DPI);
> +			drm_panel_put(panel);
>  			if (IS_ERR(bridge)) {
>  				dev_err(drm->dev,
>  					"Could not connect panel bridge\n");

[Severity: Medium]
This isn't a bug introduced by this patch, but it looks like the unmanaged
bridge created here in mcde_modeset_init() is never removed. 

If mcde_modeset_init() fails later (for example, if drm_vblank_init() fails),
or during standard driver teardown, does this permanently leak the panel bridge
and the panel reference?

[ ... ]

> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> @@ -605,6 +605,7 @@ static int rockchip_lvds_bind(struct device *dev, struct device *master,
>  
>  	if (lvds->panel) {
>  		lvds->bridge = drm_panel_bridge_add_typed(lvds->panel, DRM_MODE_CONNECTOR_LVDS);
> +		drm_panel_put(lvds->panel);
>  		if (IS_ERR(lvds->bridge)) {
>  			ret = PTR_ERR(lvds->bridge);
>  			goto err_free_encoder;

[Severity: Medium]
This is a pre-existing issue, but right after this check in
rockchip_lvds_bind(), the driver overwrites the bridge functions:

	lvds->bridge->funcs = &rockchip_lvds_bridge_funcs;

If drm_panel_bridge_remove() is called later in an error path or on unbind, it
checks the bridge identity via drm_bridge_is_panel(), which specifically looks
for bridge->funcs == &panel_bridge_bridge_funcs. Does overwriting the function
pointers here cause the removal to fail silently and leak the panel bridge?

[ ... ]

> diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c b/drivers/gpu/drm/rockchip/rockchip_rgb.c
> --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c
> @@ -139,6 +139,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev,
>  	if (panel) {
>  		bridge = drm_panel_bridge_add_typed(panel,
>  						    DRM_MODE_CONNECTOR_LVDS);
> +		drm_panel_put(panel);
>  		if (IS_ERR(bridge))
>  			return ERR_CAST(bridge);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but returning ERR_CAST(bridge)
directly here skips the err_free_encoder label. Since the embedding rgb struct
is allocated with devm_kzalloc(), does this leave a dangling encoder in the
DRM core lists when the memory is freed, potentially leading to a
use-after-free?

Additionally, in the err_free_encoder error path later in rockchip_rgb_init(),
drm_encoder_cleanup() is called but drm_panel_bridge_remove() is skipped. Does
that path permanently leak the unmanaged panel bridge?

[ ... ]

> diff --git a/drivers/gpu/drm/tve200/tve200_drv.c b/drivers/gpu/drm/tve200/tve200_drv.c
> --- a/drivers/gpu/drm/tve200/tve200_drv.c
> +++ b/drivers/gpu/drm/tve200/tve200_drv.c
> @@ -84,6 +84,7 @@ static int tve200_modeset_init(struct drm_device *dev)
>  	if (panel) {
>  		bridge = drm_panel_bridge_add_typed(panel,
>  						    DRM_MODE_CONNECTOR_Unknown);
> +		drm_panel_put(panel);
>  		if (IS_ERR(bridge)) {
>  			ret = PTR_ERR(bridge);
>  			goto out_bridge;
>  		}

[Severity: High]
This is a pre-existing issue, but when drm_panel_bridge_add_typed() fails here
in tve200_modeset_init(), the code jumps to out_bridge with an error pointer in
the bridge variable. 

The out_bridge label handles cleanup like this:

	out_bridge:
		if (panel)
			drm_panel_bridge_remove(bridge);

Since drm_panel_bridge_remove() calls drm_bridge_is_panel() which dereferences
bridge->funcs, won't this cause an immediate kernel panic when it tries to
dereference the error pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-drm_refcount_wiring-v2-0-d3bb61f4bd4d@redhat.com?part=5