linux-kernel - [RFC PATCH] drm/gem: Fix a UAF caused by invalid reference counting in drm_gem

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>] [day] [month] [year] [list]

Message-Id: <20220730231559.85423-1-mazinalhaddad05@gmail.com>
Date:   Sun, 31 Jul 2022 02:15:59 +0300
From:   Mazin Al Haddad <mazinalhaddad05@...il.com>
To:     mripard@...nel.org
Cc:     tzimmermann@...e.de, maarten.lankhorst@...ux.intel.com,
        airlied@...ux.ie, daniel@...ll.ch, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org,
        linux-kernel-mentees@...ts.linuxfoundation.org,
        skhan@...uxfoundation.org, paskripkin@...il.com,
        Mazin Al Haddad <mazinalhaddad05@...il.com>,
        syzbot+c8ae65286134dd1b800d@...kaller.appspotmail.com
Subject: [RFC PATCH] drm/gem: Fix a UAF caused by invalid reference counting in drm_gem_mmap()

Fixes a bug reported by syzkaller.

Mmaping a dumb buffer can result in a use-after-free if there is an
error in the return path of the driver specific gem object's mmap()
callback. This is due to improper reference counting in the error path.

The use-after-free occurs when attempting to close the drm file as
the freed gem object is accessed again (through handle). A failure in
drm_gem_shmem_get_pages() within drm_gem_shmem_mmap() (or equivalent
obj->funcs->mmap() call) leads to a call to drm_gem_vm_close()
in the error path, which decrements the reference count of the object
in question and leads to a free after it is further decremented in
drm_gem_mmap_object()'s error path and drm_gem_mmap().

Fix it by removing the call to drm_gem_vm_close() in error path of
driver specific callback. This changes the code so that only two decrements
happen instead of three, matching up with the references taken by the
driver within the drm_gem_mmap() function.

Trimmed syzbot report:

Call Trace:
 <TASK>
 drm_gem_object_release_handle+0xf2/0x110 drivers/gpu/drm/drm_gem.c:253
 idr_for_each+0x113/0x220 lib/idr.c:208
 drm_gem_release+0x22/0x30 drivers/gpu/drm/drm_gem.c:932
 drm_file_free.part.0+0x805/0xb80 drivers/gpu/drm/drm_file.c:281
 drm_file_free drivers/gpu/drm/drm_file.c:248 [inline]
 drm_close_helper.isra.0+0x17d/0x1f0 drivers/gpu/drm/drm_file.c:308
 drm_release+0x1e6/0x530 drivers/gpu/drm/drm_file.c:495

Freed by task 3606:
 kfree+0xd6/0x4d0 mm/slub.c:4584
 drm_gem_object_free drivers/gpu/drm/drm_gem.c:974 [inline]
 kref_put include/linux/kref.h:65 [inline]
 __drm_gem_object_put include/drm/drm_gem.h:371 [inline]
 drm_gem_object_put include/drm/drm_gem.h:384 [inline]
 drm_gem_mmap+0x4fc/0x770 drivers/gpu/drm/drm_gem.c:1134
 call_mmap include/linux/fs.h:2063 [inline]
 mmap_region+0xbe7/0x1460 mm/mmap.c:1796
 do_mmap+0x863/0xfa0 mm/mmap.c:1587
 vm_mmap_pgoff+0x1b7/0x290 mm/util.c:552
 ksys_mmap_pgoff+0x40d/0x5a0 mm/mmap.c:1633

Link: https://syzkaller.appspot.com/bug?id=e84227937d2d71da66e62f8c67b69a0cc387123c
Fixes: 45d9c8dde4cd ("drm/vgem: use shmem helpers")
Reported-by: syzbot+c8ae65286134dd1b800d@...kaller.appspotmail.com
Signed-off-by: Mazin Al Haddad <mazinalhaddad05@...il.com>
---

RFC because I'm not quite sure that this is the correct fix so guidance would be
appreciated. I looked at the implementation of the drm_gem_object's
mmap callback in various drivers including, exynos, panfrost and the
cma helper and all of them also call drm_gem_vm_close() on error
(which only decrements the object reference count).

Most likely other drivers also run into this issue on mmap() error,
as the dumb buffer create can have one reference on entry
to drm_gem_mmap(). Two references will then be taken (one in drm_gem_mmap
and the other in drm_gem_mmap_object). If the driver decides to error out
and release a reference within it's own error path it will ultimately
remove three references. So maybe it's easier to have the reference counts
be adjusted in the drm_gem_mmap() and drm_gem_mmap_object() function rather
than have it in the drivers.

Is there a need to go through the other drivers and remove the
drm_gem_vm_close() call? Or is there a better way of doing this.
I'm not sure if deleting the call is the correct thing to do here and if
there is something I'm missing. Please let me know if there is anything
I need to change.

 drivers/gpu/drm/drm_gem_shmem_helper.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 8ad0e02991ca..01908bba5970 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -622,7 +622,6 @@ int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct

 	ret = drm_gem_shmem_get_pages(shmem);
 	if (ret) {
-		drm_gem_vm_close(vma);
 		return ret;
 	}

-- 
2.37.1