Skip to content

Commit f55aaf6

Browse files
jeremyclinekarolherbst
authored andcommitted
drm/nouveau: clean up all clients on device removal
The postclose handler can run after the device has been removed (or the driver has been unbound) since userspace clients are free to hold the file open as long as they want. Because the device removal callback frees the entire nouveau_drm structure, any reference to it in the postclose handler will result in a use-after-free. To reproduce this, one must simply open the device file, unbind the driver (or physically remove the device), and then close the device file. This was found and can be reproduced easily with the IGT core_hotunplug tests. To avoid this, all clients are cleaned up in the device finalization rather than deferring it to the postclose handler, and the postclose handler is protected by a critical section which ensures the drm_dev_unplug() and the postclose handler won't race. This is not an ideal fix, since as I understand the proposed plan for the kernel<->userspace interface for hotplug support, destroying the client before the file is closed will cause problems. However, I believe to properly fix this issue, the lifetime of the nouveau_drm structure needs to be extended to match the drm_device, and this proved to be a rather invasive change. Thus, I've broken this out so the fix can be easily backported. This fixes with the two previous commits CVE-2020-27820 (Karol). Cc: stable@vger.kernel.org # 5.4+ Signed-off-by: Jeremy Cline <jcline@redhat.com> Reviewed-by: Lyude Paul <lyude@redhat.com> Reviewed-by: Ben Skeggs <bskeggs@redhat.com> Tested-by: Karol Herbst <kherbst@redhat.com> Signed-off-by: Karol Herbst <kherbst@redhat.com> Link: https://patchwork.freedesktop.org/patch/msgid/20201125202648.5220-4-jcline@redhat.com Link: https://gitlab.freedesktop.org/drm/nouveau/-/merge_requests/14
1 parent abae916 commit f55aaf6

1 file changed

Lines changed: 30 additions & 0 deletions

File tree

drivers/gpu/drm/nouveau/nouveau_drm.c

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -633,6 +633,7 @@ nouveau_drm_device_init(struct drm_device *dev)
633633
static void
634634
nouveau_drm_device_fini(struct drm_device *dev)
635635
{
636+
struct nouveau_cli *cli, *temp_cli;
636637
struct nouveau_drm *drm = nouveau_drm(dev);
637638

638639
if (nouveau_pmops_runtime()) {
@@ -657,6 +658,24 @@ nouveau_drm_device_fini(struct drm_device *dev)
657658
nouveau_ttm_fini(drm);
658659
nouveau_vga_fini(drm);
659660

661+
/*
662+
* There may be existing clients from as-yet unclosed files. For now,
663+
* clean them up here rather than deferring until the file is closed,
664+
* but this likely not correct if we want to support hot-unplugging
665+
* properly.
666+
*/
667+
mutex_lock(&drm->clients_lock);
668+
list_for_each_entry_safe(cli, temp_cli, &drm->clients, head) {
669+
list_del(&cli->head);
670+
mutex_lock(&cli->mutex);
671+
if (cli->abi16)
672+
nouveau_abi16_fini(cli->abi16);
673+
mutex_unlock(&cli->mutex);
674+
nouveau_cli_fini(cli);
675+
kfree(cli);
676+
}
677+
mutex_unlock(&drm->clients_lock);
678+
660679
nouveau_cli_fini(&drm->client);
661680
nouveau_cli_fini(&drm->master);
662681
nvif_parent_dtor(&drm->parent);
@@ -1112,6 +1131,16 @@ nouveau_drm_postclose(struct drm_device *dev, struct drm_file *fpriv)
11121131
{
11131132
struct nouveau_cli *cli = nouveau_cli(fpriv);
11141133
struct nouveau_drm *drm = nouveau_drm(dev);
1134+
int dev_index;
1135+
1136+
/*
1137+
* The device is gone, and as it currently stands all clients are
1138+
* cleaned up in the removal codepath. In the future this may change
1139+
* so that we can support hot-unplugging, but for now we immediately
1140+
* return to avoid a double-free situation.
1141+
*/
1142+
if (!drm_dev_enter(dev, &dev_index))
1143+
return;
11151144

11161145
pm_runtime_get_sync(dev->dev);
11171146

@@ -1128,6 +1157,7 @@ nouveau_drm_postclose(struct drm_device *dev, struct drm_file *fpriv)
11281157
kfree(cli);
11291158
pm_runtime_mark_last_busy(dev->dev);
11301159
pm_runtime_put_autosuspend(dev->dev);
1160+
drm_dev_exit(dev_index);
11311161
}
11321162

11331163
static const struct drm_ioctl_desc

0 commit comments

Comments
 (0)