diff mbox series

[v3,1/4] drm/panel: have drm_panel_add/remove manage a list reference

Message ID 20260717-drm_refcount_wiring-v3-1-023900c32e01@redhat.com (mailing list archive)
State New
Headers
Series drm/panel: refcounting panel lookups and references |

Commit Message

Albert Esteve July 17, 2026, 2:02 p.m. UTC 
The global panel_list holds raw pointers to drm_panel objects.
Nothing prevents a panel from being freed while it is still linked
in the list: if a driver's probe calls drm_panel_add() and then
fails at a later step, panel->list remains in panel_list. Any
subsequent call to of_drm_find_panel() that iterates the list will
dereference freed memory.

Have drm_panel_add() acquire a reference via drm_panel_get() before
inserting the panel into the list, and have drm_panel_remove() drop
it via drm_panel_put() after removing the panel from the list. The
global registry now holds a counted reference for as long as the
panel is listed, ensuring the object outlives any concurrent lookup.

Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 drivers/gpu/drm/drm_panel.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

sashiko-bot@kernel.org July 17, 2026, 2:31 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Modifying drm_panel_add() to take a reference replaces a list-iteration crash with a memory leak and a devres Use-After-Free for drivers that fail to call drm_panel_remove().

Pre-existing issues:
- [High] Lookup functions return raw pointers without acquiring references, failing to protect the caller from concurrent panel removal.
- [High] Use-After-Free in the drm_panel_follower API when a panel is unbound.
--

--- Patch [1]: [PATCH v3 1/4] drm/panel: have drm_panel_add/remove manage a list reference ---
commit 2e9c2d76403b7b7e3caee78c96bffbb1f61d8df7
Author: Albert Esteve <aesteve@redhat.com>

drm/panel: have drm_panel_add/remove manage a list reference

The commit adds a reference acquisition to drm_panel_add() and a release 
to drm_panel_remove() to prevent the panel from being freed while still 
linked in panel_list. The global registry now holds a counted reference 
for as long as the panel is listed.

> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 2c5649e433dfb..545fe93dc28fe 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -81,6 +81,7 @@ static void drm_panel_init(struct drm_panel *panel, struct device *dev,
>   */
>  void drm_panel_add(struct drm_panel *panel)
>  {
> +	drm_panel_get(panel);

[Severity: High]
Does taking a reference here replace a list-iteration crash with a memory
leak and a devres use-after-free for drivers that fail to call 
drm_panel_remove()?

If a driver calls drm_panel_add() but fails to call drm_panel_remove() on 
unbind, the new drm_panel_get() reference prevents the struct drm_panel from 
being freed, leaving it in panel_list. However, the driver's devres teardown 
still executes, freeing all underlying hardware resources like clocks, 
regulators, and GPIOs. 

If another driver later discovers this 'zombie' panel via 
of_drm_find_panel() and invokes its callbacks, will the panel driver access 
freed devres resources, causing a use-after-free or hardware lockup?

>  	mutex_lock(&panel_lock);
>  	list_add_tail(&panel->list, &panel_list);
>  	mutex_unlock(&panel_lock);
>  }


This isn't a bug introduced by this patch, but lookup functions return raw
pointers without acquiring references, failing to protect the caller from
concurrent panel removal:

drivers/gpu/drm/drm_panel.c:of_drm_find_panel() {
    ...
	list_for_each_entry(panel, &panel_list, list) {
		if (panel->dev->of_node == np) {
			mutex_unlock(&panel_lock);
			return panel;
		}
	}
    ...
}

[Severity: High]
While the commit message claims holding a reference in the registry ensures
the object outlives concurrent lookups, does this only protect the list 
iteration? Once the mutex is unlocked, if a concurrent thread executes 
drm_panel_remove() (e.g. due to driver unbind), could it drop the registry's 
reference and potentially free the panel memory while the caller is actively 
using the unprotected pointer?


This is also a pre-existing issue, but there appears to be a use-after-free
in the follower API when a panel is unbound:

drivers/gpu/drm/drm_panel.c:drm_panel_add_follower() {
    ...
	panel = find_panel_by_dev(follower_dev);
	if (IS_ERR(panel))
		return PTR_ERR(panel);

	get_device(panel->dev);
	follower->panel = panel;

	mutex_lock(&panel->follower_lock);
    ...
}

[Severity: High]
Does acquiring a reference to the panel's underlying device via 
get_device(panel->dev) fail to keep the devres-allocated struct drm_panel 
alive? Because the panel is allocated via devm_drm_panel_alloc(), its memory 
is tied to the driver's devres lifecycle, not the struct device refcount.

If the panel driver unbinds, devres frees the struct drm_panel memory 
regardless of the device refcount, leaving the follower driver with a dangling 
follower->panel pointer.

When the follower unbinds, it calls drm_panel_remove_follower():

drivers/gpu/drm/drm_panel.c:drm_panel_remove_follower() {
	struct drm_panel *panel = follower->panel;
	int ret;

	mutex_lock(&panel->follower_lock);
    ...
}

Won't attempting to lock the freed memory (mutex_lock(&panel->follower_lock)) 
cause a direct use-after-free?
Luca Ceresoli July 20, 2026, 1:11 p.m. UTC | #2 
On Fri, 17 Jul 2026 16:02:04 +0200, Albert Esteve <aesteve@redhat.com> wrote:
> The global panel_list holds raw pointers to drm_panel objects.
> Nothing prevents a panel from being freed while it is still linked
> in the list: if a driver's probe calls drm_panel_add() and then
> fails at a later step, panel->list remains in panel_list. Any
> subsequent call to of_drm_find_panel() that iterates the list will
> dereference freed memory.
> 
> [...]

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 2c5649e433dfb..545fe93dc28fe 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -81,6 +81,7 @@  static void drm_panel_init(struct drm_panel *panel, struct device *dev,
  */
 void drm_panel_add(struct drm_panel *panel)
 {
+	drm_panel_get(panel);
 	mutex_lock(&panel_lock);
 	list_add_tail(&panel->list, &panel_list);
 	mutex_unlock(&panel_lock);
@@ -98,6 +99,7 @@  void drm_panel_remove(struct drm_panel *panel)
 	mutex_lock(&panel_lock);
 	list_del_init(&panel->list);
 	mutex_unlock(&panel_lock);
+	drm_panel_put(panel);
 }
 EXPORT_SYMBOL(drm_panel_remove);