Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider: - [High] drm: NULL pointer dereferences of last_bridge and optional attach callback - [High] drm: refcount underflow on bridge returned by drm_bridge_get_next() - [Medium] drm: drmm_hotplug_helper_init returns NULL on success violating API contract - [Medium] drm: data race on hotplug_helper->connector outside of connector_mutex - [Medium] drm: duplicate invocation of attach callback bypassing drm_bridge_attach() - [Medium] drm: TOCTOU race misses hotplug events due to late notifier registration - [Low] drm: spurious drm_WARN_ON triggered on normal teardown of incomplete pipeline --
commit 251bdf083a0bf54162b14a9598f43ad560284a46 Author: Luca Ceresoli <[email protected]> drm: drm_hotplug_helper: new helper to implement bridge hotplug This commit adds a new helper to support cards implementing hotpluggable DRM bridges. It registers to get notified of relevant events and reacts by creating a bridge if the pipeline is complete, and destroying it on bridge removal. > diff --git a/drivers/gpu/drm/display/drm_hotplug_helper.c > b/drivers/gpu/drm/display/drm_hotplug_helper.c > index 0000000000000..1b89b14b52aa2 > --- /dev/null > +++ b/drivers/gpu/drm/display/drm_hotplug_helper.c [ ... ] > +static bool drm_hotplug_helper_pipeline_is_complete(struct > drm_hotplug_helper *hotplug_helper) > +{ > + struct drm_bridge *last_bridge __free(drm_bridge_put) = > + drm_bridge_chain_get_last_bridge(hotplug_helper->encoder); > + > + /* We expect at least one bridge */ > + if (!last_bridge) { > + drm_dbg_driver(hotplug_helper->drm, "no bridges in pipeline > (yet)\n"); > + return false; > + } > + > + struct drm_bridge *next_bridge __free(drm_bridge_put) = > + drm_bridge_get_next(last_bridge); [Severity: High] Does this code cause a refcount underflow? The pointer returned by drm_bridge_get_next() directly returns the pointer from the callback without incrementing its kref reference count. When the function returns, drm_bridge_put() executes and decrements the reference count. Since a reference was never acquired, this leads to an underflow and a use-after-free vulnerability. [ ... ] > +static void drm_hotplug_helper_connector_remove(struct drm_hotplug_helper > *hotplug_helper) > +{ > + guard(mutex)(&hotplug_helper->connector_mutex); > + > + if (drm_WARN_ON(hotplug_helper->drm, !hotplug_helper->connector)) > + return; [Severity: Low] Will this trigger a spurious kernel warning during normal device teardown? drmm_hotplug_helper_init() registers drm_hotplug_helper_fini() to run unconditionally on teardown, which calls this function. If a bridge was never hotplugged or the pipeline was incomplete, hotplug_helper->connector legitimately remains NULL. [ ... ] > +static void drm_hotplug_helper_try_complete(struct drm_hotplug_helper > *hotplug_helper) > +{ > + int err; > + > + /* > + * drm_connector already present, the new bridge must be for > + * another card > + */ > + if (hotplug_helper->connector) > + return; [Severity: Medium] Can this unprotected read lead to a data race on hotplug_helper->connector? The connector pointer is documented as protected by the connector_mutex. Since the DRM event notifier uses a blocking notifier chain, concurrent events could locklessly observe the connector as NULL before the addition path completes, potentially leading to duplicate bridge attach attempts. > + > + /* 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(hotplug_helper->encoder); > + err = last_bridge->funcs->attach(last_bridge, hotplug_helper->encoder, > + DRM_BRIDGE_ATTACH_NO_CONNECTOR); [Severity: High] Can this code cause a NULL pointer dereference? drm_bridge_chain_get_last_bridge() returns NULL if the chain is empty, which can occur if an unrelated global DRM_MIPI_DSI_ATTACHED event triggers this on an empty pipeline. Unconditionally dereferencing last_bridge->funcs would crash. Additionally, the attach callback in struct drm_bridge_funcs is documented as optional. Unconditionally dereferencing it will crash for bridges that omit it. [Severity: Medium] Could this also lead to a duplicate invocation of the attach callback? Because last_bridge is already in the bridge chain, it has already been attached during initialization. Bypassing the standard drm_bridge_attach() API and manually calling the internal attach callback on the same bridge breaks driver state expectations, potentially causing memory leaks or hardware state corruption. [ ... ] > +struct drm_hotplug_helper *drmm_hotplug_helper_init(struct drm_device *drm, > + struct drm_encoder *encoder) > +{ > + struct drm_hotplug_helper *hotplug_helper; > + int ret; > + > + hotplug_helper = drmm_kzalloc(drm, sizeof(*hotplug_helper), GFP_KERNEL); > + if (!hotplug_helper) > + return ERR_PTR(-ENOMEM); > + > + mutex_init(&hotplug_helper->connector_mutex); > + hotplug_helper->drm = drm; > + hotplug_helper->encoder = encoder; > + hotplug_helper->drm_event_nb.notifier_call = > drm_hotplug_helper_handle_event; > + > + if (drm_hotplug_helper_pipeline_is_complete(hotplug_helper)) { > + ret = drm_hotplug_helper_connector_add(hotplug_helper); > + if (ret) > + return ERR_PTR(ret); > + } > + > + ret = drmm_add_action_or_reset(drm, drm_hotplug_helper_fini, > hotplug_helper); > + if (ret) > + return ERR_PTR(ret); > + > + ret = drm_event_notifier_register(&hotplug_helper->drm_event_nb); [Severity: Medium] Does the gap between evaluating pipeline completeness and registering for notifications create a time-of-check to time-of-use race? If a bridge is hotplugged in the exact window between the pipeline check above and this notifier registration, the DRM_MIPI_DSI_ATTACHED event is missed, leaving the pipeline permanently incomplete. > + if (ret) > + return ERR_PTR(ret); > + > + ret = drmm_add_action_or_reset(drm, > drm_hotplug_helper_notifier_unregister, > + &hotplug_helper->drm_event_nb); > + if (ret) > + return ERR_PTR(ret); > + > + return 0; [Severity: Medium] Does returning 0 violate the API contract? The function signature and kernel-doc state it returns a struct drm_hotplug_helper pointer on success, or an error pointer on failure. The success path executes return 0, returning a NULL pointer. Callers correctly checking IS_ERR() will assume success and any subsequent dereference of the returned pointer will crash. > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=23
