Re: [PATCH] drm/amd/display: Fix NULL pointer dereference in amdgpu_dm_crtc_set_vblank()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless access to `crtc->state` in the vblank enable hook can lead to a Use-After-Free of the state and stream objects.
--

--- Patch [1]: [PATCH] drm/amd/display: Fix NULL pointer dereference in amdgpu_dm_crtc_set_vblank() ---
commit 22682e17c05c1df8dcb5140d5111a3b728f76c8e
Author: Samuel Pitoiset <[email protected]>
Subject: drm/amd/display: Fix NULL pointer dereference in amdgpu_dm_crtc_set_vblank()

This commit adds a check to ensure acrtc_state->stream is non-NULL before
dereferencing it in the vblank enable path, fixing a crash when vblank is
queried early.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> index 46d85457c77b9..62eac6e65334b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c
> @@ -268,7 +268,7 @@ static inline int amdgpu_dm_crtc_set_vblank(struct drm_crtc *crtc, bool enable)
>  
>  	irq_type = amdgpu_display_crtc_idx_to_irq_type(adev, acrtc->crtc_id);
>  
> -	if (enable) {
> +	if (enable && acrtc_state->stream) {
>  		struct dc *dc = adev->dm.dc;
>  		struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>  		struct psr_settings *psr = &acrtc_state->stream->link->psr_settings;

[Severity: High]
This is a pre-existing issue, but could this lockless access to crtc->state
lead to a use-after-free of the state and stream objects?

The enable_vblank hook (amdgpu_dm_crtc_set_vblank) is reached through the ioctl
path where only vbl_lock is held, not crtc->mutex:

drm_crtc_get_sequence_ioctl()
  drm_vblank_get()
    amdgpu_dm_crtc_set_vblank()

Concurrently, a non-blocking atomic commit can swap crtc->state and free the
old state asynchronously.

If the hook is preempted or delayed, the old state could be freed before
dereferencing acrtc_state->stream here.

Could this also cause the dc_stream_retain(acrtc_state->stream) call further
down in this function to corrupt memory by incrementing the refcount of a
freed object?

>  		struct replay_settings *pr = &acrtc_state->stream->link->replay_settings;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1