Re: [PATCH v3 2/4] 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] Releasing the panel reference in the unregister paths instead of a .destroy callback introduces a Use-After-Free.

Pre-existing issues:
- [High] Accessing `panel->dev` in `drm_panel_bridge_remove()` can cause a Use-After-Free of a `struct device` if the panel device has been unregistered.
--

--- Patch [2]: [PATCH v3 2/4] drm/bridge/panel: hold a reference to the wrapped panel ---
commit ac2f873b583c71488e3debe5277a8e3347eee2cd
Author: Albert Esteve <aesteve@redhat.com>

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

This commit acquires a reference to the drm_panel in 
drm_panel_bridge_add_typed() to prevent dangling pointers. It then 
releases the reference in each teardown path.

> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 4978ec98a0828..3bfa18167dd26 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 is a pre-existing issue, but could accessing panel->dev here lead to a 
use-after-free if the panel device has already been unregistered?

While drm_panel_get() keeps the drm_panel alive, it doesn't appear to acquire
a reference to the underlying struct device. If the device is unbound and 
freed, panel->dev becomes a dangling pointer.

Could devm_drm_put_bridge() then dereference it when traversing the devres 
list?

> +	drm_panel_put(panel);

[Severity: High]
Does this drop the panel reference prematurely and cause a regression?

Since drm_bridge objects are refcounted, calling drm_bridge_remove() only 
unregisters the bridge and drops the list reference. The bridge could remain 
alive if an active DRM pipeline holds a reference.

If the panel is freed here, could subsequent DRM operations on the bridge 
dereference panel_bridge->panel and trigger a use-after-free?

Would it be safer to move the drm_panel_put() to a new .destroy callback in 
panel_bridge_bridge_funcs, which is invoked by __drm_bridge_free() when the 
bridge's refcount reaches zero?

>  }
>  EXPORT_SYMBOL(drm_panel_bridge_remove);
>  
> @@ -357,11 +360,16 @@ 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;
> +	struct drm_panel *panel;
>  
>  	if (!bridge)
>  		return;
>  
> +	panel_bridge = drm_bridge_to_panel_bridge(bridge);
> +	panel = panel_bridge->panel;
>  	drm_bridge_remove(bridge);
> +	drm_panel_put(panel);

[Severity: High]
Similarly, if the bridge is kept alive by a reference elsewhere, does dropping 
the panel reference here risk a use-after-free regression in bridge callbacks 
later on?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-drm_refcount_wiring-v3-0-023900c32e01@redhat.com?part=2