On Fri, Nov 4, 2022 at 10:55 AM Carlos Llamas <cmllamas@xxxxxxxxxx> wrote:
>
> 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.
>
> Note this patch is specific to stable branches 5.4 and 5.10. Since in
> newer kernel releases binder no longer caches a pointer to the vma.
> Instead, it has been refactored to use vma_lookup() which avoids the
> issue described here. This switch was introduced in commit a43cfc87caaf
> ("android: binder: stop saving a pointer to the VMA").
>
> Fixes: dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munmap")
> Reported-by: Jann Horn <jannh@xxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # 5.10.x
> Cc: Minchan Kim <minchan@xxxxxxxxxx>
> Cc: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>
> Cc: Liam Howlett <liam.howlett@xxxxxxxxxx>
> Signed-off-by: Carlos Llamas <cmllamas@xxxxxxxxxx>
Acked-by: Todd Kjos <tkjos@xxxxxxxxxx>
> ---
> drivers/android/binder_alloc.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
> index 95ca4f934d28..a77ed66425f2 100644
> --- a/drivers/android/binder_alloc.c
> +++ b/drivers/android/binder_alloc.c
> @@ -212,7 +212,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
> mm = alloc->vma_vm_mm;
>
> if (mm) {
> - mmap_read_lock(mm);
> + mmap_write_lock(mm);
> vma = alloc->vma;
> }
>
> @@ -270,7 +270,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
> trace_binder_alloc_page_end(alloc, index);
> }
> if (mm) {
> - mmap_read_unlock(mm);
> + mmap_write_unlock(mm);
> mmput(mm);
> }
> return 0;
> @@ -303,7 +303,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
> }
> err_no_vma:
> if (mm) {
> - mmap_read_unlock(mm);
> + mmap_write_unlock(mm);
> mmput(mm);
> }
> return vma ? -ENOMEM : -ESRCH;
> --
> 2.38.1.431.g37b22c650d-goog
>
