Re: [PATCH 36/37] drm/display: bridge-connector: handle bridge hotplug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] `drmm_bridge_connector_init()` returns NULL instead of the allocated pointer on success.
- [High] Unchecked execution of the optional `attach` callback causes NULL pointer dereference.
- [High] TOCTOU race condition in `dynconn` creation leads to memory leaks and duplicate connector registrations.
- [High] Dynamic connector destruction on hot-unplug leaks `drmm` actions, leading to a Use-After-Free.
- [High] Lockless traversal of `encoder->bridge_chain` risks list corruption and Use-After-Free.
--

commit 31a28cb0ac7316fddfb3b8148c8e28f4da0bce4f
Author: Luca Ceresoli <luca.ceresoli@bootlin.com>

drm/display: bridge-connector: handle bridge hotplug

This commit updates the DRM bridge connector infrastructure to handle bridge
hot-plugging by creating and destroying the DRM connector dynamically. It
introduces the drmm_bridge_connector_init() API, which registers a notifier
to react to attach/detach events and manage the connector lifecycle.

> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index b83da529cc7af..2ce6c04e2fc7a 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c

[ ... ]

> @@ -1037,6 +1042,19 @@ static int drm_bridge_connector_init_hdmi_audio_cec(struct drm_bridge_connector_
>  	return 0;
>  }
>  
> +static bool drm_bridge_connector_pipeline_is_complete(struct drm_bridge_connector *bridge_connector)
> +{
> +	struct drm_bridge *last_bridge __free(drm_bridge_put) =
> +		drm_bridge_chain_get_last_bridge(bridge_connector->encoder);

Is it safe to call drm_bridge_chain_get_last_bridge() here without holding
bridge_connector->encoder->bridge_chain_mutex? It seems this can be called
asynchronously from hotplug event notifiers, potentially traversing the
list while it is being modified.

> +
> +	if (!last_bridge || !drm_bridge_is_tail(last_bridge)) {
> +		drm_dbg_driver(bridge_connector->drm, "pipeline not (yet) fully connected");
> +		return false;
> +	}
> +
> +	return true;
> +}

[ ... ]

> @@ -1153,6 +1171,79 @@ static int drm_bridge_connector_add_connector(struct drm_bridge_connector *bridg
>  	return ret;
>  }
>  
> +/*
> + * Propagate the attach chain and possibly add a drm_connector after a new
> + * drm_bridge is hot-plugged.
> + *
> + * The connector is added only if the pipeline is now complete. This could
> + * not be the case for various reasons:
> + *
> + * - the new bridge is just unrelated to our encoder
> + * - the new bridge is not be the next one in the pipeline
> + * - the new bridge is the next in the pipeline but the pipeline is not yet
> + *   complete
> + *
> + * All these cases are normal, not an error.
> + */
> +static void drm_bridge_connector_try_complete(struct drm_bridge_connector *bridge_connector)
> +{
> +	int err;
> +
> +	/*
> +	 * drm_connector already present, the new bridge must be for
> +	 * another card
> +	 */
> +	if (bridge_connector->dynconn)

Could there be a race condition here? If multiple hotplug events are processed
concurrently, two threads could observe dynconn as NULL here outside the
lock, and both proceed to allocate and assign it in
drm_bridge_connector_add_connector().

Would it be better to re-verify bridge_connector->dynconn after acquiring
dynconn_mutex inside drm_bridge_connector_add_connector()?

> +		return;
> +
> +	/* Propagate the attach call chain to newly hotplugged bridge(s) */
> +	struct drm_bridge *last_bridge __free(drm_bridge_put) =
> +		drm_bridge_chain_get_last_bridge(bridge_connector->encoder);

As mentioned above, should this list traversal be protected by
bridge_connector->encoder->bridge_chain_mutex?

> +
> +	/* Encoder chain empty? */
> +	if (!last_bridge)
> +		return;
> +
> +	err = last_bridge->funcs->attach(last_bridge, bridge_connector->encoder,
> +					 DRM_BRIDGE_ATTACH_NO_CONNECTOR);

What happens if the last bridge does not implement the attach callback?

Since attach is optional in struct drm_bridge_funcs, should there be a check
for last_bridge->funcs->attach before calling it to avoid a NULL pointer
dereference?

> +	if (err)
> +		return;
> +
> +	/* Add the connector if the pipeline is now complete */
> +	if (drm_bridge_connector_pipeline_is_complete(bridge_connector))
> +		drm_bridge_connector_add_connector(bridge_connector);
> +}
> +
> +static int drm_bridge_connector_handle_event(struct notifier_block *nb,
> +					     unsigned long event, void *data)
> +{
> +	struct drm_bridge_connector *bridge_connector =
> +		container_of(nb, struct drm_bridge_connector, drm_event_nb);
> +
> +	switch (event) {
> +	case DRM_MIPI_DSI_ATTACHED:
> +		/* One or more bridges hot-plugged, try adding the drm_connector */
> +		drm_bridge_connector_try_complete(bridge_connector);
> +		break;
> +	case DRM_BRIDGE_DETACHED:
> +	{
> +		/*
> +		 * A bridge was unplugged, remove the drm_connector if it's
> +		 * part of the same pipeline
> +		 */
> +		struct drm_bridge *bridge = (struct drm_bridge *)data;
> +
> +		if (bridge_connector->dynconn &&
> +		    bridge->encoder == bridge_connector->encoder)
> +			drm_bridge_connector_dynconn_release(bridge_connector);

Does drm_bridge_connector_dynconn_release() safely clean up the drmm actions
registered in drm_bridge_connector_init_hdmi_audio_cec()?

Since drmm_connector_hdmi_cec_register() and similar tie cleanup actions to
the drm_device, releasing the connector here and freeing the dynconn
structure might leave those actions active, potentially causing a
use-after-free when the device is eventually destroyed.

> +		break;
> +	}
> +	default:
> +	}
> +
> +	return NOTIFY_DONE;
> +}

[ ... ]

> +struct drm_bridge_connector *drmm_bridge_connector_init(struct drm_device *drm,
> +							struct drm_encoder *encoder)
> +{
> +	struct drm_bridge_connector *bridge_connector;
> +	int ret;
> +
> +	bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL);
> +	if (!bridge_connector)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mutex_init(&bridge_connector->dynconn_mutex);
> +	bridge_connector->drm = drm;
> +	bridge_connector->encoder = encoder;
> +	bridge_connector->drm_event_nb.notifier_call = drm_bridge_connector_handle_event;
> +
> +	if (drm_bridge_connector_pipeline_is_complete(bridge_connector)) {
> +		ret = drm_bridge_connector_add_connector(bridge_connector);
> +		if (ret)
> +			return ERR_PTR(ret);
> +	}
> +
> +	ret = drmm_add_action_or_reset(drm, drm_bridge_connector_fini, bridge_connector);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = drm_event_notifier_register(&bridge_connector->drm_event_nb);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = drmm_add_action_or_reset(drm, drm_bridge_connector_notifier_unregister,
> +				       &bridge_connector->drm_event_nb);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return 0;

Should this return the bridge_connector pointer instead of 0? Callers using
IS_ERR() will treat 0 (NULL) as success and potentially dereference it.

> +}
> +EXPORT_SYMBOL_GPL(drmm_bridge_connector_init);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-drm-bridge-hotplug-v1-0-45e2bdb3dfb4@bootlin.com?part=36