Re: [PATCH] drm/ingenic: fix bridge allocation

(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.hns@goldelico.com?part=1