(Response created with ChatGPT analyzing patch, drm subsystem, memory management and Sahsiko comments)
> Am 23.08.2026 um 11:45 schrieb [email protected]: > > 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. Thanks for the review. - The devm_drm_bridge_alloc() lifecycle concern appears not to apply here. The DRM bridge is refcounted, and drm_bridge_attach() takes a reference, so the bridge is not simply freed when the device-managed reference is released. The existing Ingenic component unbind path also unregisters and shuts down the DRM device, allowing the bridge/encoder chain to be cleaned up in the normal DRM lifecycle. - I agree with the DRM_BRIDGE_OP_EDID / DRM_BRIDGE_OP_DETECT finding; these flags are not backed by callbacks in ingenic_drm_bridge_funcs and will be removed in v2. - The devm_drm_panel_bridge_add_typed() error handling and the ib->bus_cfg modification in atomic_check() are pre-existing issues and are unrelated to this patch. BR, Nikolaus > > > >> 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
