Re: [PATCH] drm: reset dev->registered on drm_dev_register() error path

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Userspace can open the DRM minor device before probe completes, leading to UAF if probe fails.
--

--- Patch [1]: [PATCH] drm: reset dev->registered on drm_dev_register() error path ---
commit 4863f2824dd190d908ed2dadca48efc26b615e3b
Author: Shixiong Ou <[email protected]>

drm: reset dev->registered on drm_dev_register() error path
    
This commit ensures dev->registered is reset to false on the error path
in drm_dev_register(). This prevents a spurious WARN_ON during teardown
of drmm-managed resources after a failed probe.

> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 1ff0bf7cba6aa..4497af8c9d1c6 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -1120,6 +1120,7 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
>  	if (dev->driver->unload)
>  		dev->driver->unload(dev);
>  err_minors:
> +	dev->registered = false;

[Severity: High]
This isn't a bug introduced by this patch, but while looking at the error
path here, can userspace open the DRM minor device before probe completes and
trigger a use-after-free if probe fails?

In drm_dev_register(), the device node is exposed to userspace early:

drm_dev_register() {
    ...
    ret = drm_minor_register(dev, DRM_MINOR_PRIMARY);
    ...
}

If a subsequent step fails (like drm_modeset_register_all), we unwind here,
but what happens if userspace already opened the device?

The drm_device refcount prevents the structure from being freed, but the
driver probe returns an error, triggering devres cleanup of MMIO and
driver-private data.

Because the DRM core doesn't set dev->unplugged = true on this path, and 
drm_ioctl() doesn't check dev->registered:

drm_ioctl() {
    if (drm_dev_is_unplugged(dev))
        return -ENODEV;
    ...
}

Does this allow userspace to continue issuing ioctls to the torn-down 
hardware?

>  	remove_compat_control_link(dev);
>  	drm_minor_unregister(dev, DRM_MINOR_ACCEL);
>  	drm_minor_unregister(dev, DRM_MINOR_PRIMARY);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1