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
