On Sat, May 20, 2023 at 10:55:02AM +0100, Greg Kroah-Hartman wrote:
> On Fri, May 19, 2023 at 07:59:49PM +0000, Carlos Llamas wrote:
> > [ cmllamas: clean forward port from commit 015ac18be7de ("binder: fix
> > UAF of alloc->vma in race with munmap()") in 5.10 stable. It is needed
> > in mainline after the revert of commit a43cfc87caaf ("android: binder:
> > stop saving a pointer to the VMA") as pointed out by Liam. The commit
> > log and tags have been tweaked to reflect this. ]
> >
> > In commit 720c24192404 ("ANDROID: binder: change down_write to
> > down_read") binder assumed the mmap read lock is sufficient to protect
> > alloc->vma inside binder_update_page_range(). This used to be accurate
> > until commit dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in
> > munmap"), which now downgrades the mmap_lock after detaching the vma
> > from the rbtree in munmap(). Then it proceeds to teardown and free the
> > vma with only the read lock held.
> >
> > This means that accesses to alloc->vma in binder_update_page_range() now
> > will race with vm_area_free() in munmap() and can cause a UAF as shown
> > in the following KASAN trace:
> >
> > ==================================================================
> > BUG: KASAN: use-after-free in vm_insert_page+0x7c/0x1f0
> > Read of size 8 at addr ffff16204ad00600 by task server/558
> >
> > CPU: 3 PID: 558 Comm: server Not tainted 5.10.150-00001-gdc8dcf942daa #1
> > Hardware name: linux,dummy-virt (DT)
> > Call trace:
> > dump_backtrace+0x0/0x2a0
> > show_stack+0x18/0x2c
> > dump_stack+0xf8/0x164
> > print_address_description.constprop.0+0x9c/0x538
> > kasan_report+0x120/0x200
> > __asan_load8+0xa0/0xc4
> > vm_insert_page+0x7c/0x1f0
> > binder_update_page_range+0x278/0x50c
> > binder_alloc_new_buf+0x3f0/0xba0
> > binder_transaction+0x64c/0x3040
> > binder_thread_write+0x924/0x2020
> > binder_ioctl+0x1610/0x2e5c
> > __arm64_sys_ioctl+0xd4/0x120
> > el0_svc_common.constprop.0+0xac/0x270
> > do_el0_svc+0x38/0xa0
> > el0_svc+0x1c/0x2c
> > el0_sync_handler+0xe8/0x114
> > el0_sync+0x180/0x1c0
> >
> > Allocated by task 559:
> > kasan_save_stack+0x38/0x6c
> > __kasan_kmalloc.constprop.0+0xe4/0xf0
> > kasan_slab_alloc+0x18/0x2c
> > kmem_cache_alloc+0x1b0/0x2d0
> > vm_area_alloc+0x28/0x94
> > mmap_region+0x378/0x920
> > do_mmap+0x3f0/0x600
> > vm_mmap_pgoff+0x150/0x17c
> > ksys_mmap_pgoff+0x284/0x2dc
> > __arm64_sys_mmap+0x84/0xa4
> > el0_svc_common.constprop.0+0xac/0x270
> > do_el0_svc+0x38/0xa0
> > el0_svc+0x1c/0x2c
> > el0_sync_handler+0xe8/0x114
> > el0_sync+0x180/0x1c0
> >
> > Freed by task 560:
> > kasan_save_stack+0x38/0x6c
> > kasan_set_track+0x28/0x40
> > kasan_set_free_info+0x24/0x4c
> > __kasan_slab_free+0x100/0x164
> > kasan_slab_free+0x14/0x20
> > kmem_cache_free+0xc4/0x34c
> > vm_area_free+0x1c/0x2c
> > remove_vma+0x7c/0x94
> > __do_munmap+0x358/0x710
> > __vm_munmap+0xbc/0x130
> > __arm64_sys_munmap+0x4c/0x64
> > el0_svc_common.constprop.0+0xac/0x270
> > do_el0_svc+0x38/0xa0
> > el0_svc+0x1c/0x2c
> > el0_sync_handler+0xe8/0x114
> > el0_sync+0x180/0x1c0
> >
> > [...]
> > ==================================================================
> >
> > To prevent the race above, revert back to taking the mmap write lock
> > inside binder_update_page_range(). One might expect an increase of mmap
> > lock contention. However, binder already serializes these calls via top
> > level alloc->mutex. Also, there was no performance impact shown when
> > running the binder benchmark tests.
> >
> > Fixes: c0fd2101781e ("Revert "android: binder: stop saving a pointer to the VMA"")
>
> I can't find this commit in any tree, are you sure it's correct?
The commit comes from your char-misc-linus branch, it hasn't really
landed in mainline yet. I added this tag to make sure this fix is
bounded to the revert otherwise it exposes the UAF. I know I'm relying
on a merge, so let me know if I should drop the tag instead.
--
Carlos Llamas
