Re: [PATCH 2/4] drm/bridge: display-connector: Fix I2C adapter resource leak

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free of devm-managed resources due to DRM bridge lifetime mismatch
- [Medium] Hardware state leak of `ddc_en` GPIO on probe error paths
--

commit 9c76898e592dfddd3f50beaa692cc32ae295e2c7
Author: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

drm/bridge: display-connector: Fix I2C adapter resource leak

If the probe function returns an error after getting the I2C adapter for
DDC, the reference to the adapter is never released. Fix it by releasing
it in the bridge .destroy() handler.

> diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
> index d1e74dd2e0518..eb3412ec30a07 100644
> --- a/drivers/gpu/drm/bridge/display-connector.c
> +++ b/drivers/gpu/drm/bridge/display-connector.c
> @@ -43,6 +43,13 @@ static int display_connector_attach(struct drm_bridge *bridge,
>  	return flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR ? 0 : -EINVAL;
>  }
>  
> +static void display_connector_destroy(struct drm_bridge *bridge)
> +{
> +	struct display_connector *conn = to_display_connector(bridge);
> +
> +	i2c_put_adapter(conn->bridge.ddc);
> +}
> +
>  static enum drm_connector_status display_connector_detect(struct drm_bridge *bridge)
>  {
>  	struct display_connector *conn = to_display_connector(bridge);

[ ... ]

> @@ -439,9 +447,6 @@ static void display_connector_remove(struct platform_device *pdev)
>  		regulator_disable(conn->supply);
>  
>  	drm_bridge_remove(&conn->bridge);
> -
> -	if (!IS_ERR(conn->bridge.ddc))
> -		i2c_put_adapter(conn->bridge.ddc);
>  }
>  
>  static const struct of_device_id display_connector_match[] = {

[Severity: High]
This isn't a bug introduced by this patch, but does the bridge lifetime 
mismatch with devm resources cause a use-after-free?

In display_connector_probe(), the bridge is allocated with 
devm_drm_bridge_alloc(), tying its lifetime to the DRM bridge kref:

display_connector_probe() {
    ...
    conn = devm_drm_bridge_alloc(&pdev->dev, struct display_connector, bridge,
                                 &display_connector_bridge_funcs);
    ...
}

However, devm-managed resources like the HPD GPIO and IRQ are tied to the 
platform device unbind. If the device is unbound via sysfs while the DRM 
core still holds a reference to the bridge, devres will free the GPIO and 
IRQ resources.

If the DRM core later invokes callbacks like detect or hpd_disable, would it 
dereference the freed GPIO or attempt to disable the freed IRQ?

display_connector_detect() {
    ...
    if (gpiod_get_value_cansleep(conn->hpd_gpio))
    ...
}

display_connector_hpd_disable() {
    ...
    disable_irq(conn->hpd_irq);
    ...
}

[Severity: Medium]
This is a pre-existing issue, but is there a hardware state leak on the 
probe error path?

In display_connector_probe(), the DDC enable GPIO is acquired and 
immediately asserted high:

display_connector_probe() {
    ...
    if (type == DRM_MODE_CONNECTOR_HDMIA) {
        ...
        conn->ddc_en = devm_gpiod_get_optional(&pdev->dev, "ddc-en",
                                               GPIOD_OUT_HIGH);

        if (IS_ERR(conn->ddc_en)) {
            ...
        }

        ret = display_connector_get_supply(pdev, conn, "hdmi-pwr");
        if (ret < 0)
            return dev_err_probe(...);
    ...
}

If a subsequent step like display_connector_get_supply() or 
regulator_enable() fails, the function returns an error. While devres will 
free the GPIO descriptor, it doesn't revert the physical pin state.

Unlike display_connector_remove() which explicitly sets the GPIO to 0, does 
this error path leave the DDC bus permanently enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705213542.28987-1-laurent.pinchart+renesas@ideasonboard.com?part=2