diff mbox series

[v2,1/5] drm/panel: have drm_panel_add/remove manage a list reference

Message ID 20260713-drm_refcount_wiring-v2-1-d3bb61f4bd4d@redhat.com (mailing list archive)
State New
Headers
Series drm/panel: refcounting panel lookups and references |

Commit Message

Albert Esteve July 13, 2026, 11:53 a.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 13, 2026, 12:06 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Acquiring a reference in `drm_panel_add()` creates 'zombie' panels on driver probe failure, leading to deferred Use-After-Free of devres-managed resources.

Pre-existing issues:
- [High] `of_drm_find_panel()` and `find_panel_by_fwnode()` return un-refcounted pointers, leaving consumers vulnerable to TOCTOU Use-After-Free.
--

--- Patch [1]: [PATCH v2 1/5] drm/panel: have drm_panel_add/remove manage a list reference ---
commit b7da36095272a2101eb01a96e0511080d18be813
Author: Albert Esteve <aesteve@redhat.com>
Subject: drm/panel: have drm_panel_add/remove manage a list reference

This commit updates the panel registry to manage a reference count for
struct drm_panel objects while they are present in the global list. By
calling drm_panel_get() on addition and drm_panel_put() on removal, it
attempts to prevent panels from being freed while still linked.

> The global registry now holds a counted reference for as long as the
> panel is listed, ensuring the object outlives any concurrent lookup.

[Severity: High]
This is a pre-existing issue, but does the global list reference actually
ensure the object outlives the consumer's use of it?

Looking at of_drm_find_panel() and find_panel_by_fwnode(), they search the
list under panel_lock and return the pointer without acquiring a reference
for the caller:

of_drm_find_panel() {
	mutex_lock(&panel_lock);
	...
			mutex_unlock(&panel_lock);
			return panel;
}

As soon as panel_lock is unlocked, a concurrent driver unbind could call
drm_panel_remove(), which deletes the panel from the list and drops its
refcount. If the panel is then freed, won't the consumer thread that called
of_drm_find_panel() be left holding a dangling pointer?

> 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 acquiring a reference here create zombie panels if a driver's probe
fails after adding the panel?

For example, if a panel driver successfully calls drm_panel_add() but fails a
subsequent probe step (like mipi_dsi_attach) and forgets to call
drm_panel_remove(), the device's probe fails. The devres framework will then
free all devm-managed hardware resources like regulators, clocks, and GPIOs.

However, because the list now holds a reference, the struct drm_panel memory
is kept alive and remains in the registry. If a display controller later
looks up this zombie panel via of_drm_find_panel() and invokes its prepare()
or enable() callbacks, will it dereference those freed devres pointers?

>  	mutex_lock(&panel_lock);
>  	list_add_tail(&panel->list, &panel_list);
>  	mutex_unlock(&panel_lock);
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);