[PATCH v4] drm/ingenic: fix bridge allocation

[PATCH v4] drm/ingenic: fix bridge allocation

[H. Nikolaus Schaller]

Bridge allocation API has changed and ingenic/drm was broken
leading to

[   54.997593] dw-hdmi-ingenic 10180000.hdmi: Detected HDMI \X controller v1.31a with HDCP (DWC HDMI 3D TX PHY)
[   55.491338] dw-hdmi-ingenic 10180000.hdmi: registered DesignWare HDMI I2C bus driver
[   55.899132] [drm] DRM bridge corrupted or not allocated by devm_drm_bridge_alloc()
[   55.904136] ------------[ cut here ]------------
[   55.908753] WARNING: lib/refcount.c:25 at drm_bridge_get+0x58/0x6c [drm], CPU#0: kworker/u4:2/36
[   55.917538] refcount_t: addition on 0; use-after-free.
...
[   56.354928] [<c04898b8>] drm_bridge_attach+0x80/0x208 [drm]
...

Fixes: 9347f2fbb0183b0 ("drm/bridge: add warning for bridges using neither devm_drm_bridge_alloc() nor drm_bridge_add()")
Tested-by: Waldemar Brodkorb <wbx@openadk.org> (on CI20 with HDMI)
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Cc: Waldemar Brodkorb <wbx@openadk.org>
Cc: stable@vger.kernel.org
---

Notes:
    v4: remove setting interlaced mode (would be new feature and not a fix) as suggested by paul@crapouillou.net
    
    v3: fixed a malformed diff in v2
    
    v2: removed ib->bridge->ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT as suggested by Sashiko-reviews
        https://sashiko.dev/#/patchset/400ba2fe0d4f76484e929d2efaa32f67a940163a.1787477392.git.hns@goldelico.com?part=1

 drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 42c86f195c66b3..f3c6c823314688 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -122,7 +122,7 @@ struct ingenic_drm {
 
 struct ingenic_drm_bridge {
 	struct drm_encoder encoder;
-	struct drm_bridge bridge, *next_bridge;
+	struct drm_bridge *bridge, *next_bridge;
 
 	struct drm_bus_cfg bus_cfg;
 };
@@ -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,
@@ -1314,10 +1314,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);
+		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,
+		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");
-- 
2.55.0

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

[sashiko-bot]

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 <hns@goldelico.com>

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