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

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

New issues:
- [High] devm_drm_bridge_add() is called inside a component bind function, causing bridge list pollution and memory leaks on bind failure or component unbind.
- [Medium] Invalid usage of the `devm_drm_bridge_alloc` macro by passing `base` as the member argument.

Pre-existing issues:
- [High] Missing `IS_ERR` check after calling `devm_drm_panel_bridge_add_typed()`, leading to an `ERR_PTR` dereference in `drm_bridge_attach()`.
--

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

drm/ingenic: fix bridge allocation

Bridge allocation API has changed and ingenic/drm was broken
leading to use-after-free errors.

> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 738a80b2550ff..16fefdac8ce3d 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
[ ... ]
> @@ -1316,10 +1316,23 @@ 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: Medium]
Does passing base as the member argument safely allocate the bridge?

The devm_drm_bridge_alloc() macro is designed to allocate a driver-specific
container struct that embeds a struct drm_bridge, expecting member to be
the embedded bridge. It expands to:

__devm_drm_bridge_alloc(dev, sizeof(type), offsetof(type, member), funcs)

Passing struct drm_bridge as the type and base as the member relies on
base remaining at offset 0. If the struct layout changes and base moves,
the pointer offset calculation will write out of bounds since the internal
logic only allocates sizeof(struct drm_bridge) bytes.

Could this be updated to allocate a proper embedding container instead?

> +		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 IS_ERR check before
being assigned here?

Earlier in ingenic_drm_bind(), devm_drm_panel_bridge_add_typed() is called
which can return an error pointer if memory allocation fails:

    if (panel) {
        bridge = devm_drm_panel_bridge_add_typed(...)
        drm_panel_put(panel);
    }

If this allocation fails, bridge contains an error pointer that is blindly
assigned to ib->next_bridge. When ingenic_drm_bridge_attach() later passes
this to drm_bridge_attach(), it isn't protected by a NULL check and the error
pointer will be dereferenced, potentially causing a panic.

> -		ret = drm_bridge_attach(encoder, &ib->bridge, NULL,
> +		ret = devm_drm_bridge_add(priv->dev, ib->bridge);

[Severity: High]
Is it safe to use devm_drm_bridge_add() inside a component bind function?

When tied to priv->dev (the master platform device), the devm cleanup
actions are only triggered when the master device is removed.

If ingenic_drm_bind() fails in a subsequent initialization step and defers,
or if the component is manually unbound via sysfs, the master device remains
bound.

This would leave the bridge memory allocated and the bridge itself in the
global DRM bridge_list. Upon retry or rebind, a new bridge will be
allocated and added, leaking memory and polluting the global list with
ghost entries.

> +		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/1630a544a26fac0b87187885374ebe59fc92df3b.1787575203.git.hns@goldelico.com?part=1