From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-19.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_GIT autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 32D71C433F5 for ; Thu, 16 Sep 2021 17:30:11 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1DA64604E9 for ; Thu, 16 Sep 2021 17:30:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1347203AbhIPRb2 (ORCPT ); Thu, 16 Sep 2021 13:31:28 -0400 Received: from mail.kernel.org ([198.145.29.99]:44396 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1344020AbhIPRWE (ORCPT ); Thu, 16 Sep 2021 13:22:04 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id A03ED61BB6; Thu, 16 Sep 2021 16:42:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1631810578; bh=LIccbwUMCsKxYmS9YFvN1aYFghjMDZmQ3kO8FQM++28=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=wBuPIp5sXhUeaCLkp5H9a6duLdELooHGcF3fU85iWm4B5EIhf8BIg+iTCiy0cGSyO M1gUK3GLn65buD4Qv75+2/dc8Tz+N0T1Y0ryBIb18owckZMCSdGDt3b9edRu2WxLVc XpvNQQMewy+JcOnpcR3caW0Y7hyJe8/rKiyUrbbA= From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, Daniel Vetter , Desmond Cheong Zhi Xi , Emil Velikov , Sasha Levin Subject: [PATCH 5.14 191/432] drm: protect drm_master pointers in drm_lease.c Date: Thu, 16 Sep 2021 17:59:00 +0200 Message-Id: <20210916155817.231792360@linuxfoundation.org> X-Mailer: git-send-email 2.33.0 In-Reply-To: <20210916155810.813340753@linuxfoundation.org> References: <20210916155810.813340753@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: Desmond Cheong Zhi Xi [ Upstream commit 56f0729a510f92151682ff6c89f69724d5595d6e ] drm_file->master pointers should be protected by drm_device.master_mutex or drm_file.master_lookup_lock when being dereferenced. However, in drm_lease.c, there are multiple instances where drm_file->master is accessed and dereferenced while neither lock is held. This makes drm_lease.c vulnerable to use-after-free bugs. We address this issue in 2 ways: 1. Add a new drm_file_get_master() function that calls drm_master_get on drm_file->master while holding on to drm_file.master_lookup_lock. Since drm_master_get increments the reference count of master, this prevents master from being freed until we unreference it with drm_master_put. 2. In each case where drm_file->master is directly accessed and eventually dereferenced in drm_lease.c, we wrap the access in a call to the new drm_file_get_master function, then unreference the master pointer once we are done using it. Reported-by: Daniel Vetter Signed-off-by: Desmond Cheong Zhi Xi Reviewed-by: Emil Velikov Signed-off-by: Daniel Vetter Link: https://patchwork.freedesktop.org/patch/msgid/20210712043508.11584-6-desmondcheongzx@gmail.com Signed-off-by: Sasha Levin --- drivers/gpu/drm/drm_auth.c | 25 ++++++++++++ drivers/gpu/drm/drm_lease.c | 81 ++++++++++++++++++++++++++++--------- include/drm/drm_auth.h | 1 + include/drm/drm_file.h | 6 +++ 4 files changed, 93 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index cbb896b91d94..3a298df00901 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -377,6 +377,31 @@ struct drm_master *drm_master_get(struct drm_master *master) } EXPORT_SYMBOL(drm_master_get); +/** + * drm_file_get_master - reference &drm_file.master of @file_priv + * @file_priv: DRM file private + * + * Increments the reference count of @file_priv's &drm_file.master and returns + * the &drm_file.master. If @file_priv has no &drm_file.master, returns NULL. + * + * Master pointers returned from this function should be unreferenced using + * drm_master_put(). + */ +struct drm_master *drm_file_get_master(struct drm_file *file_priv) +{ + struct drm_master *master = NULL; + + spin_lock(&file_priv->master_lookup_lock); + if (!file_priv->master) + goto unlock; + master = drm_master_get(file_priv->master); + +unlock: + spin_unlock(&file_priv->master_lookup_lock); + return master; +} +EXPORT_SYMBOL(drm_file_get_master); + static void drm_master_destroy(struct kref *kref) { struct drm_master *master = container_of(kref, struct drm_master, refcount); diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index 00fb433bcef1..92eac73d9001 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -106,10 +106,19 @@ static bool _drm_has_leased(struct drm_master *master, int id) */ bool _drm_lease_held(struct drm_file *file_priv, int id) { - if (!file_priv || !file_priv->master) + bool ret; + struct drm_master *master; + + if (!file_priv) return true; - return _drm_lease_held_master(file_priv->master, id); + master = drm_file_get_master(file_priv); + if (!master) + return true; + ret = _drm_lease_held_master(master, id); + drm_master_put(&master); + + return ret; } /** @@ -128,13 +137,22 @@ bool drm_lease_held(struct drm_file *file_priv, int id) struct drm_master *master; bool ret; - if (!file_priv || !file_priv->master || !file_priv->master->lessor) + if (!file_priv) return true; - master = file_priv->master; + master = drm_file_get_master(file_priv); + if (!master) + return true; + if (!master->lessor) { + ret = true; + goto out; + } mutex_lock(&master->dev->mode_config.idr_mutex); ret = _drm_lease_held_master(master, id); mutex_unlock(&master->dev->mode_config.idr_mutex); + +out: + drm_master_put(&master); return ret; } @@ -154,10 +172,16 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) int count_in, count_out; uint32_t crtcs_out = 0; - if (!file_priv || !file_priv->master || !file_priv->master->lessor) + if (!file_priv) return crtcs_in; - master = file_priv->master; + master = drm_file_get_master(file_priv); + if (!master) + return crtcs_in; + if (!master->lessor) { + crtcs_out = crtcs_in; + goto out; + } dev = master->dev; count_in = count_out = 0; @@ -176,6 +200,9 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) count_in++; } mutex_unlock(&master->dev->mode_config.idr_mutex); + +out: + drm_master_put(&master); return crtcs_out; } @@ -489,7 +516,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, size_t object_count; int ret = 0; struct idr leases; - struct drm_master *lessor = lessor_priv->master; + struct drm_master *lessor; struct drm_master *lessee = NULL; struct file *lessee_file = NULL; struct file *lessor_file = lessor_priv->filp; @@ -501,12 +528,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP; - /* Do not allow sub-leases */ - if (lessor->lessor) { - DRM_DEBUG_LEASE("recursive leasing not allowed\n"); - return -EINVAL; - } - /* need some objects */ if (cl->object_count == 0) { DRM_DEBUG_LEASE("no objects in lease\n"); @@ -518,12 +539,22 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, return -EINVAL; } + lessor = drm_file_get_master(lessor_priv); + /* Do not allow sub-leases */ + if (lessor->lessor) { + DRM_DEBUG_LEASE("recursive leasing not allowed\n"); + ret = -EINVAL; + goto out_lessor; + } + object_count = cl->object_count; object_ids = memdup_user(u64_to_user_ptr(cl->object_ids), array_size(object_count, sizeof(__u32))); - if (IS_ERR(object_ids)) - return PTR_ERR(object_ids); + if (IS_ERR(object_ids)) { + ret = PTR_ERR(object_ids); + goto out_lessor; + } idr_init(&leases); @@ -534,14 +565,15 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, if (ret) { DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret); idr_destroy(&leases); - return ret; + goto out_lessor; } /* Allocate a file descriptor for the lease */ fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK)); if (fd < 0) { idr_destroy(&leases); - return fd; + ret = fd; + goto out_lessor; } DRM_DEBUG_LEASE("Creating lease\n"); @@ -577,6 +609,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, /* Hook up the fd */ fd_install(fd, lessee_file); + drm_master_put(&lessor); DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n"); return 0; @@ -586,6 +619,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, out_leases: put_unused_fd(fd); +out_lessor: + drm_master_put(&lessor); DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret); return ret; } @@ -608,7 +643,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, struct drm_mode_list_lessees *arg = data; __u32 __user *lessee_ids = (__u32 __user *) (uintptr_t) (arg->lessees_ptr); __u32 count_lessees = arg->count_lessees; - struct drm_master *lessor = lessor_priv->master, *lessee; + struct drm_master *lessor, *lessee; int count; int ret = 0; @@ -619,6 +654,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP; + lessor = drm_file_get_master(lessor_priv); DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id); mutex_lock(&dev->mode_config.idr_mutex); @@ -642,6 +678,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, arg->count_lessees = count; mutex_unlock(&dev->mode_config.idr_mutex); + drm_master_put(&lessor); return ret; } @@ -661,7 +698,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, struct drm_mode_get_lease *arg = data; __u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr); __u32 count_objects = arg->count_objects; - struct drm_master *lessee = lessee_priv->master; + struct drm_master *lessee; struct idr *object_idr; int count; void *entry; @@ -675,6 +712,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP; + lessee = drm_file_get_master(lessee_priv); DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id); mutex_lock(&dev->mode_config.idr_mutex); @@ -702,6 +740,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, arg->count_objects = count; mutex_unlock(&dev->mode_config.idr_mutex); + drm_master_put(&lessee); return ret; } @@ -720,7 +759,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, void *data, struct drm_file *lessor_priv) { struct drm_mode_revoke_lease *arg = data; - struct drm_master *lessor = lessor_priv->master; + struct drm_master *lessor; struct drm_master *lessee; int ret = 0; @@ -730,6 +769,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP; + lessor = drm_file_get_master(lessor_priv); mutex_lock(&dev->mode_config.idr_mutex); lessee = _drm_find_lessee(lessor, arg->lessee_id); @@ -750,6 +790,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, fail: mutex_unlock(&dev->mode_config.idr_mutex); + drm_master_put(&lessor); return ret; } diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h index 6bf8b2b78991..f99d3417f304 100644 --- a/include/drm/drm_auth.h +++ b/include/drm/drm_auth.h @@ -107,6 +107,7 @@ struct drm_master { }; struct drm_master *drm_master_get(struct drm_master *master); +struct drm_master *drm_file_get_master(struct drm_file *file_priv); void drm_master_put(struct drm_master **master); bool drm_is_current_master(struct drm_file *fpriv); diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 9b82988e3427..726cfe0ff5f5 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -233,6 +233,12 @@ struct drm_file { * this only matches &drm_device.master if the master is the currently * active one. * + * When dereferencing this pointer, either hold struct + * &drm_device.master_mutex for the duration of the pointer's use, or + * use drm_file_get_master() if struct &drm_device.master_mutex is not + * currently held and there is no other need to hold it. This prevents + * @master from being freed during use. + * * See also @authentication and @is_master and the :ref:`section on * primary nodes and authentication `. */ -- 2.30.2