diff mbox series

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

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

Commit Message

Maxime Ripard May 26, 2026, 4:46 p.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>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
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 26, 2026, 5:30 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Improper error handling in `drm_atomic_private_obj_init()` leaves partially initialized objects on the global list, leading to a NULL pointer dereference.
--

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

drm/atomic: Drop drm_private_obj.state assignment from create_state

This commit removes the assignment of the newly allocated state to
drm_private_obj.state inside __drm_atomic_helper_private_obj_create_state().
This assignment is redundant because drm_atomic_private_obj_init() already
sets this pointer after calling the atomic_create_state hook.

> 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;

[Severity: High]
This isn't a bug introduced by this patch, but since we are discussing
the obj->state assignment here and deferring it to
drm_atomic_private_obj_init(), could this lead to a NULL pointer dereference
if drm_atomic_private_obj_init() fails?

Looking at 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 the error immediately
without removing obj->head from the list. This leaves obj->state as NULL.

Multiple drivers (such as drm_dp_mst_topology_mgr_init() or
vc4_ctm_obj_init()) ignore the return value of drm_atomic_private_obj_init()
and continue initialization, meaning the device successfully registers
despite the failure.

Once registered, reading /sys/kernel/debug/dri/X/state triggers
drm_state_dump(), which iterates over privobj_list and calls
drm_atomic_private_obj_print_state(p, obj->state).

Because obj->state is NULL, drm_atomic_private_obj_print_state() will
immediately dereference it (struct drm_private_obj *obj = state->obj;),
causing a kernel oops that can be triggered by userspace from a diagnostic
read.

>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_private_obj_create_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