Re: [PATCH v5 16/19] drm: fix race between partial drm_dev_register() failure and ioctl

[sashiko-bot@kernel.org]

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 <dakr@kernel.org>

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/20260620190648.2E9F61F000E9@smtp.kernel.org/

> 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/20260628145406.2107056-1-dakr@kernel.org?part=16