Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] drm_dev_register() fails to unmap CPU memory mappings on the error path, allowing userspace to retain access to physical memory that is subsequently freed. - [High] drm_dev_synchronize_unplug() fails to protect against concurrent drm_open() calls, leaving dev->driver->open() vulnerable to use-after-free. -- commit e64c045955a50d4e3b92080a95ee364a79c625f5 Author: Danilo Krummrich <[email protected]> drm: fix race between partial drm_dev_register() failure and ioctl This commit introduces drm_dev_synchronize_unplug() to fix a race where userspace could enter a drm_dev_enter() critical section during a partial drm_dev_register() failure. It calls the new function on the error path to ensure all in-flight critical sections complete before cleanup proceeds. Closes: https://lore.kernel.org/all/[email protected]/ > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c [ ... ] > @@ -473,6 +473,22 @@ void drm_dev_exit(int idx) > } > EXPORT_SYMBOL(drm_dev_exit); > > +/* > + * Mark the device as unplugged and wait for any in-flight drm_dev_enter() > + * critical sections to complete. > + */ > +static void drm_dev_synchronize_unplug(struct drm_device *dev) > +{ > + /* > + * After synchronizing any critical read section is guaranteed to see > + * the new value of ->unplugged, and any critical section which might > + * still have seen the old value of ->unplugged is guaranteed to have > + * finished. > + */ > + dev->unplugged = true; > + synchronize_srcu(&drm_unplug_srcu); > +} [Severity: High] This is a pre-existing issue, but does this function completely protect against concurrent drm_open() calls? Looking at drm_open() in drivers/gpu/drm/drm_file.c, it checks the unplugged flag via drm_minor_acquire() without entering the SRCU critical section: minor = drm_minor_acquire(&drm_minors_xa, iminor(inode)); if (IS_ERR(minor)) return PTR_ERR(minor); Could a thread calling open() read dev->unplugged == false just before this function sets it to true? Since drm_open() doesn't hold the SRCU read lock, synchronize_srcu() would return immediately, allowing the teardown to proceed while dev->driver->open() executes concurrently. [ ... ] > @@ -1118,6 +1127,13 @@ int drm_dev_register(struct drm_device *dev, unsigned > long flags) > if (dev->driver->unload) > dev->driver->unload(dev); > err_minors: > + /* > + * If a minor was registered before the failure, userspace could have > + * opened it and entered a drm_dev_enter() critical section. Ensure all > + * such sections complete before we clean up. > + */ > + drm_dev_synchronize_unplug(dev); > + > remove_compat_control_link(dev); > drm_minor_unregister(dev, DRM_MINOR_ACCEL); > drm_minor_unregister(dev, DRM_MINOR_PRIMARY); [Severity: High] This isn't a bug introduced by this patch, but is there a missing call to unmap_mapping_range() on this error path? If a minor was successfully registered before the failure, userspace could have opened it and mapped a GEM object. In drm_dev_unplug(), we unmap the CPU mappings: drm_dev_synchronize_unplug(dev); drm_dev_unregister(dev); unmap_mapping_range(dev->anon_inode->i_mapping, 0, 0, 1); Does omitting this in the err_minors path in drm_dev_register() allow userspace to retain access to physical memory that devres will subsequently free? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=16
