Re: [PATCH RFC v2 12/24] drm/bridge: shutdown and cleanup on bridge unplug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] drm/bridge: UAF from dangling encoder pointer in drm_bridge_remove()
- [Critical] drm/bridge: list corruption in drm_encoder_cleanup_from() on concurrent unplug
- [Critical] drm/bridge: data race on privobj_list during hot-unplug detachment
- [High] drm/bridge: overbroad shutdown via drm_atomic_shutdown() on unplug
- [High] drm/bridge: lockless TOCTOU race on encoder pointer in drm_bridge_remove()
--

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

drm/bridge: shutdown and cleanup on bridge unplug

This commit adds logic to shut down the pipeline and detach the removed
bridge (and subsequent ones) from the encoder chain when a bridge is
hot-unplugged. It modifies drm_bridge_remove() to invoke
drm_atomic_shutdown() and drm_encoder_cleanup_from() if the bridge is
attached to an encoder.

> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 3cf84f2da33ba..468ceed36eb9c 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -507,9 +507,17 @@ EXPORT_SYMBOL(devm_drm_bridge_add);
>   * it won't be found by users via of_drm_find_and_get_bridge(), and add it
>   * to the lingering bridge list, to keep track of it until its allocated
>   * memory is eventually freed.
> + *
> + * If the bridge was attached, also shutdown CRTCs and detach this bridge
> + * and the following ones.
>   */
>  void drm_bridge_remove(struct drm_bridge *bridge)
>  {
> +	if (bridge->encoder) {

[Severity: Critical]
Is it possible for bridge->encoder to be a dangling pointer here?

When a display driver unbinds, drm_encoder_cleanup() calls drm_bridge_detach().
The detachment logic clears bridge->dev but fails to clear bridge->encoder:

drivers/gpu/drm/drm_bridge.c:drm_bridge_detach() {
	...
	bridge->dev = NULL;
	drm_bridge_put(bridge);
}

If the encoder memory is then freed, and the bridge driver later unbinds,
drm_bridge_remove() will observe the dangling bridge->encoder pointer and pass
it to drm_encoder_cleanup_from(), which immediately attempts to acquire the
freed encoder's bridge_chain_mutex.


[Severity: High]
Is this lockless read of bridge->encoder susceptible to a TOCTOU race?

If this races with drm_bridge_attach(), it can observe the newly assigned
bridge->encoder before drm_bridge_attach() links the bridge into the encoder
chain under bridge_chain_mutex.

drm_bridge_remove() would then call drm_encoder_cleanup_from(), which acquires
the mutex, sees the bridge is not yet in the list, and skips detachment. The
bridge is then linked into the chain by drm_bridge_attach(), while
drm_bridge_remove() proceeds to destroy bridge->hpd_mutex and drops its
refcount.

> +		drm_atomic_shutdown(bridge->dev);

[Severity: High]
Does this unconditionally disable all display pipelines on the DRM device during
a single bridge unplug?

Looking at drm_atomic_shutdown(), it invokes drm_atomic_disable_all() on the
entire device. For systems with multiple independent displays (e.g., an internal
eDP panel and an external hot-pluggable HDMI bridge), couldn't this blank the
internal display when the external one is unplugged?

> +		drm_encoder_cleanup_from(bridge->encoder, bridge);

[Severity: Critical]
Can this lead to list corruption on concurrent unplugs?

In drm_encoder_cleanup_from(), the code evaluates !list_empty(&bridge->chain_node)
to check if a bridge is attached:

drivers/gpu/drm/drm_encoder.c:drm_encoder_cleanup_from() {
	...
	if (!list_empty(&bridge->chain_node))
		list_for_each_entry_safe_from(bridge, next,
					      &encoder->bridge_chain, chain_node)
			list_move_tail(&bridge->chain_node, &tmplist);
	...
}

If Thread A removes an upstream bridge, it moves downstream bridges to its local
stack tmplist. If Thread B concurrently removes one of these downstream bridges,
it acquires the mutex and evaluates !list_empty(). Since the node is in Thread
A's tmplist, it evaluates to true.

Thread B then executes list_for_each_entry_safe_from(), which expects to terminate
at &encoder->bridge_chain. Because the node is actually on Thread A's stack
list, the loop never reaches the encoder's list head, wrapping around the stack
LIST_HEAD and interpreting arbitrary memory as struct drm_bridge.


[Severity: Critical]
Does detaching a bridge here introduce a data race on privobj_list?

drm_encoder_cleanup_from() calls drm_bridge_detach(), which calls
drm_atomic_private_obj_fini(). This executes list_del(&obj->head) to remove the
bridge from dev->mode_config.privobj_list.

However, privobj_list is documented as invariant over the lifetime of the device
and doesn't need locks:

include/drm/drm_mode_config.h:struct drm_mode_config {
	...
	/**
	 * @privobj_list:
	 *
	 * List of private objects linked with &drm_private_obj.head. This is
	 * invariant over the lifetime of a device and hence doesn't need any
	 * locks.
	 */
	...
}

Modifying it dynamically on hot-unplug without protective locks (like RCU)
introduces a data race with concurrent atomic operations or mode settings that
iterate the list locklessly.

> +	}
> +
>  	mutex_lock(&bridge_lock);
>  	list_move_tail(&bridge->list, &bridge_lingering_list);
>  	mutex_unlock(&bridge_lock);

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