Re: [PATCH] drm/amd/display: Snapshot dm_crtc_state->stream before use

[sashiko-bot@kernel.org]

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