Re: [PATCH 06/11] drm/bridge: tc358767: don't create a panel_bridge

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

New issues:
- [Critical] Refcount underflow and use-after-free for bridges acquired in DPI mode.
- [High] Resource leak of the bridge reference on probe error paths.

Pre-existing issues:
- [High] Undefined behavior due to a negative bitshift when evaluating `tc->hpd_pin`.
--

--- Patch [6]: [PATCH 06/11] drm/bridge: tc358767: don't create a panel_bridge ---
commit ae8aef2f483ffa927af25a2f1ac42b0420349ca8
Author: Luca Ceresoli <[email protected]>

drm/bridge: tc358767: don't create a panel_bridge

This commit prevents creating a second panel_bridge by retrieving the
already-existing one using of_drm_get_bridge_by_endpoint() for eDP
endpoints. It updates tc_remove() to release the reference.

> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index e3a57f8228dae..2fef5d9fa361d 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -2347,24 +2347,15 @@ static int tc_probe_dpi_bridge_endpoint(struct tc_data *tc)
>  static int tc_probe_edp_bridge_endpoint(struct tc_data *tc)
>  {
>  	struct device *dev = tc->dev;
> -	struct drm_panel *panel;
> -	int ret;
> +	struct drm_bridge *bridge;
>  
>  	/* port@2 is the output port */
> -	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &panel, NULL);
> -	if (ret && ret != -ENODEV)
> -		return dev_err_probe(dev, ret,
> -				     "Could not find DSI panel or bridge\n");
> +	bridge = of_drm_get_bridge_by_endpoint(dev->of_node, 2, 0);
> +	if (IS_ERR(bridge) && PTR_ERR(bridge) != -ENODEV)
> +		return dev_err_probe(dev, PTR_ERR(bridge), "Could not find DSI bridge\n");
>  
> -	if (panel) {
> -		struct drm_bridge *panel_bridge;
> -
> -		panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> -		drm_panel_put(panel);
> -		if (IS_ERR(panel_bridge))
> -			return PTR_ERR(panel_bridge);
> -
> -		tc->panel_bridge = panel_bridge;
> +	if (!IS_ERR(bridge)) {
> +		tc->panel_bridge = bridge;
>  		tc->bridge.type = DRM_MODE_CONNECTOR_eDP;
>  	} else {
>  		tc->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>  	}

[Severity: High]
If of_drm_get_bridge_by_endpoint() successfully returns a refcounted bridge
pointer, it is stored in tc->panel_bridge. If any subsequent initialization
step in tc_probe() fails, it appears to return directly without cleaning up
the bridge reference:

tc_probe() {
    ...
    tc_probe_edp_bridge_endpoint(tc);
    ...
    tc->refclk = devm_clk_get_enabled(dev, "ref");
    if (IS_ERR(tc->refclk))
        return dev_err_probe(dev, PTR_ERR(tc->refclk),
                             "Failed to get and enable the ref clk\n");
    ...
}

Could this cause a resource leak on probe error paths?

[Severity: High]
This is a pre-existing issue, but does evaluating tc->hpd_pin here lead to a
negative bitshift later?

Due to initialization order, tc_data is zero-allocated, meaning tc->hpd_pin
is 0 when checked in tc_probe_edp_bridge_endpoint():

    if (tc->hpd_pin >= 0)
        tc->bridge.ops |= DRM_BRIDGE_OP_DETECT;

Later in tc_probe(), if the DT property is absent, tc->hpd_pin is assigned
-ENODEV (-19).

When the DRM core periodically polls the connector, it calls
tc_bridge_detect(), which executes:

    conn = val & BIT(tc->hpd_pin);

Does this lead to undefined behavior by evaluating BIT(-19)?

[ ... ]

> @@ -2609,6 +2600,7 @@ static void tc_remove(struct i2c_client *client)
>  {
>  	struct tc_data *tc = i2c_get_clientdata(client);
>  
> +	drm_bridge_put(tc->panel_bridge);
>  	drm_bridge_remove(&tc->bridge);
>  }

[Severity: Critical]
Does this unconditional drm_bridge_put() cause a double-free for bridges
acquired in DPI mode?

When operating in DPI mode, tc_probe_dpi_bridge_endpoint() is called instead,
which assigns tc->panel_bridge to a devm-managed bridge:

tc_probe_dpi_bridge_endpoint() {
    ...
    bridge = devm_drm_panel_bridge_add(dev, panel);
    ...
    tc->panel_bridge = bridge;
}

When the device unbinds, tc_remove() drops a reference it does not own. For
the devm-managed bridge, this prematurely drops its refcount to 0, freeing
the bridge immediately. Later, the devres framework will attempt to clean up
the same bridge, causing a double-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6