Re: [PATCH 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:

New issues:
- [Medium] Memory leak of DRM panel reference when multiple endpoints define panels in pl111_modeset_init().

Pre-existing issues:
- [High] Uninitialized `tmp_bridge` can be used and dereferenced if `drm_of_find_panel_or_bridge` returns `-ENODEV`.
- [High] Passing an `ERR_PTR` to `drm_panel_bridge_remove()` in `tve200_modeset_init()` error path causes a kernel panic.
- [High] Missing `IS_ERR()` check on bridge returned by `devm_drm_panel_bridge_add_typed()` leads to `ERR_PTR` dereference in `ingenic_drm_bind()`.
- [High] Overwriting `bridge->funcs` corrupts external bridges and defeats `drm_panel_bridge_remove()` cleanup in `rockchip_lvds_bind()`.
--

commit ffb6fad5e4cf513ed7c3fe05c38be4669593628f
Author: Albert Esteve <aesteve@redhat.com>

drm: release panel reference after panel bridge creation

This commit updates drivers to release the panel reference after wrapping
it in a panel bridge, as the panel bridge now holds its own reference. It
also drops unused references on error paths.

> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 42c86f195c66b..1887e01d29701 100644
> --- 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 does this missing error check lead to a
panic? 

devm_drm_panel_bridge_add_typed() returns an ERR_PTR on failure. By
not checking IS_ERR(), the error pointer is stored and later passed to
drm_bridge_attach(), which will try to dereference it since it only checks
for NULL.

>  
>  		ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
>  					NULL, DRM_MODE_ENCODER_DPI, NULL);

[ ... ]

> diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
> index ac7b1d12a0f59..8ec659b3c08eb 100644
> --- a/drivers/gpu/drm/pl111/pl111_drv.c
> +++ b/drivers/gpu/drm/pl111/pl111_drv.c
> @@ -145,6 +145,7 @@ static int pl111_modeset_init(struct drm_device *dev)
>  	if (panel) {
>  		bridge = drm_panel_bridge_add_typed(panel,
>  						    DRM_MODE_CONNECTOR_Unknown);
> +		drm_panel_put(panel);

[Severity: Medium]
Could this leak panel references if multiple endpoints define panels?
Earlier in pl111_modeset_init(), it iterates through endpoints:

	for_each_endpoint_of_node(np, remote) {
		struct drm_panel *tmp_panel;
		...
		if (tmp_panel) {
			...
			panel = tmp_panel;
		}
	}

If the loop iterates multiple times and finds more than one panel, the
panel variable is overwritten without calling drm_panel_put() on the
previously found panel. Since this patch makes the panels return counted
references, does this introduce a leak for the overwritten panels?

[Severity: High]
This is a pre-existing issue, but could an uninitialized tmp_bridge cause a
crash here? In the same loop:

	for_each_endpoint_of_node(np, remote) {
		struct drm_panel *tmp_panel;
		struct drm_bridge *tmp_bridge;

		ret = drm_of_find_panel_or_bridge(..., &tmp_panel, &tmp_bridge);

If drm_of_find_panel_or_bridge() returns -ENODEV, the tmp_bridge parameter
is untouched. The code then evaluates if (tmp_bridge), which checks
uninitialized stack memory. If it evaluates to true, the garbage pointer is
assigned to the bridge variable and eventually dereferenced.

>  		if (IS_ERR(bridge)) {
>  			ret = PTR_ERR(bridge);
>  			goto finish;

[ ... ]

> diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c b/drivers/gpu/drm/rockchip/rockchip_lvds.c
> index 7a0c4fa29f2f0..f754445d2631b 100644
> --- 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);

[Severity: High]
This is a pre-existing issue, but does overwriting the bridge functions
cause memory leaks or panel failures? Later in rockchip_lvds_bind(), the
code does:

	lvds->bridge->funcs = &rockchip_lvds_bridge_funcs;

Since lvds->bridge points to an external bridge or panel bridge created by
drm_panel_bridge_add_typed(), overwriting its function pointers modifies an
object owned by another driver. 

Does this permanently disable the bridge's original operations? Additionally,
would drm_panel_bridge_remove() in the error path skip freeing the bridge
since it checks if the funcs pointer matches the default panel bridge
functions?

>  			goto err_free_encoder;

[ ... ]

> diff --git a/drivers/gpu/drm/tve200/tve200_drv.c b/drivers/gpu/drm/tve200/tve200_drv.c
> index 562f3f11812a3..f858c58ccb994 100644
> --- 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 can the error path cause a panic? 

If drm_panel_bridge_add_typed() fails and returns an ERR_PTR, we jump to
out_bridge:

out_bridge:
	if (panel)
		drm_panel_bridge_remove(bridge);

Since panel is not NULL, drm_panel_bridge_remove(bridge) is called with the
ERR_PTR. The removal function checks if (!bridge) which is false for an
ERR_PTR, and then immediately dereferences bridge->funcs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626-drm_refcount_wiring-v1-0-cca1a7b3bdef@redhat.com?part=5