diff mbox series

[v5,03/19] drm/atomic: Drop drm_private_obj.state assignment from create_state

Message ID 20260519-drm-mode-config-init-v5-3-388b03321e38@kernel.org (mailing list archive)
State New
Headers
Series drm/atomic: Rework initial state allocation |

Commit Message

Maxime Ripard May 19, 2026, 9:01 a.m. UTC 
The initial intent of the atomic_create_state hook was to simply
allocate a proper drm_private_state and return it, without any side
effect.

However, __drm_atomic_helper_private_obj_create_state(), which most
atomic_create_state implementations call, introduces a side effect by
setting drm_private_obj.state to the newly allocated state.

This assignment defeats the purpose, but is also redundant since
drm_atomic_private_obj_init(), the only call site for the
atomic_create_state hook, will also set this pointer to the newly
allocated state.

Drop the assignment in __drm_atomic_helper_private_obj_create_state().

Fixes: e7be39ed1716 ("drm/atomic-helper: Add private_obj atomic_create_state helper")
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 2 --
 1 file changed, 2 deletions(-)

Comments

sashiko-bot@kernel.org May 19, 2026, 11:35 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unbalanced Lifecycle / List Management Error in drm_atomic_private_obj_init() leads to UAF or NULL pointer dereference.
--

commit 44b9224847cfbcc3b1c83e99bfc50096eaebde99
Author: Maxime Ripard <mripard@kernel.org>

drm/atomic: Drop drm_private_obj.state assignment from create_state

The initial intent of the atomic_create_state hook was to simply
allocate a proper drm_private_state and return it, without any side
effect. This patch removes a redundant assignment in
__drm_atomic_helper_private_obj_create_state() that defeats this purpose.

> This assignment defeats the purpose, but is also redundant since
> drm_atomic_private_obj_init(), the only call site for the
> atomic_create_state hook, will also set this pointer to the newly
> allocated state.

This isn't a bug introduced by this patch, but since the commit message
discusses drm_atomic_private_obj_init(), I noticed a potential lifecycle
issue in that function.

In drm_atomic_private_obj_init(), the private object is added to
privobj_list before invoking the atomic_create_state() callback:

drivers/gpu/drm/drm_atomic.c:drm_atomic_private_obj_init() {
	...
	list_add_tail(&obj->head, &dev->mode_config.privobj_list);

	state = obj->funcs->atomic_create_state(obj);
	if (IS_ERR(state))
		return PTR_ERR(state);

	obj->state = state;
	...
}

If atomic_create_state() fails, the function returns an error but fails to
remove the partially initialized object from privobj_list.

Since many callers (e.g., vc4_ctm_obj_init(), drm_bridge_is_atomic(),
drm_dp_mst_topology_mgr_init()) ignore the return value, could this leave
the object in privobj_list with obj->state == NULL?

If this occurs, could the next atomic commit trigger a NULL pointer
dereference when drm_atomic_get_private_obj_state() invokes
atomic_duplicate_state(), and implementations call kmemdup(obj->state, ...)?

Also, if the device is torn down and drm_atomic_private_obj_fini() is called,
it unconditionally executes:

drivers/gpu/drm/drm_atomic.c:drm_atomic_private_obj_fini() {
	...
	obj->funcs->atomic_destroy_state(obj, obj->state);
	...
}

Since obj->state is NULL, would wrappers using container_of() evaluate to a
negative pointer offset, causing a panic when kfree() is called on it?

> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index cc70508d4fdba..a82568d87e4f7 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -731,8 +731,6 @@ void __drm_atomic_helper_private_obj_create_state(struct drm_private_obj *obj,
>  {
>  	if (state)
>  		state->obj = obj;
> -
> -	obj->state = state;
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_private_obj_create_state);
Thomas Zimmermann May 26, 2026, 9:03 a.m. UTC | #2 
Am 19.05.26 um 11:01 schrieb Maxime Ripard:
> The initial intent of the atomic_create_state hook was to simply
> allocate a proper drm_private_state and return it, without any side
> effect.
>
> However, __drm_atomic_helper_private_obj_create_state(), which most
> atomic_create_state implementations call, introduces a side effect by
> setting drm_private_obj.state to the newly allocated state.
>
> This assignment defeats the purpose, but is also redundant since
> drm_atomic_private_obj_init(), the only call site for the
> atomic_create_state hook, will also set this pointer to the newly
> allocated state.
>
> Drop the assignment in __drm_atomic_helper_private_obj_create_state().
>
> Fixes: e7be39ed1716 ("drm/atomic-helper: Add private_obj atomic_create_state helper")
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/drm_atomic_state_helper.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index cc70508d4fdb..a82568d87e4f 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -729,12 +729,10 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_destroy_state);
>   void __drm_atomic_helper_private_obj_create_state(struct drm_private_obj *obj,
>   						  struct drm_private_state *state)
>   {
>   	if (state)
>   		state->obj = obj;
> -
> -	obj->state = state;
>   }
>   EXPORT_SYMBOL(__drm_atomic_helper_private_obj_create_state);
>   
>   /**
>    * __drm_atomic_helper_private_obj_duplicate_state - copy atomic private state
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index cc70508d4fdb..a82568d87e4f 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -729,12 +729,10 @@  EXPORT_SYMBOL(drm_atomic_helper_connector_destroy_state);
 void __drm_atomic_helper_private_obj_create_state(struct drm_private_obj *obj,
 						  struct drm_private_state *state)
 {
 	if (state)
 		state->obj = obj;
-
-	obj->state = state;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_private_obj_create_state);
 
 /**
  * __drm_atomic_helper_private_obj_duplicate_state - copy atomic private state