Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [High] The patch introduces a lifecycle mismatch and memory leak by using `devm_` allocations for a DRM bridge inside a component bind function, while attaching it to a `drmm_`-managed encoder. - [High] Falsely claiming DRM_BRIDGE_OP_EDID and DRM_BRIDGE_OP_DETECT capabilities on a bridge that lacks the corresponding callback implementations causes a NULL pointer dereference. Pre-existing issues: - [High] Unhandled error pointer from devm_drm_panel_bridge_add_typed() leads to invalid memory dereference. - [High] atomic_check phase corrupts global driver state by directly modifying ib->bus_cfg. -- --- Patch [1]: [PATCH] drm/ingenic: fix bridge allocation --- commit caf3be8c5a53047a7d5ddaf2c5ee3db764a2f7fb Author: H. Nikolaus Schaller <[email protected]> drm/ingenic: fix bridge allocation Bridge allocation API has changed and ingenic/drm was broken leading to a warning. This patch replaces the embedded drm_bridge structure with a pointer and uses devm_drm_bridge_alloc() to resolve the issue. > diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > index 738a80b2550ff..91fe0ee7ae9ad 100644 > --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c > @@ -802,7 +802,7 @@ static int ingenic_drm_bridge_attach(struct drm_bridge > *bridge, > struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(encoder); > > return drm_bridge_attach(encoder, ib->next_bridge, > - &ib->bridge, flags); > + bridge, flags); > } > > static int ingenic_drm_bridge_atomic_check(struct drm_bridge *bridge, > struct drm_bridge_state > *bridge_state, > struct drm_crtc_state *crtc_state, > struct drm_connector_state > *conn_state) > { > struct drm_display_mode *mode = &crtc_state->adjusted_mode; > struct ingenic_drm_bridge *ib = to_ingenic_drm_bridge(bridge->encoder); > > ib->bus_cfg = bridge_state->output_bus_cfg; [Severity: High] This is a pre-existing issue, but does modifying ib->bus_cfg here break the atomic contract? Since DRM atomic_check can be called for TEST_ONLY commits that are meant to be discarded, directly overwriting the global driver state here corrupts the display configuration if a real commit later relies on this modified state. [ ... ] > @@ -1316,10 +1316,26 @@ static int ingenic_drm_bind(struct device *dev, bool > has_components) > > drm_encoder_helper_add(encoder, > &ingenic_drm_encoder_helper_funcs); > [Severity: High] This is a pre-existing issue, but earlier in this same ingenic_drm_bind() function, what happens if the panel bridge allocation fails? if (panel) { bridge = devm_drm_panel_bridge_add_typed(dev, panel, DRM_MODE_CONNECTOR_DPI); drm_panel_put(panel); } If devm_drm_panel_bridge_add_typed() fails, it returns an ERR_PTR(). This unvalidated error pointer is saved into bridge, and eventually passed into drm_bridge_attach() as ib->next_bridge, where it will be dereferenced and panic the kernel. Should we check this with IS_ERR() here? > - ib->bridge.funcs = &ingenic_drm_bridge_funcs; > + ib->bridge = devm_drm_bridge_alloc(priv->dev, struct drm_bridge, > + base, > &ingenic_drm_bridge_funcs); > + if (IS_ERR(ib->bridge)) { > + ret = PTR_ERR(ib->bridge); > + goto err_drvdata; > + } > + > + ib->bridge->of_node = priv->dev->of_node; > ib->next_bridge = bridge; > > - ret = drm_bridge_attach(encoder, &ib->bridge, NULL, > + ib->bridge->ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT; [Severity: High] Does this bridge actually implement these operations? Setting DRM_BRIDGE_OP_EDID and DRM_BRIDGE_OP_DETECT advertises these capabilities to the DRM core. However, the ingenic_drm_bridge_funcs structure lacks the corresponding .detect, .edid_read, or .get_edid callbacks. When the core initializes the bridge connector and queries its state, it will unconditionally call these missing functions, leading to a NULL pointer dereference. > + ib->bridge->interlace_allowed = true; > + > + ret = devm_drm_bridge_add(priv->dev, ib->bridge); > + if (ret) { > + dev_err(dev, "Failed to register DRM bridge: %d\n", > ret); > + goto err_drvdata; > + } > + > + ret = drm_bridge_attach(encoder, ib->bridge, NULL, > DRM_BRIDGE_ATTACH_NO_CONNECTOR); [Severity: High] Can these devm_ allocations cause a use-after-free or a memory leak on unbind? Using devm_drm_bridge_alloc() and devm_drm_bridge_add() inside a component bind function ties the bridge's lifecycle to device removal, not component unbind. This means each bind/unbind cycle leaks memory and leaves stale bridges in the global DRM list. Additionally, the parent encoder is allocated via drmm_encoder_alloc(), tying its lifecycle to the DRM device, which can outlive the platform device if userspace holds an open file descriptor. When the platform device is removed, devres frees the bridge early, creating a UAF hazard when the DRM device later attempts to clean up its encoder bridge chains. > if (ret) { > dev_err(dev, "Unable to attach bridge\n"); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/400ba2fe0d4f76484e929d2efaa32f67a940163a.1787477392.git....@goldelico.com?part=1
