Re: [PATCH RFC] drm/amd/display: Disable CRTC on MST connector unregister

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/amd: TOCTOU race and UAF from lockless crtc read
- [Medium] drm/amd: incomplete CRTC disable leaks planes and FBs
--

commit fad621ba901c426d67d64397ce2dc5fd953430db
Author: David Medina <b2amedina@gmail.com>

drm/amd/display: Disable CRTC on MST connector unregister

This commit forces a modeset that disables the CRTC when an MST connector is
unregistered. This prevents an encoder conflict during subsequent re-plugs.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index 045a7f88b7546..24b1460687393 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -227,6 +228,7 @@ amdgpu_dm_mst_connector_early_unregister(struct drm_connector *connector)
>  	struct amdgpu_dm_connector *root = aconnector->mst_root;
>  	struct dc_link *dc_link = aconnector->dc_link;
>  	struct dc_sink *dc_sink = aconnector->dc_sink;
> +	struct drm_crtc *crtc = connector->state ? connector->state->crtc : NULL;

[Severity: High]
Does amdgpu_dm_mst_connector_early_unregister() in
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c read
connector->state->crtc locklessly before locks are acquired?

If a concurrent modeset occurs when hot-unplugging an MST display, the
captured crtc pointer may be stale. When drm_atomic_get_crtc_state() later
locks and disables this CRTC, could it now belong to another display or have
been freed, leading to a use-after-free or unintended display teardown?

[ ... ]
> +	if (crtc) {
> +		struct drm_atomic_commit *state;
> +		struct drm_connector_state *conn_state;
> +		struct drm_crtc_state *crtc_state;
> +		struct drm_modeset_acquire_ctx ctx;
> +		int ret;
> +
> +		drm_modeset_acquire_init(&ctx, 0);
> +retry:
> +		state = drm_atomic_commit_alloc(connector->dev);
> +		if (!state)
> +			goto out;
> +		state->acquire_ctx = &ctx;
> +
> +		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +		ret = PTR_ERR_OR_ZERO(crtc_state);
> +		if (!ret)
> +			crtc_state->active = false;
> +		if (!ret)
> +			ret = drm_atomic_set_mode_prop_for_crtc(crtc_state, NULL);
> +		if (!ret) {
> +			conn_state = drm_atomic_get_connector_state(state, connector);
> +			ret = PTR_ERR_OR_ZERO(conn_state);
> +		}
> +		if (!ret)
> +			ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> +		if (!ret)
> +			ret = drm_atomic_commit(state);

[Severity: Medium]
Is amdgpu_dm_mst_connector_early_unregister() missing the step to detach the
affected planes from the disabled CRTC before committing the atomic state?

The DRM core atomic helpers require that planes attached to a disabled CRTC
have their framebuffers detached.

If drm_atomic_helper_check_modeset() automatically adds the planes to the
state, won't drm_atomic_helper_check_planes() reject the commit because the
planes still have framebuffers?

If these checks are bypassed, could this leak planes and framebuffers
indefinitely when an active MST connector is unplugged?

> +
> +		drm_atomic_commit_put(state);
> +		if (ret == -EDEADLK) {
> +			drm_modeset_backoff(&ctx);
> +			goto retry;

[Severity: High]
If a deadlock occurs and we drop locks to backoff, should the crtc pointer
be re-read safely under lock after jumping back to the retry label?

By reusing the pointer read locklessly earlier in the function, the retry
loop might operate on a stale pointer if the state changed during the
backoff.

> +		}
> +		if (ret)
> +			drm_err(connector->dev,
> +				"DM_MST: failed to disable CRTC for removed connector %s (%d)\n",
> +				connector->name, ret);
> +out:
> +		drm_modeset_drop_locks(&ctx);
> +		drm_modeset_acquire_fini(&ctx);
> +	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-amd-mst-teardown-v1-1-16a39ebdd4de@gmail.com?part=1