Re: [PATCH v5 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:
- [Critical] Embedding `drm_driver` inside `nouveau_drm` causes a Use-After-Free of `dev->driver` in the DRM core when the device is unbound or hot-unplugged while userspace holds an open file descriptor.

Pre-existing issues:
- [Critical] The `NOUVEAU_NVIF` ioctl bypasses DRM core unplug checks, leading to a Use-After-Free on `nouveau_cli` if executed after the device is removed.
--

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

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

This commit moves the global drm_driver structures for PCI and platform
devices into the nouveau_drm struct. It initializes the embedded driver
struct by copying a const stub and dynamically adds the atomic modesetting
capability.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 451009adee0de..9cd12ebc7e449 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
[ ... ]
> @@ -744,9 +741,10 @@ 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: Critical]
Does embedding the drm_driver inside the nouveau_drm struct cause a
use-after-free of dev->driver during device unbind?

When the device is unbound via sysfs or hot-unplugged, the driver's remove
routine calls nouveau_drm_device_del(), which drops its reference to
drm_device (via drm_dev_put()) and unconditionally frees the nouveau_drm
structure, along with the embedded drm_driver.

However, userspace might still hold an open file descriptor, keeping the
drm_device alive. Later, when the file is closed, the DRM core's
drm_file_free() dereferences dev->driver->postclose:

drivers/gpu/drm/drm_file.c:drm_file_free() {
    ...
    if (dev->driver->postclose)
        dev->driver->postclose(dev, file);
    ...
}

Since dev->driver points to the freed embedded structure, does this result
in a use-after-free?


[Severity: Critical]
This is a pre-existing issue, but does the NOUVEAU_NVIF ioctl bypass DRM
core unplug checks and cause a use-after-free on nouveau_cli?

In nouveau_drm_ioctl(), this specific command routes directly to the ABI16
handler, bypassing drm_ioctl() and its drm_dev_is_unplugged() check:

drivers/gpu/drm/nouveau/nouveau_drm.c:nouveau_drm_ioctl() {
    ...
    case DRM_NOUVEAU_NVIF:
        ret = nouveau_abi16_ioctl(filp, (void __user *)arg, _IOC_SIZE(cmd));
        break;
    ...
}

If the device is unbound or unplugged, nouveau_drm_device_fini() iterates
over drm->clients and frees all cli structures:

drivers/gpu/drm/nouveau/nouveau_drm.c:nouveau_drm_device_fini() {
    ...
    list_for_each_entry_safe(cli, temp_cli, &drm->clients, head) {
        list_del(&cli->head);
        ...
        kfree(cli);
    }
    ...
}

If a userspace process then calls the DRM_NOUVEAU_NVIF ioctl,
nouveau_abi16_ioctl() calls nouveau_abi16_get(), which retrieves the freed
cli struct from file->driver_priv and attempts to lock its mutex:

drivers/gpu/drm/nouveau/nouveau_abi16.c:nouveau_abi16_get() {
    ...
    struct nouveau_cli *cli = nouveau_cli(file_priv);
    mutex_lock(&cli->mutex);
    ...
}

Can this lead to memory corruption if executed after the device is removed?

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