Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues: - [Medium] Memory leak of DRM panel reference when multiple endpoints define panels in pl111_modeset_init(). Pre-existing issues: - [High] Uninitialized `tmp_bridge` can be used and dereferenced if `drm_of_find_panel_or_bridge` returns `-ENODEV`. - [High] Passing an `ERR_PTR` to `drm_panel_bridge_remove()` in `tve200_modeset_init()` error path causes a kernel panic. - [High] Missing `IS_ERR()` check on bridge returned by `devm_drm_panel_bridge_add_typed()` leads to `ERR_PTR` dereference in `ingenic_drm_bind()`. - [High] Overwriting `bridge->funcs` corrupts external bridges and defeats `drm_panel_bridge_remove()` cleanup in `rockchip_lvds_bind()`. -- commit ffb6fad5e4cf513ed7c3fe05c38be4669593628f Author: Albert Esteve <[email protected]> drm: release panel reference after panel bridge creation This commit updates drivers to release the panel reference after wrapping it in a panel bridge, as the panel bridge now holds its own reference. It also drops unused references on error paths. > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index 42c86f195c66b..1887e01d29701 100644 > --- 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 does this missing error check lead to a panic? devm_drm_panel_bridge_add_typed() returns an ERR_PTR on failure. By not checking IS_ERR(), the error pointer is stored and later passed to drm_bridge_attach(), which will try to dereference it since it only checks for NULL. > > ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder, > NULL, DRM_MODE_ENCODER_DPI, NULL); [ ... ] > diff --git a/drivers/gpu/drm/pl111/pl111_drv.c > b/drivers/gpu/drm/pl111/pl111_drv.c > index ac7b1d12a0f59..8ec659b3c08eb 100644 > --- a/drivers/gpu/drm/pl111/pl111_drv.c > +++ b/drivers/gpu/drm/pl111/pl111_drv.c > @@ -145,6 +145,7 @@ static int pl111_modeset_init(struct drm_device *dev) > if (panel) { > bridge = drm_panel_bridge_add_typed(panel, > DRM_MODE_CONNECTOR_Unknown); > + drm_panel_put(panel); [Severity: Medium] Could this leak panel references if multiple endpoints define panels? Earlier in pl111_modeset_init(), it iterates through endpoints: for_each_endpoint_of_node(np, remote) { struct drm_panel *tmp_panel; ... if (tmp_panel) { ... panel = tmp_panel; } } If the loop iterates multiple times and finds more than one panel, the panel variable is overwritten without calling drm_panel_put() on the previously found panel. Since this patch makes the panels return counted references, does this introduce a leak for the overwritten panels? [Severity: High] This is a pre-existing issue, but could an uninitialized tmp_bridge cause a crash here? In the same loop: for_each_endpoint_of_node(np, remote) { struct drm_panel *tmp_panel; struct drm_bridge *tmp_bridge; ret = drm_of_find_panel_or_bridge(..., &tmp_panel, &tmp_bridge); If drm_of_find_panel_or_bridge() returns -ENODEV, the tmp_bridge parameter is untouched. The code then evaluates if (tmp_bridge), which checks uninitialized stack memory. If it evaluates to true, the garbage pointer is assigned to the bridge variable and eventually dereferenced. > if (IS_ERR(bridge)) { > ret = PTR_ERR(bridge); > goto finish; [ ... ] > diff --git a/drivers/gpu/drm/rockchip/rockchip_lvds.c > b/drivers/gpu/drm/rockchip/rockchip_lvds.c > index 7a0c4fa29f2f0..f754445d2631b 100644 > --- 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); [Severity: High] This is a pre-existing issue, but does overwriting the bridge functions cause memory leaks or panel failures? Later in rockchip_lvds_bind(), the code does: lvds->bridge->funcs = &rockchip_lvds_bridge_funcs; Since lvds->bridge points to an external bridge or panel bridge created by drm_panel_bridge_add_typed(), overwriting its function pointers modifies an object owned by another driver. Does this permanently disable the bridge's original operations? Additionally, would drm_panel_bridge_remove() in the error path skip freeing the bridge since it checks if the funcs pointer matches the default panel bridge functions? > goto err_free_encoder; [ ... ] > diff --git a/drivers/gpu/drm/tve200/tve200_drv.c > b/drivers/gpu/drm/tve200/tve200_drv.c > index 562f3f11812a3..f858c58ccb994 100644 > --- 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 can the error path cause a panic? If drm_panel_bridge_add_typed() fails and returns an ERR_PTR, we jump to out_bridge: out_bridge: if (panel) drm_panel_bridge_remove(bridge); Since panel is not NULL, drm_panel_bridge_remove(bridge) is called with the ERR_PTR. The removal function checks if (!bridge) which is false for an ERR_PTR, and then immediately dereferences bridge->funcs. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
