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

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

Pre-existing issues:
- [High] Missing error check on devm_drm_panel_bridge_add_typed() leads to an ERR_PTR dereference.
- [High] The driver uses the deprecated drm_of_find_panel_or_bridge() API which returns a raw, non-refcounted pointer to a bridge, creating a Use-After-Free risk.
--

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

drm/ingenic: fix bridge allocation

This commit fixes the bridge allocation API in ingenic/drm by allocating
a stable driver-specific container struct that embeds a struct drm_bridge.
It also adjusts the object lifecycle by calling drm_bridge_add before
drm_bridge_attach.

> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 738a80b2550ff..4098192c642bd 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c

[ ... ]

> @@ -1316,10 +1321,27 @@ 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;
> +		bridge_alloc = devm_drm_bridge_alloc(priv->dev,
> +						     struct ingenic_drm_bridge_alloc,
> +						     bridge,

[Severity: High]
This is a pre-existing issue, but since we are working with the bridge
pointer here, should we consider migrating away from the deprecated
drm_of_find_panel_or_bridge() API called earlier in this function?

Looking at ingenic_drm_bind(), it calls drm_of_find_panel_or_bridge() which
returns a non-refcounted bridge. If an external bridge driver unbinds
concurrently, the bridge memory could be freed before we finish executing
ingenic_drm_bind(), leaving us passing a dangling pointer into
drm_bridge_attach().

[Severity: High]
This isn't a bug introduced by this patch, but does the bridge pointer need
an IS_ERR() check before we use it?

A few lines above in ingenic_drm_bind(), devm_drm_panel_bridge_add_typed()
is called. If it fails due to memory allocation issues, it returns an
ERR_PTR. We blindly assign that to bridge, which eventually gets passed to
drm_bridge_attach(). 

Because drm_bridge_attach() only checks if (!bridge), the ERR_PTR bypasses
this check and gets dereferenced when accessing bridge->container, leading
to a crash.

> +						     &ingenic_drm_bridge_funcs);
> +		if (IS_ERR(bridge_alloc)) {
> +			ret = PTR_ERR(bridge_alloc);
> +			goto err_drvdata;
> +		}

[ ... ]

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