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 <[email protected]> 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/[email protected]?part=45
