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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] Syntax error due to a malformed diff leaving an incomplete function call.
- [High] Use-After-Free due to lifecycle mismatch between drmm-allocated encoder and devm-allocated bridge.

Pre-existing issues:
- [High] Missing IS_ERR check on devm_drm_panel_bridge_add_typed results in potential ERR_PTR dereference.
--

--- Patch [1]: [PATCH v2] drm/ingenic: fix bridge allocation ---
commit b8ae6c3aa521f256f398e75faabccc79e7491a95
Author: H. Nikolaus Schaller <[email protected]>

drm/ingenic: fix bridge allocation

This commit modifies the Ingenic DRM driver to use the devm bridge allocation
API to fix a registration warning.

> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
[ ... ]
> @@ -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);
>  
> -		ib->bridge.funcs = &ingenic_drm_bridge_funcs;
> +		ib->bridge = devm_drm_bridge_alloc(priv->dev, struct drm_bridge,
> +						   base, &ingenic_drm_bridge_funcs);

[Severity: High]
Does this code introduce a lifecycle mismatch?

The encoder wrapper `ib` is allocated via drmm (tied to the DRM device's
lifecycle) earlier in this function:

		ib = drmm_encoder_alloc(drm, struct ingenic_drm_bridge, encoder,
					NULL, DRM_MODE_ENCODER_DPI, NULL);

However, the bridge is allocated here via devm (tied to the platform device).
If the platform device is unbound while the DRM device is still held open by
userspace, the devm infrastructure will free the bridge while the drmm-managed
encoder continues to exist.

Could this lead to a use-after-free when the DRM device is later cleaned
up and iterates over the encoder's bridge chain containing the freed bridge
pointer?

> +		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;

[Severity: High]
This is a pre-existing issue, but does `bridge` need an error check before
we assign it here?

Earlier in this function, `devm_drm_panel_bridge_add_typed()` is called:

		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. If this
unvalidated pointer is stored in `ib->next_bridge`, it will eventually be
passed to `drm_bridge_attach()`, which could attempt to dereference the
error pointer.

>  
>  		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
> +		ib->bridge->interlace_allowed = true;

[Severity: Critical]
Does this result in a syntax error?

It looks like the `ret = drm_bridge_attach(...)` call from the original code
was partially left behind, leaving an unclosed function call and a trailing
comma. Will the compiler fail when trying to parse this?

> +
> +		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);
>  		if (ret) {
>  			dev_err(dev, "Unable to attach bridge\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5db9a2ad762f1515431fff714fc7d6d93363ccb2.1787491144.git.hns@goldelico.com?part=1