Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6AC26C433EF for ; Wed, 24 Nov 2021 13:41:26 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1350612AbhKXNod (ORCPT ); Wed, 24 Nov 2021 08:44:33 -0500 Received: from mail.kernel.org ([198.145.29.99]:32918 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1353381AbhKXNmB (ORCPT ); Wed, 24 Nov 2021 08:42:01 -0500 Received: by mail.kernel.org (Postfix) with ESMTPSA id 8A80163245; Wed, 24 Nov 2021 12:57:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1637758680; bh=dtjZJbxr3hLbtzNJc9/6bVILUTPwX5vFAIYsC7SVm6Q=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=CEzBUDkpXvhWcppnP5Lh8Z0nr+0O28BEnqIaP3cpUQx5b7Y2+CCj0OSz/ZiH9439M eUpHen7e0f4TrFLWsOYhBDZOG/QukJG6ZkC7pNsMLnwJU7+OWVewyfQdJy3hXNnncg Rm/g1mVbhtiodghe/zLgZ/RSfjiJOuiLxkkktaTs= From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, Jeremy Cline , Lyude Paul , Ben Skeggs , Karol Herbst Subject: [PATCH 5.10 141/154] drm/nouveau: clean up all clients on device removal Date: Wed, 24 Nov 2021 12:58:57 +0100 Message-Id: <20211124115707.050459844@linuxfoundation.org> X-Mailer: git-send-email 2.34.0 In-Reply-To: <20211124115702.361983534@linuxfoundation.org> References: <20211124115702.361983534@linuxfoundation.org> User-Agent: quilt/0.66 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Jeremy Cline commit f55aaf63bde0d0336c3823bb3713bd4a464abbcf upstream. 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 Reviewed-by: Lyude Paul Reviewed-by: Ben Skeggs Tested-by: Karol Herbst Signed-off-by: Karol Herbst Link: https://patchwork.freedesktop.org/patch/msgid/20201125202648.5220-4-jcline@redhat.com Link: https://gitlab.freedesktop.org/drm/nouveau/-/merge_requests/14 Signed-off-by: Greg Kroah-Hartman --- drivers/gpu/drm/nouveau/nouveau_drm.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -628,6 +628,7 @@ fail_alloc: static void nouveau_drm_device_fini(struct drm_device *dev) { + struct nouveau_cli *cli, *temp_cli; struct nouveau_drm *drm = nouveau_drm(dev); if (nouveau_pmops_runtime()) { @@ -652,6 +653,24 @@ nouveau_drm_device_fini(struct drm_devic nouveau_ttm_fini(drm); nouveau_vga_fini(drm); + /* + * There may be existing clients from as-yet unclosed files. For now, + * clean them up here rather than deferring until the file is closed, + * but this likely not correct if we want to support hot-unplugging + * properly. + */ + mutex_lock(&drm->clients_lock); + list_for_each_entry_safe(cli, temp_cli, &drm->clients, head) { + list_del(&cli->head); + mutex_lock(&cli->mutex); + if (cli->abi16) + nouveau_abi16_fini(cli->abi16); + mutex_unlock(&cli->mutex); + nouveau_cli_fini(cli); + kfree(cli); + } + mutex_unlock(&drm->clients_lock); + nouveau_cli_fini(&drm->client); nouveau_cli_fini(&drm->master); nvif_parent_dtor(&drm->parent); @@ -1108,6 +1127,16 @@ nouveau_drm_postclose(struct drm_device { struct nouveau_cli *cli = nouveau_cli(fpriv); struct nouveau_drm *drm = nouveau_drm(dev); + int dev_index; + + /* + * The device is gone, and as it currently stands all clients are + * cleaned up in the removal codepath. In the future this may change + * so that we can support hot-unplugging, but for now we immediately + * return to avoid a double-free situation. + */ + if (!drm_dev_enter(dev, &dev_index)) + return; pm_runtime_get_sync(dev->dev); @@ -1124,6 +1153,7 @@ nouveau_drm_postclose(struct drm_device kfree(cli); pm_runtime_mark_last_busy(dev->dev); pm_runtime_put_autosuspend(dev->dev); + drm_dev_exit(dev_index); } static const struct drm_ioctl_desc