Re: [PATCH 2/5] drm/bridge/panel: hold a reference to the wrapped panel

[sashiko-bot@kernel.org]

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

New issues:
- [High] Accessing `panel_bridge->panel` after calling `drm_bridge_remove()` causes a use-after-free if the panel device was removed first.

Pre-existing issues:
- [High] Passing `panel->dev` to `devm_drm_put_bridge()` can cause a use-after-free or a WARN_ON if the panel's struct device has already been freed.
--

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

drm/bridge/panel: hold a reference to the wrapped panel

drm_panel_bridge_add_typed() stores a pointer to the drm_panel it
wraps, but never acquires a reference to it. If the panel device
goes away while a panel_bridge still exists, the dangling pointer can
be dereferenced through panel_bridge->panel.

Acquire a reference in drm_panel_bridge_add_typed() with drm_panel_get()
and release it in each teardown path.

> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 4978ec98a0828..6b98ad19508df 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c

[ ... ]

> @@ -316,6 +316,7 @@ EXPORT_SYMBOL(drm_panel_bridge_add_typed);
>  void drm_panel_bridge_remove(struct drm_bridge *bridge)
>  {
>  	struct panel_bridge *panel_bridge;
> +	struct drm_panel *panel;
>  
>  	if (!bridge)
>  		return;
> @@ -326,10 +327,12 @@ void drm_panel_bridge_remove(struct drm_bridge *bridge)
>  	}
>  
>  	panel_bridge = drm_bridge_to_panel_bridge(bridge);
> +	panel = panel_bridge->panel;
>  
>  	drm_bridge_remove(bridge);
>  	/* TODO remove this after reworking panel_bridge lifetime */
> -	devm_drm_put_bridge(panel_bridge->panel->dev, bridge);
> +	devm_drm_put_bridge(panel->dev, bridge);

[Severity: High]
This isn't a bug introduced by this patch, but could this pass a freed or
unbound device pointer to devm_drm_put_bridge()?

If the panel device is unbound and unregistered, its struct device might be
freed if no other references are held. Even though the struct drm_panel is
kept alive by drm_panel_get(), it does not inherently hold a reference to
panel->dev.

If the host device later removes the bridge via drm_panel_bridge_remove(),
passing panel->dev to devm_drm_put_bridge() would dereference a dangling
pointer, or trigger a WARN_ON if the device was kept alive but unbound.

> +	drm_panel_put(panel);
>  }
>  EXPORT_SYMBOL(drm_panel_bridge_remove);

[ ... ]

> @@ -357,11 +360,14 @@ EXPORT_SYMBOL(drm_panel_bridge_set_orientation);
>  static void devm_drm_panel_bridge_release(struct device *dev, void *res)
>  {
>  	struct drm_bridge *bridge = *(struct drm_bridge **)res;
> +	struct panel_bridge *panel_bridge;
>  
>  	if (!bridge)
>  		return;
>  
> +	panel_bridge = drm_bridge_to_panel_bridge(bridge);
>  	drm_bridge_remove(bridge);
> +	drm_panel_put(panel_bridge->panel);

[Severity: High]
Does this cause a use-after-free of panel_bridge?

If the panel device is unbound before the host device, its devres actions
drop the bridge's initial reference. Later, when the host device unbinds,
devm_drm_panel_bridge_release() calls drm_bridge_remove().

Because that was the final reference, drm_bridge_remove() frees the
panel_bridge structure. The immediate next line dereferences
panel_bridge->panel.

Should we cache the panel pointer in a local variable before calling
drm_bridge_remove(), similar to what was done in drm_panel_bridge_remove()?

>  }

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