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