Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues: - [High] Calling `drm_panel_bridge_remove` with an `ERR_PTR` on the error path causes a kernel oops. - [High] Missing `IS_ERR` check for `devm_drm_panel_bridge_add_typed` return value leads to `ERR_PTR` dereference in `drm_bridge_attach`. - [High] Error handling failures in `rockchip_rgb_init()` lead to a dangling encoder in DRM core lists and a leaked panel bridge. - [Medium] Hijacking `bridge->funcs` in `rockchip_lvds_bind` breaks panel bridge cleanup, causing memory and reference leaks. - [Medium] The `mcde_drv` driver leaks the panel bridge and panel reference on error paths and during teardown. -- --- Patch [5]: [PATCH v2 5/5] drm: release panel reference after panel bridge creation --- commit 03ed59fea0a81ee259951270dc961716491dc0bd Author: Albert Esteve <[email protected]> Subject: drm: release panel reference after panel bridge creation This commit updates a second batch of drivers calling of_drm_find_panel() or drm_of_find_panel_or_bridge() to release the lookup reference after wrapping the panel in a bridge. It also handles paths where a panel is found but cannot be used by dropping the reference immediately. > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > @@ -1297,9 +1297,11 @@ static int ingenic_drm_bind(struct device *dev, bool > has_components) > goto err_drvdata; > } > > - if (panel) > + if (panel) { > bridge = devm_drm_panel_bridge_add_typed(dev, panel, > > DRM_MODE_CONNECTOR_DPI); > + drm_panel_put(panel); > + } > [Severity: High] This is a pre-existing issue, but I noticed devm_drm_panel_bridge_add_typed() can return an ERR_PTR on failure. Since there is no IS_ERR() check here in ingenic_drm_bind(), the error pointer gets assigned to ib->next_bridge a few lines later and is passed indirectly to drm_bridge_attach(). Could this cause the DRM core to dereference the error pointer (e.g., accessing bridge->dev) and crash during initialization? [ ... ] > diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c > --- a/drivers/gpu/drm/mcde/mcde_drv.c > +++ b/drivers/gpu/drm/mcde/mcde_drv.c > @@ -153,6 +153,7 @@ static int mcde_modeset_init(struct drm_device *drm) > if (panel) { > bridge = drm_panel_bridge_add_typed(panel, > DRM_MODE_CONNECTOR_DPI); > + drm_panel_put(panel); > if (IS_ERR(bridge)) { > dev_err(drm->dev, > "Could not connect panel bridge\n"); [Severity: Medium] This isn't a bug introduced by this patch, but it looks like the unmanaged bridge created here in mcde_modeset_init() is never removed. If mcde_modeset_init() fails later (for example, if drm_vblank_init() fails), or during standard driver teardown, does this permanently leak the panel bridge and the panel reference? [ ... ] > diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c > b/drivers/gpu/drm/rockchip/rockchip_lvds.c > --- a/drivers/gpu/drm/rockchip/rockchip_lvds.c > +++ b/drivers/gpu/drm/rockchip/rockchip_lvds.c > @@ -605,6 +605,7 @@ static int rockchip_lvds_bind(struct device *dev, struct > device *master, > > if (lvds->panel) { > lvds->bridge = drm_panel_bridge_add_typed(lvds->panel, > DRM_MODE_CONNECTOR_LVDS); > + drm_panel_put(lvds->panel); > if (IS_ERR(lvds->bridge)) { > ret = PTR_ERR(lvds->bridge); > goto err_free_encoder; [Severity: Medium] This is a pre-existing issue, but right after this check in rockchip_lvds_bind(), the driver overwrites the bridge functions: lvds->bridge->funcs = &rockchip_lvds_bridge_funcs; If drm_panel_bridge_remove() is called later in an error path or on unbind, it checks the bridge identity via drm_bridge_is_panel(), which specifically looks for bridge->funcs == &panel_bridge_bridge_funcs. Does overwriting the function pointers here cause the removal to fail silently and leak the panel bridge? [ ... ] > diff --git a/drivers/gpu/drm/rockchip/rockchip_rgb.c > b/drivers/gpu/drm/rockchip/rockchip_rgb.c > --- a/drivers/gpu/drm/rockchip/rockchip_rgb.c > +++ b/drivers/gpu/drm/rockchip/rockchip_rgb.c > @@ -139,6 +139,7 @@ struct rockchip_rgb *rockchip_rgb_init(struct device *dev, > if (panel) { > bridge = drm_panel_bridge_add_typed(panel, > DRM_MODE_CONNECTOR_LVDS); > + drm_panel_put(panel); > if (IS_ERR(bridge)) > return ERR_CAST(bridge); > } [Severity: High] This isn't a bug introduced by this patch, but returning ERR_CAST(bridge) directly here skips the err_free_encoder label. Since the embedding rgb struct is allocated with devm_kzalloc(), does this leave a dangling encoder in the DRM core lists when the memory is freed, potentially leading to a use-after-free? Additionally, in the err_free_encoder error path later in rockchip_rgb_init(), drm_encoder_cleanup() is called but drm_panel_bridge_remove() is skipped. Does that path permanently leak the unmanaged panel bridge? [ ... ] > diff --git a/drivers/gpu/drm/tve200/tve200_drv.c > b/drivers/gpu/drm/tve200/tve200_drv.c > --- a/drivers/gpu/drm/tve200/tve200_drv.c > +++ b/drivers/gpu/drm/tve200/tve200_drv.c > @@ -84,6 +84,7 @@ static int tve200_modeset_init(struct drm_device *dev) > if (panel) { > bridge = drm_panel_bridge_add_typed(panel, > DRM_MODE_CONNECTOR_Unknown); > + drm_panel_put(panel); > if (IS_ERR(bridge)) { > ret = PTR_ERR(bridge); > goto out_bridge; > } [Severity: High] This is a pre-existing issue, but when drm_panel_bridge_add_typed() fails here in tve200_modeset_init(), the code jumps to out_bridge with an error pointer in the bridge variable. The out_bridge label handles cleanup like this: out_bridge: if (panel) drm_panel_bridge_remove(bridge); Since drm_panel_bridge_remove() calls drm_bridge_is_panel() which dereferences bridge->funcs, won't this cause an immediate kernel panic when it tries to dereference the error pointer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
