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 <[email protected]> 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/[email protected]?part=3
