Re: [PATCH 25/37] drm/bridge: shutdown and cleanup on bridge unplug

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Use-After-Free (UAF) due to dangling `bridge->encoder` pointer
- [Critical] Stack corruption via cross-thread list iteration in `drm_encoder_cleanup_from()`
- [High] Unintended system-wide display shutdown (Logic Error)
--

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

drm/bridge: shutdown and cleanup on bridge unplug

This commit aims to handle DRM bridge hot-unplugging by shutting down the
pipeline and detaching the removed bridge and subsequent ones, rather than
tearing down the entire card.

> With the upcoming support for DRM bridge hot(un)plugging, bridges can be
> removed at any time withotu tearing down the entire card.

The commit message states the patch avoids tearing down the entire card, but
looking at the code, could this implementation still cause a system-wide
display shutdown?

> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 5b89168411aa0..e7e17bf4fdf9e 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
[ ... ]
> @@ -462,9 +462,17 @@ EXPORT_SYMBOL(devm_drm_bridge_add);
>  void drm_bridge_remove(struct drm_bridge *bridge)
>  {
> +	if (bridge->encoder) {
> +		drm_atomic_shutdown(bridge->dev);

Looking at drm_atomic_shutdown(), it unconditionally calls
drm_atomic_disable_all(dev, &ctx), which disables all CRTCs on the device.

If a single bridge is hot-unplugged, won't this unexpectedly shut down all
other independent, active displays on the same system?

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

Is there a possibility of a use-after-free here if bridge->encoder is left
dangling?

When a DRM device is torn down, drm_encoder_cleanup() iterates over the
chain and calls drm_bridge_detach(). That function clears bridge->dev but
does not clear bridge->encoder, and the encoder memory is subsequently freed.

Later, when the bridge driver is unbound, drm_bridge_remove() is executed.
Since bridge->encoder is never cleared, it will still evaluate to true,
passing a dangling pointer to drm_encoder_cleanup_from().

drm_encoder_cleanup_from() would then attempt to acquire the mutex from the
freed encoder memory:

void drm_encoder_cleanup_from(...) {
    ...
    mutex_lock(&encoder->bridge_chain_mutex);
    ...
}

Additionally, could calling this function concurrently for bridges on the
same chain cause stack list corruption?

Looking at drm_encoder_cleanup_from(), it uses a stack-allocated tmplist:

void drm_encoder_cleanup_from(struct drm_encoder *encoder, struct drm_bridge *bridge)
{
    ...
    LIST_HEAD(tmplist);
    mutex_lock(&encoder->bridge_chain_mutex);
    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);
    mutex_unlock(&encoder->bridge_chain_mutex);
    ...
}

If two threads concurrently remove two bridges in the same chain (for example,
Thread 1 removes Bridge A, and Thread 2 removes downstream Bridge B):

Thread 1 moves both bridges to its tmplist and unlocks the mutex.

Thread 2 then locks the mutex and sees !list_empty(&bridge_B->chain_node) is
true because Bridge B is linked within Thread 1's tmplist.

Thread 2 then executes list_for_each_entry_safe_from() looking for the
termination condition &encoder->bridge_chain. Because Bridge B is inside
Thread 1's stack list, it would iterate locklessly into Thread 1's stack
memory.

Would this cause Thread 2 to interpret Thread 1's stack LIST_HEAD as a
drm_bridge and attempt to move it, corrupting both threads' lists and stack
frames?

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

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