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

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 <[email protected]>

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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1