| Message ID | 20220730231559.85423-1-mazinalhaddad05@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/gem: Fix a UAF caused by invalid reference counting in drm_gem_mmap() | expand |
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; }
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@syzkaller.appspotmail.com Signed-off-by: Mazin Al Haddad <mazinalhaddad05@gmail.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(-)