Forwarded: [PATCH] drm/gem: fix use-after-free in drm_gem_release

[syzbot <syzbot+b2e951687503f32f74ce@syzkaller.appspotmail.com>]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com.

***

Subject: [PATCH] drm/gem: fix use-after-free in drm_gem_release
Author: kartikey406@gmail.com

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master


drm_gem_release() calls idr_for_each() with
drm_gem_object_release_handle() as callback. A concurrent
drm_gem_handle_delete() can free the GEM object between the time
idr_for_each() reads the pointer and drm_gem_object_release_handle()
uses it, causing a use-after-free.

Fix this by splitting the operation into two phases:

1. Under table_lock, walk the IDR using a new collect callback that
   only increments each object's reference count and adds it to a
   local list. Only atomic operations are performed here so holding
   a spinlock is safe.

2. Outside the lock, iterate the local list and call
   drm_gem_object_release_handle() on each object safely. Drop our
   reference afterwards and destroy the IDR only after all objects
   have been properly released.

Holding a reference during phase 1 ensures that a concurrent
drm_gem_handle_delete() cannot free any object while we are still
using it.

Reported-by: syzbot+b2e951687503f32f74ce@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=b2e951687503f32f74ce
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 drivers/gpu/drm/drm_gem.c | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 891c3bff5ae0..ee9939356de4 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1073,6 +1073,30 @@ drm_gem_open(struct drm_device *dev, struct drm_file *file_private)
 	spin_lock_init(&file_private->table_lock);
 }
 
+struct drm_gem_object_entry {
+	struct drm_gem_object *obj;
+	struct list_head head;
+};
+
+static int drm_gem_object_collect(int id, void *ptr, void *data)
+{
+	struct drm_gem_object_entry *entry;
+	struct drm_gem_object *obj = ptr;
+	struct list_head *list = data;
+
+	if (!obj)
+		return 0;
+
+	entry = kmalloc_obj(*entry, GFP_ATOMIC);
+	if (!entry)
+		return -ENOMEM;
+
+	drm_gem_object_get(obj);
+	entry->obj = obj;
+	list_add(&entry->head, list);
+	return 0;
+}
+
 /**
  * drm_gem_release - release file-private GEM resources
  * @dev: drm_device which is being closed by userspace
@@ -1085,8 +1109,21 @@ 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)
 {
+	struct drm_gem_object_entry *entry, *tmp;
+	LIST_HEAD(list);
+
+	spin_lock(&file_private->table_lock);
 	idr_for_each(&file_private->object_idr,
-		     &drm_gem_object_release_handle, file_private);
+		     &drm_gem_object_collect, &list);
+	spin_unlock(&file_private->table_lock);
+
+	list_for_each_entry_safe(entry, tmp, &list, head) {
+		list_del(&entry->head);
+		drm_gem_object_release_handle(0, entry->obj, file_private);
+		drm_gem_object_put(entry->obj);
+		kfree(entry);
+	}
+
 	idr_destroy(&file_private->object_idr);
 }
 
-- 
2.43.0