Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/amd: Missing NULL check on snapshotted stream in amdgpu_dm_commit_planes - [High] drm/amd: TOCTOU race on stream pointers in amdgpu_dm_atomic_commit_tail --
commit b5d41316e3dbf4b387efaeed72f5a145535e3564 Author: Wang Jiang <[email protected]> Subject: drm/amd/display: Snapshot dm_crtc_state->stream before use This commit introduces local snapshots for the display manager's stream pointers to prevent NULL pointer dereferences caused by concurrent clearances during system suspend and resume. However, there are a few places where the snapshots are bypassed or remain unvalidated before use. > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > index 1b42c4606002e..b31ac9434da1d 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c [ ... ] > @@ -3926,12 +3946,12 @@ static void amdgpu_dm_commit_planes(struct > drm_atomic_commit *state, > bundle->surface_updates[planes_count].plane_info = > &bundle->plane_infos[planes_count]; > > - if (acrtc_state->stream->link->psr_settings.psr_feature_enabled > || > - > acrtc_state->stream->link->replay_settings.replay_feature_enabled) { > + if (stream->link->psr_settings.psr_feature_enabled || > + stream->link->replay_settings.replay_feature_enabled) { [Severity: High] Does amdgpu_dm_commit_planes() need to verify if the captured snapshot is valid before proceeding? If the stream was concurrently cleared and the snapshot is NULL, evaluating this condition will result in a kernel panic since the stream pointer is dereferenced unconditionally here. > fill_dc_dirty_rects(plane, old_plane_state, > new_plane_state, new_crtc_state, > &bundle->flip_addrs[planes_count], > - > acrtc_state->stream->link->psr_settings.psr_version == > + > stream->link->psr_settings.psr_version == > DC_PSR_VERSION_SU_1, > &dirty_rects_changed); [ ... ] > @@ -4113,7 +4133,7 @@ static void amdgpu_dm_commit_planes(struct > drm_atomic_commit *state, > if (amdgpu_dm_is_dc_timing_adjust_needed(dm_old_crtc_state, > acrtc_state)) { [Severity: High] Does passing the acrtc_state object here bypass the snapshot entirely? The helper function amdgpu_dm_is_dc_timing_adjust_needed() directly reads the stream from the state object: amdgpu_dm_freesync.c:amdgpu_dm_is_dc_timing_adjust_needed() { if (new_state->stream->adjust.timing_adjust_pending) return true; ... If the stream was concurrently cleared, this directly dereferences the NULL pointer from the state object, causing a crash. > spin_lock_irqsave(&pcrtc->dev->event_lock, flags); > dc_stream_adjust_vmin_vmax( > - dm->dc, acrtc_state->stream, > + dm->dc, stream, > &acrtc_attach->dm_irq_params.vrr_params.adjust); > spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags); > } [ ... ] > @@ -5072,6 +5093,7 @@ static void amdgpu_dm_atomic_commit_tail(struct > drm_atomic_commit *state) > dm_old_con_state); > > if ((new_con_state->hdmi.broadcast_rgb != > old_con_state->hdmi.broadcast_rgb) && > + dm_old_crtc_state->stream && dm_new_crtc_state->stream > && > (dm_old_crtc_state->stream->output_color_space != > > amdgpu_dm_get_output_color_space(&dm_new_crtc_state->stream->timing, > new_con_state))) > output_color_space_changed = true; [Severity: High] Does checking the stream pointer here and immediately rereading it from the state structure create a time-of-check to time-of-use race in amdgpu_dm_atomic_commit_tail()? If dm_new_crtc_state->stream is concurrently set to NULL immediately after evaluating to true in the check, taking the address of its timing field: &dm_new_crtc_state->stream->timing will compute an invalid offset pointer (NULL + offset). This invalid pointer is then passed into amdgpu_dm_get_output_color_space(), which attempts to access its fields: amdgpu_dm_connector.c:amdgpu_dm_get_output_color_space() { ... case DRM_MODE_COLORIMETRY_BT601_YCC: if (dc_crtc_timing->flags.Y_ONLY) ... Dereferencing this invalid offset pointer will cause a page fault. To close this window, the state pointers should likely be captured into local snapshots before being evaluated. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
