Re: [PATCH v4 3/5] drm/nouveau: Fix drm_driver struct/nouveau.atomic parameter handling

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Embedding `drm_driver` in `struct nouveau_drm` introduces an exploitable Use-After-Free vulnerability when the device is unplugged or unbound while userspace holds open file descriptors.
--

--- Patch [3]: [PATCH v4 3/5] drm/nouveau: Fix drm_driver struct/nouveau.atomic parameter handling ---
commit 6aa4a539bc58f0cd546203fa854c1edf2120c1be
Author: Lyude Paul <[email protected]>

drm/nouveau: Fix drm_driver struct/nouveau.atomic parameter handling

This commit moves the drm_driver struct from a static global variable
into the dynamically allocated nouveau_drm structure. It also handles
conditionally enabling atomic modesetting per-device.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> @@ -216,6 +216,7 @@ struct nouveau_drm {
>  
>  	struct nouveau_cli client;
>  	struct drm_device *dev;
> +	struct drm_driver drm_driver;
>  
>  	struct list_head clients;

[ ... ]

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
[ ... ]
> @@ -744,12 +741,14 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
>  	if (!drm)
>  		return ERR_PTR(-ENOMEM);
>  
> +	drm->drm_driver = driver_stub;
>  	drm->nvkm = device;
>  
> -	drm->dev = drm_dev_alloc(drm_driver, parent);
> +	drm->dev = drm_dev_alloc(&drm->drm_driver, parent);

[Severity: High]
Does embedding drm_driver inside nouveau_drm create a use-after-free
risk if the device is unplugged or unbound while userspace holds an open
file descriptor?

If a user opens the DRM device node, the open file descriptor holds a
reference to the drm_device. If the device is then hot-unplugged or
unbound via sysfs, nouveau_drm_device_del() will unconditionally free
the nouveau_drm struct:

nouveau_drm_device_del()
    ...
    kfree(drm);

However, the drm_device remains alive due to the file descriptor's
reference. When the user later closes the file descriptor, the DRM core
will dereference the dangling dev->driver pointer:

drm_file_free()
    ...
    if (dev->driver->postclose)
        dev->driver->postclose(dev, file);

Could this lead to memory corruption since the drm_driver struct was
already freed?

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