linux-kernel - [PATCH] drm/panfrost: Fix GEM handle creation UAF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <20221216233355.542197-1-robdclark@gmail.com>
Date:   Fri, 16 Dec 2022 15:33:54 -0800
From:   Rob Clark <robdclark@...il.com>
To:     dri-devel@...ts.freedesktop.org
Cc:     Rob Clark <robdclark@...omium.org>, Rob Herring <robh@...nel.org>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        Steven Price <steven.price@....com>,
        Alyssa Rosenzweig <alyssa.rosenzweig@...labora.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        linux-kernel@...r.kernel.org (open list)
Subject: [PATCH] drm/panfrost: Fix GEM handle creation UAF

From: Rob Clark <robdclark@...omium.org>

Relying on an unreturned handle to hold a reference to an object we
dereference is not safe.  Userspace can guess the handle and race us
by closing the handle from another thread.  The _create_with_handle()
that returns an object ptr is pretty much a pattern to avoid.  And
ideally creating the handle would be done after any needed dererencing.
But in this case creation of the mapping is tied to the handle creation.
Fortunately the mapping is refcnt'd and holds a reference to the object,
so we can drop the handle's reference once we hold a mapping reference.

Signed-off-by: Rob Clark <robdclark@...omium.org>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c |  7 +++++++
 drivers/gpu/drm/panfrost/panfrost_gem.c | 10 +++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 2fa5afe21288..aa5848de647c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -98,6 +98,13 @@ static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
 		return PTR_ERR(bo);
 
 	mapping = panfrost_gem_mapping_get(bo, priv);
+
+	/*
+	 * Now that the mapping holds a reference to the bo until we no longer
+	 * need it, we can safely drop the handle's reference.
+	 */
+	drm_gem_object_put(&bo->base.base);
+
 	if (!mapping) {
 		drm_gem_object_put(&bo->base.base);
 		return -EINVAL;
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 293e799e2fe8..e3e21c500d24 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -234,6 +234,10 @@ struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t
 	return &obj->base.base;
 }
 
+/*
+ * NOTE: if this succeeds, both the handle and the returned object have
+ * an outstanding reference.
+ */
 struct panfrost_gem_object *
 panfrost_gem_create_with_handle(struct drm_file *file_priv,
 				struct drm_device *dev, size_t size,
@@ -261,10 +265,10 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv,
 	 * and handle has the id what user can see.
 	 */
 	ret = drm_gem_handle_create(file_priv, &shmem->base, handle);
-	/* drop reference from allocate - handle holds it now. */
-	drm_gem_object_put(&shmem->base);
-	if (ret)
+	if (ret) {
+		drm_gem_object_put(&shmem->base);
 		return ERR_PTR(ret);
+	}
 
 	return bo;
 }
-- 
2.38.1