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

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

New issues:
- [High] Embedding the `drm_driver` struct inside the dynamically allocated `nouveau_drm` struct causes a Use-After-Free when the device is unplugged but userspace holds open DRM file descriptors.

Pre-existing issues:
- [Critical] nouveau_drm_device_new() incorrectly returns 0 (NULL) on success, causing an immediate kernel panic during device probe.
--

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

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

The way we handled the nouveau.atomic module parameter before was fairly
broken, and had a number of issues:

[ ... ]

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 6940e774ace07..95daf2643e1df 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
[ ... ]
> @@ -727,8 +725,7 @@ nouveau_drm_device_del(struct nouveau_drm *drm)
>  }
>  
>  static struct nouveau_drm *
> -nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *parent,
> -		       struct nvkm_device *device)
> +nouveau_drm_device_new(struct device *parent, struct nvkm_device *device)
>  {

[Severity: Critical]
This is a pre-existing issue (introduced earlier in the series), but does
nouveau_drm_device_new() incorrectly return 0 (NULL) on success?

At the end of nouveau_drm_device_new(), it returns 0:

	return 0;

err_free_drm:

This causes callers like nouveau_drm_probe() to incorrectly bypass the
IS_ERR() check, as IS_ERR(NULL) is false:

nouveau_drm_probe()
	drm = nouveau_drm_device_new(&pdev->dev, device);
	if (IS_ERR(drm)) {
		ret = PTR_ERR(drm);
		goto fail_nvkm;
	}

	ret = pci_enable_device(pdev);
	if (ret)
		goto fail_drm;

	ret = nouveau_drm_device_init(drm);

The NULL pointer is then passed to nouveau_drm_device_init(), which
immediately dereferences it and causes a kernel panic during device probe.

[ ... ]
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> index 5fc75dc750ed0..5cc0001134e74 100644
> --- 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;

[Severity: High]
Can embedding the drm_driver struct inside the dynamically allocated
nouveau_drm struct cause a use-after-free when the device is unplugged?

When the device is unbound, nouveau_drm_device_del() drops the driver's
reference to the DRM device and then immediately frees the nouveau_drm struct
using kfree():

drivers/gpu/drm/nouveau/nouveau_drm.c:nouveau_drm_device_del() {
	if (drm->dev)
		drm_dev_put(drm->dev);
	...
	kfree(drm);
}

Because drm_driver is now embedded in nouveau_drm, the drm_driver struct is
also freed here.

If userspace still has an open file descriptor, the drm_device remains alive.
When userspace later closes the file descriptor, DRM core functions (such as
drm_release) will attempt to dereference dev->driver (e.g.,
dev->driver->postclose), resulting in a use-after-free.

>  
>  	struct list_head clients;
>

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