[PATCH] drm: Avoid the chaotic interleaving of change and delete handle

[Edward Adam Davis]

First, let's take a look at how GEM object is freed:
CPU0					CPU1
====					====
drm_gem_change_handle_ioctl()
 drm_gem_object_lookup() // got GEM obj and refcnt is 2
 ... blocks on prime.lock
					drm_gem_handle_delete()
	                                 drm_gem_object_release_handle()
					  ... acquires prime.lock
					  drm_prime_remove_buf_handle()
					  ... unlock prime.lock
					  drm_gem_object_handle_put_unlocked()
					   drm_gem_object_put() // obj refcnt is 1
 ... acquires prime.lock
 drm_gem_object_put() // obj refcnt is 0
  drm_gem_shmem_free() // obj is freed

After a GEM object has been freed, a Use-After-Free vulnerability [1]
is triggered when closes the DRM file, as the drm_gem_release() function
attempts to access the already-freed GEM object.

Adjust the change handle ioctl and handle delete ioctl to be atomic
operations; this prevents simultaneous change and delete operations
on the same GEM object from interfering with the release of the GEM
object during the closing of the DRM file.

[1]
BUG: KASAN: slab-use-after-free in drm_gem_object_release_handle+0x4b/0x1e0 drivers/gpu/drm/drm_gem.c:374
Call Trace:
 drm_gem_object_release_handle+0x4b/0x1e0 drivers/gpu/drm/drm_gem.c:374
 idr_for_each+0x1c6/0x2a0 lib/idr.c:210
 drm_gem_release+0x28/0x40 drivers/gpu/drm/drm_gem.c:1088
 drm_file_free+0x729/0xa00 drivers/gpu/drm/drm_file.c:261
 drm_close_helper drivers/gpu/drm/drm_file.c:290 [inline]
 drm_release+0x2de/0x3f0 drivers/gpu/drm/drm_file.c:438

Allocated by task 6090:
 __drm_gem_shmem_create+0xc4/0x2e0 drivers/gpu/drm/drm_gem_shmem_helper.c:130
 drm_gem_shmem_create drivers/gpu/drm/drm_gem_shmem_helper.c:157 [inline]
 drm_gem_shmem_create_with_handle drivers/gpu/drm/drm_gem_shmem_helper.c:460 [inline]
 drm_gem_shmem_dumb_create+0x72/0x120 drivers/gpu/drm/drm_gem_shmem_helper.c:549
 
Freed by task 6093:
 drm_gem_object_release_handle+0xc2/0x1e0 drivers/gpu/drm/drm_gem.c:385
 drm_gem_handle_delete+0x7b/0xb0 drivers/gpu/drm/drm_gem.c:413
 drm_ioctl_kernel+0x2df/0x3b0 drivers/gpu/drm/drm_ioctl.c:804

Fixes: 53096728b891 ("drm: Add DRM prime interface to reassign GEM handle")
Reported-by: syzbot+b2e951687503f32f74ce@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=b2e951687503f32f74ce
Tested-by: syzbot+b2e951687503f32f74ce@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Edward Adam Davis <eadavis@xxxxxx>
---
 drivers/gpu/drm/drm_gem.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 891c3bff5ae0..63a8d7e980b5 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -374,14 +374,8 @@ drm_gem_object_release_handle(int id, void *ptr, void *data)
 	if (obj->funcs->close)
 		obj->funcs->close(obj, file_priv);
 
-	mutex_lock(&file_priv->prime.lock);
-
 	drm_prime_remove_buf_handle(&file_priv->prime, id);
-
-	mutex_unlock(&file_priv->prime.lock);
-
 	drm_vma_node_revoke(&obj->vma_node, file_priv);
-
 	drm_gem_object_handle_put_unlocked(obj);
 
 	return 0;
@@ -401,13 +395,16 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
 {
 	struct drm_gem_object *obj;
 
+	mutex_lock(&filp->prime.lock);
 	spin_lock(&filp->table_lock);
 
 	/* Check if we currently have a reference on the object */
 	obj = idr_replace(&filp->object_idr, NULL, handle);
 	spin_unlock(&filp->table_lock);
-	if (IS_ERR_OR_NULL(obj))
+	if (IS_ERR_OR_NULL(obj)) {
+		mutex_unlock(&filp->prime.lock);
 		return -EINVAL;
+	}
 
 	/* Release driver's reference and decrement refcount. */
 	drm_gem_object_release_handle(handle, obj, filp);
@@ -416,6 +413,7 @@ drm_gem_handle_delete(struct drm_file *filp, u32 handle)
 	spin_lock(&filp->table_lock);
 	idr_remove(&filp->object_idr, handle);
 	spin_unlock(&filp->table_lock);
+	mutex_unlock(&filp->prime.lock);
 
 	return 0;
 }
@@ -1012,17 +1010,18 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
 		return -EINVAL;
 	handle = args->new_handle;
 
+	mutex_lock(&file_priv->prime.lock);
 	obj = drm_gem_object_lookup(file_priv, args->handle);
-	if (!obj)
+	if (!obj) {
+		mutex_unlock(&file_priv->prime.lock);
 		return -ENOENT;
+	}
 
 	if (args->handle == handle) {
 		ret = 0;
-		goto out;
+		goto out_unlock;
 	}
 
-	mutex_lock(&file_priv->prime.lock);
-
 	spin_lock(&file_priv->table_lock);
 	ret = idr_alloc(&file_priv->object_idr, obj, handle, handle + 1,
 			GFP_NOWAIT);
@@ -1051,9 +1050,8 @@ int drm_gem_change_handle_ioctl(struct drm_device *dev, void *data,
 	spin_unlock(&file_priv->table_lock);
 
 out_unlock:
-	mutex_unlock(&file_priv->prime.lock);
-out:
 	drm_gem_object_put(obj);
+	mutex_unlock(&file_priv->prime.lock);
 
 	return ret;
 }
@@ -1085,8 +1083,10 @@ drm_gem_open(struct drm_device *dev, struct drm_file *file_private)
 void
 drm_gem_release(struct drm_device *dev, struct drm_file *file_private)
 {
+	mutex_lock(&file_private->prime.lock);
 	idr_for_each(&file_private->object_idr,
 		     &drm_gem_object_release_handle, file_private);
+	mutex_unlock(&file_private->prime.lock);
 	idr_destroy(&file_private->object_idr);
 }
 
-- 
2.43.0