Re: [PATCH RFC v2 23/24] drm: drm_hotplug_helper: new helper to implement bridge hotplug

[sashiko-bot@kernel.org]

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 <luca.ceresoli@bootlin.com>

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/20261001-drm-bridge-hotplug-v2-0-8e34986dcb68@bootlin.com?part=23