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 <[email protected]> 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/[email protected]?part=12
