[PATCH v2] drm/amd/display: Fix dangling pointers in state reset functions on allocation failure

[PATCH v2] drm/amd/display: Fix dangling pointers in state reset functions on allocation failure

[Evgenii Burenchev]

Multiple reset functions in amdgpu_dm free the old state before allocating
a new one. If kzalloc_obj() fails, the function returns without updating
the state pointer, leaving a dangling pointer to already freed memory.

Fix this by allocating the new state first. If allocation fails, warn and
return without touching the old state, as it remains valid and the caller
will handle the error appropriately.

This affects three functions:
- amdgpu_dm_plane_drm_plane_reset()
- amdgpu_dm_crtc_reset_state()
- amdgpu_dm_connector_funcs_reset()

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Fixes: 5d945cbcd4b1 ("drm/amd/display: Create a file dedicated to planes")
Fixes: 473683a03495 ("drm/amd/display: Create a file dedicated for CRTC")
Fixes: e7b07ceef2a6 ("drm/amd/display: Merge amdgpu_dm_types and amdgpu_dm")
Signed-off-by: Evgenii Burenchev <evg28bur@yandex.ru>
---
Changes in v2:
- Also fix amdgpu_dm_crtc_reset_state() and amdgpu_dm_connector_funcs_reset()
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 48 ++++++++++---------
 .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    |  8 ++--
 .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 14 +++---
 3 files changed, 37 insertions(+), 33 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 97ab1e83b318..6ef1b07ec251 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8151,33 +8151,35 @@ static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
 
 void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
 {
-	struct dm_connector_state *state =
-		to_dm_connector_state(connector->state);
-
-	if (connector->state)
-		__drm_atomic_helper_connector_destroy_state(connector->state);
-
-	kfree(state);
+	struct dm_connector_state *state;
 
+	/* Allocate new state first */
 	state = kzalloc_obj(*state);
+	if (WARN_ON(!state))
+		return;
 
-	if (state) {
-		state->scaling = RMX_OFF;
-		state->underscan_enable = false;
-		state->underscan_hborder = 0;
-		state->underscan_vborder = 0;
-		state->base.max_requested_bpc = 8;
-		state->vcpi_slots = 0;
-		state->pbn = 0;
-
-		if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
-			if (amdgpu_dm_abm_level <= 0)
-				state->abm_level = ABM_LEVEL_IMMEDIATE_DISABLE;
-			else
-				state->abm_level = amdgpu_dm_abm_level;
-		}
+	/* Destroy old state only after successful allocation */
+	if (connector->state)
+		__drm_atomic_helper_connector_destroy_state(connector->state);
 
-		__drm_atomic_helper_connector_reset(connector, &state->base);
+	/* Let DRM core install the new state */
+	__drm_atomic_helper_connector_reset(connector, &state->base);
+
+	/* Initialize driver-specific fields */
+	state->scaling = RMX_OFF;
+	state->underscan_enable = false;
+	state->underscan_hborder = 0;
+	state->underscan_vborder = 0;
+	state->base.max_requested_bpc = 8;
+	state->vcpi_slots = 0;
+	state->pbn = 0;
+
+	/* eDP-specific initialization */
+	if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
+		if (amdgpu_dm_abm_level <= 0)
+			state->abm_level = ABM_LEVEL_IMMEDIATE_DISABLE;
+		else
+			state->abm_level = amdgpu_dm_abm_level;
 	}
 }
 
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 3dcedaa67ed8..6146fbc528c3 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
@@ -437,13 +437,15 @@ static void amdgpu_dm_crtc_reset_state(struct drm_crtc *crtc)
 {
 	struct dm_crtc_state *state;
 
-	if (crtc->state)
-		amdgpu_dm_crtc_destroy_state(crtc, crtc->state);
-
+	/* Allocate new state first */
 	state = kzalloc_obj(*state);
 	if (WARN_ON(!state))
 		return;
 
+	/* Destroy old state only after successful allocation */
+	if (crtc->state)
+		amdgpu_dm_crtc_destroy_state(crtc, crtc->state);
+
 	__drm_atomic_helper_crtc_reset(crtc, &state->base);
 }
 
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
index e957657b06c7..eb1c0a26f20d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
@@ -1488,17 +1488,17 @@ static const struct drm_plane_helper_funcs dm_primary_plane_helper_funcs = {
 
 static void amdgpu_dm_plane_drm_plane_reset(struct drm_plane *plane)
 {
-	struct dm_plane_state *amdgpu_state = NULL;
-
-	if (plane->state)
-		plane->funcs->atomic_destroy_state(plane, plane->state);
+	struct dm_plane_state *amdgpu_state;
 
+	/* Allocate new state first */
 	amdgpu_state = kzalloc_obj(*amdgpu_state);
-	WARN_ON(amdgpu_state == NULL);
-
-	if (!amdgpu_state)
+	if (WARN_ON(!amdgpu_state))
 		return;
 
+	/* Destroy old state only after successful allocation */
+	if (plane->state)
+		plane->funcs->atomic_destroy_state(plane, plane->state);
+
 	__drm_atomic_helper_plane_reset(plane, &amdgpu_state->base);
 	amdgpu_state->degamma_tf = AMDGPU_TRANSFER_FUNCTION_DEFAULT;
 	amdgpu_state->hdr_mult = AMDGPU_HDR_MULT_DEFAULT;
-- 
2.43.0

Re: [PATCH v2] drm/amd/display: Fix dangling pointers in state reset functions on allocation failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The old connector state memory is leaked during a successful connector reset in `amdgpu_dm_connector_funcs_reset()`.
--

commit 48ece48fead57764b257711d256ed0e86eefc3c4
Author: Evgenii Burenchev <evg28bur@yandex.ru>
Subject: drm/amd/display: Fix dangling pointers in state reset functions on allocation failure

This patch addresses dangling pointers in AMD display state reset functions by
allocating new state structures before destroying the old ones.

> 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 eb5696b5daeb7..c7fe28758e962 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8151,33 +8151,35 @@ static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
>  
>  void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
>  {
> -	struct dm_connector_state *state =
> -		to_dm_connector_state(connector->state);
> -
> -	if (connector->state)
> -		__drm_atomic_helper_connector_destroy_state(connector->state);
> -
> -	kfree(state);
> +	struct dm_connector_state *state;
>  
> +	/* Allocate new state first */
>  	state = kzalloc_obj(*state);
> +	if (WARN_ON(!state))
> +		return;
>  
> -	if (state) {
> -		state->scaling = RMX_OFF;

[ ... ]

> +	/* Destroy old state only after successful allocation */
> +	if (connector->state)
> +		__drm_atomic_helper_connector_destroy_state(connector->state);
>  
> -		__drm_atomic_helper_connector_reset(connector, &state->base);
> +	/* Let DRM core install the new state */
> +	__drm_atomic_helper_connector_reset(connector, &state->base);

[Severity: Medium]
Does this code leak the old connector state memory?

The original code explicitly called kfree(state) after calling
__drm_atomic_helper_connector_destroy_state(connector->state). The new
implementation removes this kfree() call.

Looking at __drm_atomic_helper_connector_destroy_state(), it only cleans up
references to nested DRM objects (like CRTCs or properties) but does not free
the connector_state structure itself. When __drm_atomic_helper_connector_reset()
then overwrites connector->state with the new pointer, it appears to
permanently orphan the old memory.

Should the old state memory be explicitly freed here as it was previously?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626133859.21492-1-evg28bur@yandex.ru?part=1

Re: [PATCH v2] drm/amd/display: Fix dangling pointers in state reset functions on allocation failure

[Mario Limonciello]

On 6/26/26 08:38, Evgenii Burenchev wrote:
> Multiple reset functions in amdgpu_dm free the old state before allocating
> a new one. If kzalloc_obj() fails, the function returns without updating
> the state pointer, leaving a dangling pointer to already freed memory.
> 
> Fix this by allocating the new state first. If allocation fails, warn and
> return without touching the old state, as it remains valid and the caller
> will handle the error appropriately.
> 
> This affects three functions:
> - amdgpu_dm_plane_drm_plane_reset()
> - amdgpu_dm_crtc_reset_state()
> - amdgpu_dm_connector_funcs_reset()
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 5d945cbcd4b1 ("drm/amd/display: Create a file dedicated to planes")
> Fixes: 473683a03495 ("drm/amd/display: Create a file dedicated for CRTC")
> Fixes: e7b07ceef2a6 ("drm/amd/display: Merge amdgpu_dm_types and amdgpu_dm")
> Signed-off-by: Evgenii Burenchev <evg28bur@yandex.ru>
> ---
> Changes in v2:
> - Also fix amdgpu_dm_crtc_reset_state() and amdgpu_dm_connector_funcs_reset()
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 48 ++++++++++---------
>   .../amd/display/amdgpu_dm/amdgpu_dm_crtc.c    |  8 ++--
>   .../amd/display/amdgpu_dm/amdgpu_dm_plane.c   | 14 +++---
>   3 files changed, 37 insertions(+), 33 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 97ab1e83b318..6ef1b07ec251 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8151,33 +8151,35 @@ static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
>   
>   void amdgpu_dm_connector_funcs_reset(struct drm_connector *connector)
>   {
> -	struct dm_connector_state *state =
> -		to_dm_connector_state(connector->state);
> -
> -	if (connector->state)
> -		__drm_atomic_helper_connector_destroy_state(connector->state);
> -
> -	kfree(state);

You seem to have lost this kfree() in your refactor.

> +	struct dm_connector_state *state;
>   
> +	/* Allocate new state first */
>   	state = kzalloc_obj(*state);
> +	if (WARN_ON(!state))
> +		return;
>   
> -	if (state) {
> -		state->scaling = RMX_OFF;
> -		state->underscan_enable = false;
> -		state->underscan_hborder = 0;
> -		state->underscan_vborder = 0;
> -		state->base.max_requested_bpc = 8;
> -		state->vcpi_slots = 0;
> -		state->pbn = 0;
> -
> -		if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
> -			if (amdgpu_dm_abm_level <= 0)
> -				state->abm_level = ABM_LEVEL_IMMEDIATE_DISABLE;
> -			else
> -				state->abm_level = amdgpu_dm_abm_level;
> -		}
> +	/* Destroy old state only after successful allocation */
> +	if (connector->state)
> +		__drm_atomic_helper_connector_destroy_state(connector->state);
>   
> -		__drm_atomic_helper_connector_reset(connector, &state->base);
> +	/* Let DRM core install the new state */
> +	__drm_atomic_helper_connector_reset(connector, &state->base);
> +
> +	/* Initialize driver-specific fields */
> +	state->scaling = RMX_OFF;
> +	state->underscan_enable = false;
> +	state->underscan_hborder = 0;
> +	state->underscan_vborder = 0;
> +	state->base.max_requested_bpc = 8;
> +	state->vcpi_slots = 0;
> +	state->pbn = 0;
> +
> +	/* eDP-specific initialization */
> +	if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
> +		if (amdgpu_dm_abm_level <= 0)
> +			state->abm_level = ABM_LEVEL_IMMEDIATE_DISABLE;
> +		else
> +			state->abm_level = amdgpu_dm_abm_level;
>   	}
>   }
>   
> 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 3dcedaa67ed8..6146fbc528c3 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
> @@ -437,13 +437,15 @@ static void amdgpu_dm_crtc_reset_state(struct drm_crtc *crtc)
>   {
>   	struct dm_crtc_state *state;
>   
> -	if (crtc->state)
> -		amdgpu_dm_crtc_destroy_state(crtc, crtc->state);
> -
> +	/* Allocate new state first */
>   	state = kzalloc_obj(*state);
>   	if (WARN_ON(!state))
>   		return;
>   
> +	/* Destroy old state only after successful allocation */
> +	if (crtc->state)
> +		amdgpu_dm_crtc_destroy_state(crtc, crtc->state);
> +
>   	__drm_atomic_helper_crtc_reset(crtc, &state->base);
>   }
>   
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> index e957657b06c7..eb1c0a26f20d 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_plane.c
> @@ -1488,17 +1488,17 @@ static const struct drm_plane_helper_funcs dm_primary_plane_helper_funcs = {
>   
>   static void amdgpu_dm_plane_drm_plane_reset(struct drm_plane *plane)
>   {
> -	struct dm_plane_state *amdgpu_state = NULL;
> -
> -	if (plane->state)
> -		plane->funcs->atomic_destroy_state(plane, plane->state);
> +	struct dm_plane_state *amdgpu_state;
>   
> +	/* Allocate new state first */
>   	amdgpu_state = kzalloc_obj(*amdgpu_state);
> -	WARN_ON(amdgpu_state == NULL);
> -
> -	if (!amdgpu_state)
> +	if (WARN_ON(!amdgpu_state))
>   		return;
>   
> +	/* Destroy old state only after successful allocation */
> +	if (plane->state)
> +		plane->funcs->atomic_destroy_state(plane, plane->state);
> +
>   	__drm_atomic_helper_plane_reset(plane, &amdgpu_state->base);
>   	amdgpu_state->degamma_tf = AMDGPU_TRANSFER_FUNCTION_DEFAULT;
>   	amdgpu_state->hdr_mult = AMDGPU_HDR_MULT_DEFAULT;