Re: [PATCH RESEND 45/83] drm/sti: Switch to drm_mode_config_create_initial_state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/sti: late initial state creation and broken error unwind in sti_bind()
--

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

drm/sti: Switch to drm_mode_config_create_initial_state

This commit replaces the call to drm_mode_config_reset() in sti_bind() with
drm_mode_config_create_initial_state() and adds an error path to unregister
the DRM device if initial state creation fails.

> diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
> index d02d71866f2a9..0ca6c6b41f18d 100644
> --- a/drivers/gpu/drm/sti/sti_drv.c
> +++ b/drivers/gpu/drm/sti/sti_drv.c
> @@ -202,12 +202,16 @@ static int sti_bind(struct device *dev)
>  	if (ret)
>  		goto err_cleanup;
>  
> -	drm_mode_config_reset(ddev);
> +	ret = drm_mode_config_create_initial_state(ddev);
> +	if (ret)
> +		goto err_unregister;
>  
>  	drm_client_setup(ddev, NULL);
>  
>  	return 0;
>  
> +err_unregister:
> +	drm_dev_unregister(ddev);
>  err_cleanup:
>  	sti_cleanup(ddev);

[Severity: High]
If drm_mode_config_create_initial_state() fails in sti_bind() (for example,
when plane or CRTC state allocation fails with -ENOMEM), jumping to
err_unregister and falling through to err_cleanup calls sti_cleanup():

drivers/gpu/drm/sti/sti_drv.c:sti_cleanup() {
	...
	drm_kms_helper_poll_fini(ddev);
	drm_atomic_helper_shutdown(ddev);
	drm_mode_config_cleanup(ddev);
	...
}

Because crtc->state is still NULL when initial state creation fails before
allocating CRTC states (or when drm_dev_register() fails before initial state
creation is reached), does calling
sti_cleanup() -> drm_atomic_helper_shutdown() ->
drm_atomic_helper_disable_all() -> drm_atomic_get_crtc_state() ->
drm_atomic_helper_crtc_duplicate_state() trigger a WARN_ON(!crtc->state) and
cause drm_atomic_helper_shutdown() to fail?

In addition, in sti_bind(), drm_dev_register() is called right before
drm_mode_config_create_initial_state():

drivers/gpu/drm/sti/sti_drv.c:sti_bind() {
	...
	ret = drm_dev_register(ddev, 0);
	if (ret)
		goto err_cleanup;

	ret = drm_mode_config_create_initial_state(ddev);
	if (ret)
		goto err_unregister;
	...
}

Since drm_dev_register() exposes the DRM device nodes and connector sysfs
attributes to userspace while plane->state, crtc->state, and
connector->state are still NULL, and drm_mode_config_create_initial_state()
subsequently populates those state pointers without holding modeset locks,
can a userspace process opening the device right after registration trigger
a NULL pointer dereference in drm_mode_object_get_properties() ->
drm_atomic_get_property()?

Should drm_mode_config_create_initial_state() be called before
drm_dev_register(), and should drm_atomic_helper_shutdown() be avoided on
error paths where initial modeset state has not been fully created?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-drm-no-more-config-reset-v1-0-1f58d1e33380@kernel.org?part=45