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

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a missing IS_ERR() check on the return value of devm_drm_panel_bridge_add_typed() which leads to an error pointer dereference.
--

--- Patch [1]: [PATCH v3] drm/ingenic: fix bridge allocation ---
commit 4187db84355de6d4a1d09fa81233bcc5ab620c4b
Author: H. Nikolaus Schaller <[email protected]>
Subject: drm/ingenic: fix bridge allocation

This patch updates the ingenic/drm driver to use the new
devm_drm_bridge_alloc() API for bridge allocation. It fixes an issue where
the bridge was corrupted or not allocated properly under the new API,
causing use-after-free warnings.

> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 738a80b2550ff..d1321a9c0f0b0 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
[ ... ]
> @@ -1316,10 +1316,25 @@ 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);
> +		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 isn't a bug introduced by this patch, but if
devm_drm_panel_bridge_add_typed() fails earlier in ingenic_drm_bind(), it
returns an ERR_PTR.

Does this assignment store that unvalidated error pointer into
ib->next_bridge without an IS_ERR() check?

When the bridge attach callback executes:

static int ingenic_drm_bridge_attach(...)
{
    ...
    return drm_bridge_attach(encoder, ib->next_bridge,
                             bridge, flags);
}

Would this pass the ERR_PTR as the bridge argument to drm_bridge_attach(),
which bypasses the NULL check and dereferences it?

int drm_bridge_attach(..., struct drm_bridge *bridge, ...)
{
    if (!encoder || !bridge)
        return -EINVAL;

    if (!bridge->container)
        ...
}

Could this lead to a kernel panic from an invalid memory access?

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

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