Re: [PATCH] drm/amd/display: fix NULL ptr deref in ISM delayed work

[Matthew Schwartz <matthew.schwartz@linux.dev>]

On 4/9/26 12:20 AM, Ray Wu wrote:
> dc_destroy() sets dm->dc to NULL before amdgpu_dm_ism_fini() is called,
> leaving a window where in-flight ISM delayed work dereferences the stale
> pointer. Call amdgpu_dm_ism_fini() in amdgpu_dm_fini() before dc_destroy().
> 
> Fixes: f5d0d3f3439e ("drm/amd/display: Add Idle state manager(ISM)")
> Signed-off-by: Ray Wu <ray.wu@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c      | 9 +++++++++
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_crtc.c | 7 ++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> 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 bac02ea15b8a..bb79b6bed3c4 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2236,6 +2236,8 @@ static int amdgpu_dm_early_fini(struct amdgpu_ip_block *ip_block)
>  static void amdgpu_dm_fini(struct amdgpu_device *adev)
>  {
>  	int i;
> +	struct drm_crtc *crtc;
> +	struct amdgpu_crtc *acrtc;
>  
>  	if (adev->dm.vblank_control_workqueue) {
>  		destroy_workqueue(adev->dm.vblank_control_workqueue);
> @@ -2252,6 +2254,13 @@ static void amdgpu_dm_fini(struct amdgpu_device *adev)
>  		adev->dm.idle_workqueue = NULL;
>  	}
>  
> +	/* Finalize ISM for each CRTC before dc_destroy() sets dm->dc to NULL */
> +	drm_for_each_crtc(crtc, adev_to_drm(adev)) {
> +		acrtc = to_amdgpu_crtc(crtc);
> +		amdgpu_dm_ism_fini(&acrtc->ism);
> +
> +	}
> +
>  	amdgpu_dm_destroy_drm_device(&adev->dm);
>  
>  #if defined(CONFIG_DRM_AMD_SECURE_DISPLAY)
> 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 26f3d513576b..de203445e084 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
> @@ -459,7 +459,12 @@ static void amdgpu_dm_crtc_destroy(struct drm_crtc *crtc)
>  {
>  	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>  
> -	amdgpu_dm_ism_fini(&acrtc->ism);
> +	/*
> +	 * amdgpu_dm_ism_fini() is intentionally called in amdgpu_dm_fini().
> +	 * It must be called before dc_destroy() in amdgpu_dm_fini()
> +	 * to avoid ISM accessing an invalid dc handle once dc is released.
> +	 */

I'm seeing a new build warning with this hunk:

drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_crtc.c: In function ‘amdgpu_dm_crtc_destroy’:
drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_crtc.c:460:29: warning: unused variable ‘acrtc’ [-Wunused-variable]
  460 |         struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
      |                             ^~~~~

This diff resolved it:
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 de203445e0844..40c5f74dbe2b6 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
@@ -457,8 +457,6 @@ static struct drm_crtc_state *amdgpu_dm_crtc_duplicate_state(struct drm_crtc *cr
 
 static void amdgpu_dm_crtc_destroy(struct drm_crtc *crtc)
 {
-	struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
-
 	/*
 	 * amdgpu_dm_ism_fini() is intentionally called in amdgpu_dm_fini().
 	 * It must be called before dc_destroy() in amdgpu_dm_fini()

Thanks,
Matt

> +
>  	drm_crtc_cleanup(crtc);
>  	kfree(crtc);
>  }