Re: [PATCH v2 3/5] drm/panel: of_drm_find_panel() return a counted reference

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] In the Tegra DSI driver, the panel reference is freed twice during driver removal, causing a use-after-free and a double free of the kref.
- [Medium] Widespread panel reference leaks in unmodified callers of `of_drm_find_panel()` and `drm_of_find_panel_or_bridge()`.
- [Medium] Modified drivers leak the panel reference if their probe or initialization functions fail after the panel lookup.
- [Medium] `tegra_dsi_host_attach()` overwrites the `output->panel` pointer, leaking the reference previously acquired during `tegra_output_probe()`.

Pre-existing issues:
- [Critical] `find_panel_by_fwnode()` was not updated to acquire a reference, preserving the use-after-free window that the commit intended to fix.
--

--- Patch [3]: [PATCH v2 3/5] drm/panel: of_drm_find_panel() return a counted reference ---
commit a89c4180aa3b0d041a29e5173be0f8523bd150fe
Author: Albert Esteve <aesteve@redhat.com>

drm/panel: of_drm_find_panel() return a counted reference

This patch modifies of_drm_find_panel() to acquire a reference with
drm_panel_get() before returning the panel, preventing a race condition where
the device can be freed before first use. It also updates several drivers to
release this reference when the panel is no longer needed.

> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
[ ... ]
> @@ -225,7 +225,8 @@ EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
>   * @np: device tree node containing encoder output ports
>   * @port: port in the device tree node
>   * @endpoint: endpoint in the device tree node
> - * @panel: pointer to hold returned drm_panel, must not be NULL
> + * @panel: pointer to hold returned drm_panel, must not be NULL. On success
> + *         the caller must call drm_panel_put() when done with the panel
>   * @bridge: pointer to hold returned drm_bridge
>   *
>   * Given a DT node's port and endpoint number, find the connected node and

[Severity: Medium]
Since of_drm_find_panel() and drm_of_find_panel_or_bridge() now return a
panel with an incremented reference count, do all unmodified callers need
to be updated?

For example, in samsung_dsim_host_attach():

drivers/gpu/drm/bridge/samsung-dsim.c:samsung_dsim_host_attach() {
    ...
    panel = of_drm_find_panel(remote);
    ...
}

And in rockchip_lvds_bind():

drivers/gpu/drm/rockchip/rockchip_lvds.c:rockchip_lvds_bind() {
    ...
    ret = drm_of_find_panel_or_bridge(dev->of_node, 1, endpoint_id,
                                      &lvds->panel, &lvds->bridge);
    ...
}

Are these panel references leaked if the callers are not updated to call
drm_panel_put() upon success or error?

> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
[ ... ]
> @@ -484,6 +487,7 @@ struct drm_panel *of_drm_find_panel(const struct device_node *np)
>  
>  	list_for_each_entry(panel, &panel_list, list) {
>  		if (panel->dev->of_node == np) {
> +			drm_panel_get(panel);
>  			mutex_unlock(&panel_lock);
>  			return panel;
>  		}

[Severity: Critical]
This isn't a bug introduced by this patch, but find_panel_by_fwnode() in
drm_panel.c appears to have the same race condition this patch fixes for
of_drm_find_panel().

drivers/gpu/drm/drm_panel.c:find_panel_by_fwnode() {
    ...
    if (dev_fwnode(panel->dev) == fwnode) {
        mutex_unlock(&panel_lock);
        return panel;
    }
    ...
}

Since it returns a raw panel pointer without acquiring its reference count,
could the panel device be unregistered and freed before the caller (like
drm_panel_add_follower()) uses it?

Does this function need the same drm_panel_get() treatment?

> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
[ ... ]
> @@ -193,6 +193,16 @@ static int exynos_dp_probe(struct platform_device *pdev)
>  
>  static void exynos_dp_remove(struct platform_device *pdev)
>  {
> +	struct exynos_dp_device *dp = platform_get_drvdata(pdev);
> +
> +	/*
> +	 * Release the probe-time reference from of_drm_find_panel(). If bind
> +	 * ran, the panel_bridge holds a second reference that devm cleanup
> +	 * will release when the bridge is destroyed after remove() returns.
> +	 */
> +	if (dp->plat_data.panel)
> +		drm_panel_put(dp->plat_data.panel);
> +

[Severity: Medium]
If exynos_dp_probe() fails after acquiring the panel reference, does it leak
the reference?

Looking at the probe function:

drivers/gpu/drm/exynos/exynos_dp.c:exynos_dp_probe() {
    ...
    // panel reference acquired here or earlier
    ...
    if (IS_ERR(dp->adp))
        return PTR_ERR(dp->adp);
    ...
}

Since the driver core does not invoke remove() for failed probes, will
returning an error without calling drm_panel_put() cause a memory leak?
(A similar error-path leak pattern seems to exist in tegra_output_probe()).

[ ... ]
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index 7f25c50621c94..57a016f47434d 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -1516,6 +1516,7 @@ static int tegra_dsi_host_detach(struct mipi_dsi_host *host,
>  	struct tegra_output *output = &dsi->output;
>  
>  	if (output->panel && &device->dev == output->panel->dev) {
> +		drm_panel_put(output->panel);
>  		output->panel = NULL;

[Severity: Critical]
Could this lead to a use-after-free and double-free during driver removal?

During removal, tegra_dsi_remove() calls tegra_output_remove() which drops
the reference:

drivers/gpu/drm/tegra/output.c:tegra_output_remove() {
    if (output->panel)
        drm_panel_put(output->panel);
    ...
}

But output->panel is not set to NULL. Subsequently, mipi_dsi_host_unregister()
calls tegra_dsi_host_detach(), which executes this code:

    if (output->panel && &device->dev == output->panel->dev) {
        drm_panel_put(output->panel);

Will this dereference the freed output->panel->dev in the condition, and then
drop the kref a second time?

[Severity: Medium]
Also in the Tegra DSI driver, does tegra_dsi_host_attach() leak a reference?

drivers/gpu/drm/tegra/dsi.c:tegra_dsi_host_attach() {
    ...
    output->panel = of_drm_find_panel(device->dev.of_node);
    ...
}

If tegra_output_probe() already acquired a panel reference and stored it in
output->panel, won't this assignment overwrite the pointer without releasing
the previously acquired reference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713-drm_refcount_wiring-v2-0-d3bb61f4bd4d@redhat.com?part=3