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

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