* [PATCH] drm/amd/display: Snapshot dm_crtc_state->stream before use
@ 2026-10-08 7:16 2564278112
2026-10-08 7:43 ` sashiko-bot
0 siblings, 1 reply; 2+ messages in thread
From: 2564278112 @ 2026-10-08 7:16 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, David Airlie, Simona Vetter, amd-gfx
Cc: Alex Hung, Mario Limonciello, Timur Kristóf, Ray Wu,
Ivan Lipski, Lu Yao, dri-devel, linux-kernel, Wang Jiang
From: Wang Jiang <jiangwang@kylinos.cn>
The suspend/resume path releases the DC streams and clears
dm_crtc_state->stream (dm_destroy_cached_state()) while an atomic commit
can still be in flight. A commit that races with it therefore observes
the member becoming NULL after it has already been NULL checked, and
dereferences it again for every plane or stream update. The fbdev
damage worker is one such commit: it is queued by the console and runs
after the workqueues are thawed during resume, which is exactly when the
display manager is tearing down and rebuilding its cached state.
Observed with the amdgpu DKMS module:
BUG: unable to handle page fault for address: 0000000000006490
RIP: amdgpu_dm_atomic_commit_tail+0x14c3/0x3cb0 [amdgpu_pro]
Workqueue: events drm_fb_helper_damage_work
Kernel panic - not syncing: amdgpu-pro 0000:04:00.0: unrecoverable failure
0x6490 is offsetof(struct dc_stream_state, abm_level), i.e. the stream
pointer read as NULL right before
acrtc_state->stream->abm_level = acrtc_state->abm_level;
Take a single snapshot of dm_crtc_state->stream in
amdgpu_dm_commit_planes() and amdgpu_dm_enable_self_refresh(), check it
before use, and use the same snapshot for the connector stream update in
amdgpu_dm_atomic_commit_tail(), skipping the update when no stream is
attached.
Signed-off-by: Wang Jiang <jiangwang@kylinos.cn>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 112 ++++++++++++------
1 file changed, 73 insertions(+), 39 deletions(-)
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 91fdf3de7202..c4c8904fea42 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3696,10 +3696,21 @@ static void amdgpu_dm_enable_self_refresh(struct amdgpu_display_manager *dm,
const struct dm_crtc_state *acrtc_state,
const u64 current_ts)
{
- struct psr_settings *psr = &acrtc_state->stream->link->psr_settings;
- struct replay_settings *pr = &acrtc_state->stream->link->replay_settings;
- struct amdgpu_dm_connector *aconn =
- (struct amdgpu_dm_connector *)acrtc_state->stream->dm_stream_context;
+ struct dc_stream_state *stream = acrtc_state->stream;
+ struct psr_settings *psr;
+ struct replay_settings *pr;
+ struct amdgpu_dm_connector *aconn;
+
+ /*
+ * The stream can be cleared concurrently by the system suspend/resume
+ * path, so make sure the snapshot we are about to dereference is valid.
+ */
+ if (!stream || !stream->link)
+ return;
+
+ psr = &stream->link->psr_settings;
+ pr = &stream->link->replay_settings;
+ aconn = (struct amdgpu_dm_connector *)stream->dm_stream_context;
/* Decrement skip count when SR is enabled and we're doing fast updates. */
if (acrtc_state->update_type == UPDATE_TYPE_FAST &&
@@ -3720,10 +3731,10 @@ static void amdgpu_dm_enable_self_refresh(struct amdgpu_display_manager *dm,
*/
if (acrtc_attach->dm_irq_params.allow_sr_entry &&
(current_ts - psr->psr_dirty_rects_change_timestamp_ns) > 500000000) {
- amdgpu_dm_psr_set_event(dm, acrtc_state->stream, false,
+ amdgpu_dm_psr_set_event(dm, stream, false,
psr_event_hw_programming, false);
- amdgpu_dm_replay_set_event(dm, acrtc_state->stream, false,
+ amdgpu_dm_replay_set_event(dm, stream, false,
replay_event_hw_programming, false);
}
} else {
@@ -3799,6 +3810,15 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
struct dm_crtc_state *acrtc_state = to_dm_crtc_state(new_pcrtc_state);
struct dm_crtc_state *dm_old_crtc_state =
to_dm_crtc_state(drm_atomic_get_old_crtc_state(state, pcrtc));
+ /*
+ * Take a single snapshot of the stream pointer and use it everywhere
+ * below. dm_crtc_state->stream can be cleared concurrently, e.g. by the
+ * system suspend/resume path (dm_destroy_cached_state()), so re-reading
+ * the member after a NULL check can observe NULL and crash while writing
+ * dc_stream_state members (see the NULL write at
+ * offsetof(struct dc_stream_state, abm_level)).
+ */
+ struct dc_stream_state *stream = acrtc_state->stream;
int planes_count = 0, vpos, hpos;
unsigned long flags;
u32 target_vblank, last_flip_vblank;
@@ -3836,12 +3856,12 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
dm_old_crtc_state->cursor_mode == DM_CURSOR_NATIVE_MODE) {
struct dc_cursor_position cursor_position = {0};
- if (!dc_stream_set_cursor_position(acrtc_state->stream,
+ if (!dc_stream_set_cursor_position(stream,
&cursor_position))
drm_err(dev, "DC failed to disable native cursor\n");
bundle->stream_update.cursor_position =
- &acrtc_state->stream->cursor_position;
+ &stream->cursor_position;
}
if (acrtc_state->active_planes == 0 &&
@@ -3919,12 +3939,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) {
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);
@@ -3935,14 +3955,14 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
* amdgpu_dm_crtc_vblank_control_worker() if user
* pause the video during the PSR-SU was disabled.
*/
- if (acrtc_state->stream->link->psr_settings.psr_version >= DC_PSR_VERSION_SU_1 &&
+ if (stream->link->psr_settings.psr_version >= DC_PSR_VERSION_SU_1 &&
acrtc_attach->dm_irq_params.allow_sr_entry &&
dirty_rects_changed) {
mutex_lock(&dm->dc_lock);
- acrtc_state->stream->link->psr_settings.psr_dirty_rects_change_timestamp_ns =
+ stream->link->psr_settings.psr_dirty_rects_change_timestamp_ns =
timestamp_ns;
dc_exit_ips_for_hw_access(dm->dc);
- amdgpu_dm_psr_set_event(dm, acrtc_state->stream, true,
+ amdgpu_dm_psr_set_event(dm, stream, true,
psr_event_hw_programming, true);
mutex_unlock(&dm->dc_lock);
}
@@ -3985,7 +4005,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
amdgpu_dm_update_freesync_state_on_stream(
dm,
acrtc_state,
- acrtc_state->stream,
+ stream,
dc_plane,
bundle->flip_addrs[planes_count].flip_timestamp_in_us);
@@ -4038,10 +4058,10 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
usleep_range(1000, 1100);
}
- if (acrtc_state->stream) {
+ if (stream) {
if (acrtc_state->freesync_vrr_info_changed)
bundle->stream_update.vrr_infopacket =
- &acrtc_state->stream->vrr_infopacket;
+ &stream->vrr_infopacket;
}
}
@@ -4063,7 +4083,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
/* Update the planes if changed or disable if we don't have any. */
if ((planes_count || acrtc_state->active_planes == 0) &&
- acrtc_state->stream) {
+ stream) {
/*
* If PSR or idle optimizations are enabled then flush out
* any pending work before hardware programming.
@@ -4071,10 +4091,10 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
if (dm->vblank_control_workqueue)
flush_workqueue(dm->vblank_control_workqueue);
- bundle->stream_update.stream = acrtc_state->stream;
+ bundle->stream_update.stream = stream;
if (new_pcrtc_state->mode_changed) {
- bundle->stream_update.src = acrtc_state->stream->src;
- bundle->stream_update.dst = acrtc_state->stream->dst;
+ bundle->stream_update.src = stream->src;
+ bundle->stream_update.dst = stream->dst;
}
if (new_pcrtc_state->color_mgmt_changed) {
@@ -4083,18 +4103,18 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
* already modified the stream in place.
*/
bundle->stream_update.gamut_remap =
- &acrtc_state->stream->gamut_remap_matrix;
+ &stream->gamut_remap_matrix;
bundle->stream_update.output_csc_transform =
- &acrtc_state->stream->csc_color_matrix;
+ &stream->csc_color_matrix;
bundle->stream_update.out_transfer_func =
- &acrtc_state->stream->out_transfer_func;
+ &stream->out_transfer_func;
bundle->stream_update.lut3d_func =
- (struct dc_3dlut *) acrtc_state->stream->lut3d_func;
+ (struct dc_3dlut *) stream->lut3d_func;
bundle->stream_update.func_shaper =
- (struct dc_transfer_func *) acrtc_state->stream->func_shaper;
+ (struct dc_transfer_func *) stream->func_shaper;
}
- acrtc_state->stream->abm_level = acrtc_state->abm_level;
+ stream->abm_level = acrtc_state->abm_level;
if (acrtc_state->abm_level != dm_old_crtc_state->abm_level)
bundle->stream_update.abm_level = &acrtc_state->abm_level;
@@ -4106,7 +4126,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)) {
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);
}
@@ -4114,7 +4134,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
update_planes_and_stream_adapter(dm->dc,
acrtc_state->update_type,
planes_count,
- acrtc_state->stream,
+ stream,
&bundle->stream_update,
bundle->surface_updates);
updated_planes_and_streams = true;
@@ -5045,6 +5065,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
struct dc_stream_update stream_update;
struct dc_info_packet hdr_packet;
struct dc_stream_status *status = NULL;
+ struct dc_stream_state *stream = NULL;
bool abm_changed, hdr_changed, scaling_changed, output_color_space_changed = false;
memset(&stream_update, 0, sizeof(stream_update));
@@ -5065,6 +5086,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;
@@ -5078,24 +5100,36 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
if (!scaling_changed && !abm_changed && !hdr_changed && !output_color_space_changed)
continue;
- stream_update.stream = dm_new_crtc_state->stream;
+ /*
+ * Take a snapshot of the stream pointer once. The member can be
+ * cleared concurrently by the system suspend/resume path
+ * (dm_destroy_cached_state()), so re-reading it after this point
+ * can observe NULL and crash.
+ */
+ stream = dm_new_crtc_state->stream;
+ if (!stream) {
+ drm_dbg_state(dev, "No stream attached to CRTC, skipping stream update\n");
+ continue;
+ }
+
+ stream_update.stream = stream;
if (scaling_changed) {
amdgpu_dm_update_stream_scaling_settings(dev, &dm_new_con_state->base.crtc->mode,
- dm_new_con_state, dm_new_crtc_state->stream);
+ dm_new_con_state, stream);
- stream_update.src = dm_new_crtc_state->stream->src;
- stream_update.dst = dm_new_crtc_state->stream->dst;
+ stream_update.src = stream->src;
+ stream_update.dst = stream->dst;
}
if (output_color_space_changed) {
- dm_new_crtc_state->stream->output_color_space
- = amdgpu_dm_get_output_color_space(&dm_new_crtc_state->stream->timing, new_con_state);
+ stream->output_color_space
+ = amdgpu_dm_get_output_color_space(&stream->timing, new_con_state);
- stream_update.output_color_space = &dm_new_crtc_state->stream->output_color_space;
+ stream_update.output_color_space = &stream->output_color_space;
}
if (abm_changed) {
- dm_new_crtc_state->stream->abm_level = dm_new_crtc_state->abm_level;
+ stream->abm_level = dm_new_crtc_state->abm_level;
stream_update.abm_level = &dm_new_crtc_state->abm_level;
}
@@ -5105,7 +5139,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
stream_update.hdr_static_metadata = &hdr_packet;
}
- status = dc_stream_get_status(dm_new_crtc_state->stream);
+ status = dc_stream_get_status(stream);
if (WARN_ON(!status))
continue;
@@ -5133,7 +5167,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_commit *state)
dc_update_planes_and_stream(dm->dc,
dummy_updates,
status->plane_count,
- dm_new_crtc_state->stream,
+ stream,
&stream_update);
mutex_unlock(&dm->dc_lock);
kfree(dummy_updates);
--
2.25.1
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH] drm/amd/display: Snapshot dm_crtc_state->stream before use
2026-10-08 7:16 [PATCH] drm/amd/display: Snapshot dm_crtc_state->stream before use 2564278112
@ 2026-10-08 7:43 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-10-08 7:43 UTC (permalink / raw)
To: 2564278112; +Cc: dri-devel
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 <jiangwang@kylinos.cn>
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/tencent_3F4F1469A115C5112FD7CA3CC4A9CF6F6C07@qq.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-10-08 7:43 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-08 7:16 [PATCH] drm/amd/display: Snapshot dm_crtc_state->stream before use 2564278112
2026-10-08 7:43 ` sashiko-bot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.