[PATCH v2] drm/ingenic: fix bridge allocation

[PATCH v2] 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:
    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 | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 42c86f195c66b3..fb78bdef8ec9e8 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,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);
+		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,
+		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");
-- 
2.55.0

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

[sashiko-bot]

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

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