Re: [PATCH RFC v2 06/24] drm/display: bridge-connector: add APIs to add/remove the connector dynamically

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/display: UAF in CEC unregister due to decoupled lifetimes
- [Medium] drm/display: memory leak in drm_bridge_connector_init() error paths
--

commit 53e1c33d3e43a19a4088655af78ef18e7e7c5200
Author: Luca Ceresoli <luca.ceresoli@bootlin.com>

drm/display: bridge-connector: add APIs to add/remove the connector dynamically

This commit changes bridge connector allocation to use standard non-managed
allocations (kzalloc_obj) instead of drmm_kzalloc(), and introduces
dynamic add/remove functions, to prepare for bridge hotplug.

> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 1c222e27767d4..2a0065208fb8a 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -285,6 +285,7 @@ static void drm_bridge_connector_dynconn_destroy(struct drm_connector *connector
>  
>  	drm_connector_cleanup(connector);
>  	drm_bridge_connector_put_bridges(bridge_connector);
> +	kfree(bridge_connector);
>  }

[Severity: High]
Can this cause a use-after-free during device teardown?

During initialization, the connector may register drmm managed cleanup
actions on the parent drm_device (such as calling
drmm_connector_hdmi_cec_notifier_register() inside
drm_bridge_connector_initialize()).

If a dynamic bridge connector is hot-removed, this function will call
kfree() on bridge_connector while those drmm actions are still queued on
the drm_device.

When the parent drm_device is later torn down, could those managed actions
execute and access the already freed connector memory?

[ ... ]

> @@ -1184,7 +1185,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  	struct drm_connector *connector;
>  	int ret;
>  
> -	bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL);
> +	bridge_connector = kzalloc_obj(*bridge_connector);
>  	if (!bridge_connector)
>  		return ERR_PTR(-ENOMEM);
>  

[Severity: Medium]
Since bridge_connector is now allocated with kzalloc_obj() instead of
drmm_kzalloc(), does this leak bridge_connector if the subsequent call to
drm_bridge_connector_initialize() fails and returns an error pointer?

[ ... ]

> @@ -1199,3 +1200,24 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  	return connector;
>  }
>  EXPORT_SYMBOL_GPL(drm_bridge_connector_init);
> +
> +struct drm_connector *drm_bridge_connector_dynamic_add(struct drm_device *drm,
> +						       struct drm_encoder *encoder)
> +{
> +	struct drm_bridge_connector *bridge_connector;
> +
> +	bridge_connector = kzalloc_obj(*bridge_connector);
> +	if (!bridge_connector)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return drm_bridge_connector_initialize(bridge_connector, drm, encoder);
> +}

[Severity: Medium]
Similarly here, if drm_bridge_connector_initialize() returns an error, will
it leak the bridge_connector allocation instead of freeing it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-drm-bridge-hotplug-v2-0-8e34986dcb68@bootlin.com?part=6