* KASAN: null-ptr-deref Write in binder_update_page_range @ 2018-08-22 6:07 Dae R. Jeong 2018-08-23 5:29 ` Minchan Kim 0 siblings, 1 reply; 7+ messages in thread From: Dae R. Jeong @ 2018-08-22 6:07 UTC (permalink / raw) To: gregkh, arve, tkjos, maco; +Cc: devel, linux-kernel Reporting the crash: KASAN: null-ptr-deref Write in binder_update_page_range This crash has been found in v4.18-rc3 using RaceFuzzer (a modified version of Syzkaller), which we describe more at the end of this report. Our analysis shows that the race occurs when invoking two syscalls concurrently, mmap$binder() and ioctl$BINDER_WRITE_READ. More specifically, since two code lines `alloc->vma = vma;` and `alloc->vma_vm_mm = vma->vm_mm;` in binder_alloc_mmap_handler() is not an atomic operation during mmap$binder() syscall, there is a time window that `alloc->vma` is assigned but `alloc->vma_vm_mm` isn't assigned. It causes the null pointer dereference in binder_alloc_new_buf_locked() since it checks whether `alloc->vma` is NULL, but it doesn't check that `alloc->vma_vm_mm` is NULL. More details on the thread interleaving and the crash log are follows. Thread interleaving: CPU0 (binder_alloc_mmap_handler) CPU1 (binder_alloc_new_buf_locked) ===== ===== // drivers/android/binder_alloc.c // #L718 (v4.18-rc3) alloc->vma = vma; // drivers/android/binder_alloc.c // #L346 (v4.18-rc3) if (alloc->vma == NULL) { ... // alloc->vma is not NULL at this point return ERR_PTR(-ESRCH); } ... // #L438 binder_update_page_range(alloc, 0, (void *)PAGE_ALIGN((uintptr_t)buffer->data), end_page_addr); // In binder_update_page_range() #L218 // But still alloc->vma_vm_mm is NULL here if (need_mm && mmget_not_zero(alloc->vma_vm_mm)) alloc->vma_vm_mm = vma->vm_mm; Crash Log: ================================================================== BUG: KASAN: null-ptr-deref in __atomic_add_unless include/asm-generic/atomic-instrumented.h:89 [inline] BUG: KASAN: null-ptr-deref in atomic_add_unless include/linux/atomic.h:533 [inline] BUG: KASAN: null-ptr-deref in mmget_not_zero include/linux/sched/mm.h:75 [inline] BUG: KASAN: null-ptr-deref in binder_update_page_range+0xece/0x18e0 drivers/android/binder_alloc.c:218 Write of size 4 at addr 0000000000000058 by task syz-executor0/11184 CPU: 1 PID: 11184 Comm: syz-executor0 Not tainted 4.18.0-rc3 #1 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x16e/0x22c lib/dump_stack.c:113 kasan_report_error mm/kasan/report.c:352 [inline] kasan_report+0x163/0x380 mm/kasan/report.c:412 check_memory_region_inline mm/kasan/kasan.c:260 [inline] check_memory_region+0x140/0x1a0 mm/kasan/kasan.c:267 kasan_check_write+0x14/0x20 mm/kasan/kasan.c:278 __atomic_add_unless include/asm-generic/atomic-instrumented.h:89 [inline] atomic_add_unless include/linux/atomic.h:533 [inline] mmget_not_zero include/linux/sched/mm.h:75 [inline] binder_update_page_range+0xece/0x18e0 drivers/android/binder_alloc.c:218 binder_alloc_new_buf_locked drivers/android/binder_alloc.c:443 [inline] binder_alloc_new_buf+0x467/0xc30 drivers/android/binder_alloc.c:513 binder_transaction+0x125b/0x4fb0 drivers/android/binder.c:2957 binder_thread_write+0xc08/0x2770 drivers/android/binder.c:3528 binder_ioctl_write_read.isra.39+0x24f/0x8e0 drivers/android/binder.c:4456 binder_ioctl+0xa86/0xf34 drivers/android/binder.c:4596 vfs_ioctl fs/ioctl.c:46 [inline] do_vfs_ioctl+0x154/0xd40 fs/ioctl.c:686 ksys_ioctl+0x94/0xb0 fs/ioctl.c:701 __do_sys_ioctl fs/ioctl.c:708 [inline] __se_sys_ioctl fs/ioctl.c:706 [inline] __x64_sys_ioctl+0x43/0x50 fs/ioctl.c:706 do_syscall_64+0x167/0x4b0 arch/x86/entry/common.c:290 entry_SYSCALL_64_after_hwframe+0x49/0xbe RIP: 0033:0x456469 Code: 1d ba fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 eb b9 fb ff c3 66 2e 0f 1f 84 00 00 00 00 RSP: 002b:00007f575f268b28 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 RAX: ffffffffffffffda RBX: 000000000072bfa0 RCX: 0000000000456469 RDX: 00000000200003c0 RSI: 00000000c0306201 RDI: 0000000000000016 RBP: 00000000000001a2 R08: 0000000000000000 R09: 0000000000000000 R10: 0000000000000000 R11: 0000000000000246 R12: 00007f575f2696d4 R13: 00000000ffffffff R14: 00000000006f77d0 R15: 0000000000000000 ================================================================== = About RaceFuzzer RaceFuzzer is a customized version of Syzkaller, specifically tailored to find race condition bugs in the Linux kernel. While we leverage many different technique, the notable feature of RaceFuzzer is in leveraging a custom hypervisor (QEMU/KVM) to interleave the scheduling. In particular, we modified the hypervisor to intentionally stall a per-core execution, which is similar to supporting per-core breakpoint functionality. This allows RaceFuzzer to force the kernel to deterministically trigger racy condition (which may rarely happen in practice due to randomness in scheduling). RaceFuzzer's C repro always pinpoints two racy syscalls. Since C repro's scheduling synchronization should be performed at the user space, its reproducibility is limited (reproduction may take from 1 second to 10 minutes (or even more), depending on a bug). This is because, while RaceFuzzer precisely interleaves the scheduling at the kernel's instruction level when finding this bug, C repro cannot fully utilize such a feature. Please disregard all code related to "should_hypercall" in the C repro, as this is only for our debugging purposes using our own hypervisor. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: KASAN: null-ptr-deref Write in binder_update_page_range 2018-08-22 6:07 KASAN: null-ptr-deref Write in binder_update_page_range Dae R. Jeong @ 2018-08-23 5:29 ` Minchan Kim 2018-08-23 10:03 ` Dae R. Jeong 2018-08-27 13:35 ` Martijn Coenen 0 siblings, 2 replies; 7+ messages in thread From: Minchan Kim @ 2018-08-23 5:29 UTC (permalink / raw) To: Dae R. Jeong; +Cc: gregkh, arve, tkjos, maco, devel, linux-kernel Hi, On Wed, Aug 22, 2018 at 03:07:04PM +0900, Dae R. Jeong wrote: > Reporting the crash: KASAN: null-ptr-deref Write in binder_update_page_range > > This crash has been found in v4.18-rc3 using RaceFuzzer (a modified > version of Syzkaller), which we describe more at the end of this > report. > > Our analysis shows that the race occurs when invoking two syscalls > concurrently, mmap$binder() and ioctl$BINDER_WRITE_READ. More > specifically, since two code lines `alloc->vma = vma;` and > `alloc->vma_vm_mm = vma->vm_mm;` in binder_alloc_mmap_handler() is not > an atomic operation during mmap$binder() syscall, there is a time > window that `alloc->vma` is assigned but `alloc->vma_vm_mm` isn't > assigned. It causes the null pointer dereference in > binder_alloc_new_buf_locked() since it checks whether `alloc->vma` is > NULL, but it doesn't check that `alloc->vma_vm_mm` is NULL. More > details on the thread interleaving and the crash log are follows. > > > Thread interleaving: > CPU0 (binder_alloc_mmap_handler) CPU1 (binder_alloc_new_buf_locked) > ===== ===== > // drivers/android/binder_alloc.c > // #L718 (v4.18-rc3) > alloc->vma = vma; > // drivers/android/binder_alloc.c > // #L346 (v4.18-rc3) > if (alloc->vma == NULL) { > ... > // alloc->vma is not NULL at this point > return ERR_PTR(-ESRCH); > } > ... > // #L438 > binder_update_page_range(alloc, 0, > (void *)PAGE_ALIGN((uintptr_t)buffer->data), > end_page_addr); > > // In binder_update_page_range() #L218 > // But still alloc->vma_vm_mm is NULL here > if (need_mm && mmget_not_zero(alloc->vma_vm_mm)) > alloc->vma_vm_mm = vma->vm_mm; > > > Crash Log: > ================================================================== > BUG: KASAN: null-ptr-deref in __atomic_add_unless include/asm-generic/atomic-instrumented.h:89 [inline] > BUG: KASAN: null-ptr-deref in atomic_add_unless include/linux/atomic.h:533 [inline] > BUG: KASAN: null-ptr-deref in mmget_not_zero include/linux/sched/mm.h:75 [inline] > BUG: KASAN: null-ptr-deref in binder_update_page_range+0xece/0x18e0 drivers/android/binder_alloc.c:218 > Write of size 4 at addr 0000000000000058 by task syz-executor0/11184 > > CPU: 1 PID: 11184 Comm: syz-executor0 Not tainted 4.18.0-rc3 #1 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014 > Call Trace: > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0x16e/0x22c lib/dump_stack.c:113 > kasan_report_error mm/kasan/report.c:352 [inline] > kasan_report+0x163/0x380 mm/kasan/report.c:412 > check_memory_region_inline mm/kasan/kasan.c:260 [inline] > check_memory_region+0x140/0x1a0 mm/kasan/kasan.c:267 > kasan_check_write+0x14/0x20 mm/kasan/kasan.c:278 > __atomic_add_unless include/asm-generic/atomic-instrumented.h:89 [inline] > atomic_add_unless include/linux/atomic.h:533 [inline] > mmget_not_zero include/linux/sched/mm.h:75 [inline] > binder_update_page_range+0xece/0x18e0 drivers/android/binder_alloc.c:218 > binder_alloc_new_buf_locked drivers/android/binder_alloc.c:443 [inline] > binder_alloc_new_buf+0x467/0xc30 drivers/android/binder_alloc.c:513 > binder_transaction+0x125b/0x4fb0 drivers/android/binder.c:2957 > binder_thread_write+0xc08/0x2770 drivers/android/binder.c:3528 > binder_ioctl_write_read.isra.39+0x24f/0x8e0 drivers/android/binder.c:4456 > binder_ioctl+0xa86/0xf34 drivers/android/binder.c:4596 > vfs_ioctl fs/ioctl.c:46 [inline] > do_vfs_ioctl+0x154/0xd40 fs/ioctl.c:686 > ksys_ioctl+0x94/0xb0 fs/ioctl.c:701 > __do_sys_ioctl fs/ioctl.c:708 [inline] > __se_sys_ioctl fs/ioctl.c:706 [inline] > __x64_sys_ioctl+0x43/0x50 fs/ioctl.c:706 > do_syscall_64+0x167/0x4b0 arch/x86/entry/common.c:290 > entry_SYSCALL_64_after_hwframe+0x49/0xbe > RIP: 0033:0x456469 > Code: 1d ba fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 eb b9 fb ff c3 66 2e 0f 1f 84 00 00 00 00 > RSP: 002b:00007f575f268b28 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 > RAX: ffffffffffffffda RBX: 000000000072bfa0 RCX: 0000000000456469 > RDX: 00000000200003c0 RSI: 00000000c0306201 RDI: 0000000000000016 > RBP: 00000000000001a2 R08: 0000000000000000 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000246 R12: 00007f575f2696d4 > R13: 00000000ffffffff R14: 00000000006f77d0 R15: 0000000000000000 > ================================================================== > > = About RaceFuzzer > > RaceFuzzer is a customized version of Syzkaller, specifically tailored > to find race condition bugs in the Linux kernel. While we leverage > many different technique, the notable feature of RaceFuzzer is in > leveraging a custom hypervisor (QEMU/KVM) to interleave the > scheduling. In particular, we modified the hypervisor to intentionally > stall a per-core execution, which is similar to supporting per-core > breakpoint functionality. This allows RaceFuzzer to force the kernel > to deterministically trigger racy condition (which may rarely happen > in practice due to randomness in scheduling). > > RaceFuzzer's C repro always pinpoints two racy syscalls. Since C > repro's scheduling synchronization should be performed at the user > space, its reproducibility is limited (reproduction may take from 1 > second to 10 minutes (or even more), depending on a bug). This is > because, while RaceFuzzer precisely interleaves the scheduling at the > kernel's instruction level when finding this bug, C repro cannot fully > utilize such a feature. Please disregard all code related to > "should_hypercall" in the C repro, as this is only for our debugging > purposes using our own hypervisor. What a amazing tool! Could you test this patch? I found that bug a month ago but didn't submit yet. From 03fb1de4784a0ae34ddd60ab0706ec43b7dfaf8d Mon Sep 17 00:00:00 2001 From: Minchan Kim <minchan@kernel.org> Date: Thu, 23 Aug 2018 14:14:13 +0900 Subject: [PATCH] android: binder: fix the race mmap and alloc_new_buf_locked There is RaceFuzzer report like below because we have no lock to close below the race between binder_mmap and binder_alloc_new_buf_locked. To close the race, let's use memory barrier so that if someone see alloc->vma is not NULL, alloc->vma_vm_mm should be never NULL. (I didn't add stable mark intentionallybecause standard android userspace libraries that interact with binder (libbinder & libhwbinder) prevent the mmap/ioctl race. - from Todd) " Thread interleaving: CPU0 (binder_alloc_mmap_handler) CPU1 (binder_alloc_new_buf_locked) ===== ===== // drivers/android/binder_alloc.c // #L718 (v4.18-rc3) alloc->vma = vma; // drivers/android/binder_alloc.c // #L346 (v4.18-rc3) if (alloc->vma == NULL) { ... // alloc->vma is not NULL at this point return ERR_PTR(-ESRCH); } ... // #L438 binder_update_page_range(alloc, 0, (void *)PAGE_ALIGN((uintptr_t)buffer->data), end_page_addr); // In binder_update_page_range() #L218 // But still alloc->vma_vm_mm is NULL here if (need_mm && mmget_not_zero(alloc->vma_vm_mm)) alloc->vma_vm_mm = vma->vm_mm; Crash Log: ================================================================== BUG: KASAN: null-ptr-deref in __atomic_add_unless include/asm-generic/atomic-instrumented.h:89 [inline] BUG: KASAN: null-ptr-deref in atomic_add_unless include/linux/atomic.h:533 [inline] BUG: KASAN: null-ptr-deref in mmget_not_zero include/linux/sched/mm.h:75 [inline] BUG: KASAN: null-ptr-deref in binder_update_page_range+0xece/0x18e0 drivers/android/binder_alloc.c:218 Write of size 4 at addr 0000000000000058 by task syz-executor0/11184 CPU: 1 PID: 11184 Comm: syz-executor0 Not tainted 4.18.0-rc3 #1 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x16e/0x22c lib/dump_stack.c:113 kasan_report_error mm/kasan/report.c:352 [inline] kasan_report+0x163/0x380 mm/kasan/report.c:412 check_memory_region_inline mm/kasan/kasan.c:260 [inline] check_memory_region+0x140/0x1a0 mm/kasan/kasan.c:267 kasan_check_write+0x14/0x20 mm/kasan/kasan.c:278 __atomic_add_unless include/asm-generic/atomic-instrumented.h:89 [inline] atomic_add_unless include/linux/atomic.h:533 [inline] mmget_not_zero include/linux/sched/mm.h:75 [inline] binder_update_page_range+0xece/0x18e0 drivers/android/binder_alloc.c:218 binder_alloc_new_buf_locked drivers/android/binder_alloc.c:443 [inline] binder_alloc_new_buf+0x467/0xc30 drivers/android/binder_alloc.c:513 binder_transaction+0x125b/0x4fb0 drivers/android/binder.c:2957 binder_thread_write+0xc08/0x2770 drivers/android/binder.c:3528 binder_ioctl_write_read.isra.39+0x24f/0x8e0 drivers/android/binder.c:4456 binder_ioctl+0xa86/0xf34 drivers/android/binder.c:4596 vfs_ioctl fs/ioctl.c:46 [inline] do_vfs_ioctl+0x154/0xd40 fs/ioctl.c:686 ksys_ioctl+0x94/0xb0 fs/ioctl.c:701 __do_sys_ioctl fs/ioctl.c:708 [inline] __se_sys_ioctl fs/ioctl.c:706 [inline] __x64_sys_ioctl+0x43/0x50 fs/ioctl.c:706 do_syscall_64+0x167/0x4b0 arch/x86/entry/common.c:290 entry_SYSCALL_64_after_hwframe+0x49/0xbe " Signed-off-by: Todd Kjos <tkjos@google.com> Signed-off-by: Minchan Kim <minchan@kernel.org> --- drivers/android/binder_alloc.c | 43 +++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c index 3f3b7b253445..64fd96eada31 100644 --- a/drivers/android/binder_alloc.c +++ b/drivers/android/binder_alloc.c @@ -332,6 +332,35 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate, return vma ? -ENOMEM : -ESRCH; } + +static inline void binder_alloc_set_vma(struct binder_alloc *alloc, + struct vm_area_struct *vma) +{ + if (vma) + alloc->vma_vm_mm = vma->vm_mm; + /* + * If we see alloc->vma is not NULL, buffer data structures set up + * completely. Look at smp_rmb side binder_alloc_get_vma. + * We also want to guarantee new alloc->vma_vm_mm is always visible + * if alloc->vma is set. + */ + smp_wmb(); + alloc->vma = vma; +} + +static inline struct vm_area_struct *binder_alloc_get_vma( + struct binder_alloc *alloc) +{ + struct vm_area_struct *vma = NULL; + + if (alloc->vma) { + /* Look at description in binder_alloc_set_vma */ + smp_rmb(); + vma = alloc->vma; + } + return vma; +} + static struct binder_buffer *binder_alloc_new_buf_locked( struct binder_alloc *alloc, size_t data_size, @@ -348,7 +377,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked( size_t size, data_offsets_size; int ret; - if (alloc->vma == NULL) { + if (!binder_alloc_get_vma(alloc)) { binder_alloc_debug(BINDER_DEBUG_USER_ERROR, "%d: binder_alloc_buf, no vma\n", alloc->pid); @@ -723,9 +752,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, buffer->free = 1; binder_insert_free_buffer(alloc, buffer); alloc->free_async_space = alloc->buffer_size / 2; - barrier(); - alloc->vma = vma; - alloc->vma_vm_mm = vma->vm_mm; + binder_alloc_set_vma(alloc, vma); mmgrab(alloc->vma_vm_mm); return 0; @@ -754,10 +781,10 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc) int buffers, page_count; struct binder_buffer *buffer; - BUG_ON(alloc->vma); - buffers = 0; mutex_lock(&alloc->mutex); + BUG_ON(alloc->vma); + while ((n = rb_first(&alloc->allocated_buffers))) { buffer = rb_entry(n, struct binder_buffer, rb_node); @@ -900,7 +927,7 @@ int binder_alloc_get_allocated_count(struct binder_alloc *alloc) */ void binder_alloc_vma_close(struct binder_alloc *alloc) { - WRITE_ONCE(alloc->vma, NULL); + binder_alloc_set_vma(alloc, NULL); } /** @@ -935,7 +962,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item, index = page - alloc->pages; page_addr = (uintptr_t)alloc->buffer + index * PAGE_SIZE; - vma = alloc->vma; + vma = binder_alloc_get_vma(alloc); if (vma) { if (!mmget_not_zero(alloc->vma_vm_mm)) goto err_mmget; -- 2.18.0.1017.ga543ac7ca45-goog ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: KASAN: null-ptr-deref Write in binder_update_page_range 2018-08-23 5:29 ` Minchan Kim @ 2018-08-23 10:03 ` Dae R. Jeong 2018-08-24 0:36 ` Minchan Kim 2018-08-27 13:35 ` Martijn Coenen 1 sibling, 1 reply; 7+ messages in thread From: Dae R. Jeong @ 2018-08-23 10:03 UTC (permalink / raw) To: Minchan Kim; +Cc: gregkh, arve, tkjos, maco, devel, linux-kernel > Could you test this patch? I found that bug a month ago but didn't submit > yet. I don't have a reproducer now. I manually analzed a root cause of the crash using a fuzzer's log. The log reported a race on 'alloc->vma'. Because I don't have a reproducer, I can't test the patch. I'm sorry. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: KASAN: null-ptr-deref Write in binder_update_page_range 2018-08-23 10:03 ` Dae R. Jeong @ 2018-08-24 0:36 ` Minchan Kim 0 siblings, 0 replies; 7+ messages in thread From: Minchan Kim @ 2018-08-24 0:36 UTC (permalink / raw) To: Dae R. Jeong; +Cc: gregkh, arve, tkjos, maco, devel, linux-kernel On Thu, Aug 23, 2018 at 07:03:34PM +0900, Dae R. Jeong wrote: > > Could you test this patch? I found that bug a month ago but didn't submit > > yet. > > I don't have a reproducer now. I manually analzed a root cause of the > crash using a fuzzer's log. The log reported a race on 'alloc->vma'. > Because I don't have a reproducer, I can't test the patch. I'm sorry. Ah, Okay. Anyway, one of author for the patch is already binder maintainer. If other maintainers don't object it, let's fix the race in this chance. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: KASAN: null-ptr-deref Write in binder_update_page_range 2018-08-23 5:29 ` Minchan Kim 2018-08-23 10:03 ` Dae R. Jeong @ 2018-08-27 13:35 ` Martijn Coenen 2018-09-07 3:34 ` Minchan Kim 1 sibling, 1 reply; 7+ messages in thread From: Martijn Coenen @ 2018-08-27 13:35 UTC (permalink / raw) To: Minchan Kim Cc: Dae R. Jeong, Greg KH, Arve Hjønnevåg, Todd Kjos, open list:ANDROID DRIVERS, LKML Thanks Minchan! On Thu, Aug 23, 2018 at 7:29 AM, Minchan Kim <minchan@kernel.org> wrote: > Signed-off-by: Todd Kjos <tkjos@google.com> > Signed-off-by: Minchan Kim <minchan@kernel.org> Reviewed-by: Martijn Coenen <maco@android.com> > --- > drivers/android/binder_alloc.c | 43 +++++++++++++++++++++++++++------- > 1 file changed, 35 insertions(+), 8 deletions(-) > > diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c > index 3f3b7b253445..64fd96eada31 100644 > --- a/drivers/android/binder_alloc.c > +++ b/drivers/android/binder_alloc.c > @@ -332,6 +332,35 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate, > return vma ? -ENOMEM : -ESRCH; > } > > + > +static inline void binder_alloc_set_vma(struct binder_alloc *alloc, > + struct vm_area_struct *vma) > +{ > + if (vma) > + alloc->vma_vm_mm = vma->vm_mm; > + /* > + * If we see alloc->vma is not NULL, buffer data structures set up > + * completely. Look at smp_rmb side binder_alloc_get_vma. > + * We also want to guarantee new alloc->vma_vm_mm is always visible > + * if alloc->vma is set. > + */ > + smp_wmb(); > + alloc->vma = vma; > +} > + > +static inline struct vm_area_struct *binder_alloc_get_vma( > + struct binder_alloc *alloc) > +{ > + struct vm_area_struct *vma = NULL; > + > + if (alloc->vma) { > + /* Look at description in binder_alloc_set_vma */ > + smp_rmb(); > + vma = alloc->vma; > + } > + return vma; > +} > + > static struct binder_buffer *binder_alloc_new_buf_locked( > struct binder_alloc *alloc, > size_t data_size, > @@ -348,7 +377,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked( > size_t size, data_offsets_size; > int ret; > > - if (alloc->vma == NULL) { > + if (!binder_alloc_get_vma(alloc)) { > binder_alloc_debug(BINDER_DEBUG_USER_ERROR, > "%d: binder_alloc_buf, no vma\n", > alloc->pid); > @@ -723,9 +752,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, > buffer->free = 1; > binder_insert_free_buffer(alloc, buffer); > alloc->free_async_space = alloc->buffer_size / 2; > - barrier(); > - alloc->vma = vma; > - alloc->vma_vm_mm = vma->vm_mm; > + binder_alloc_set_vma(alloc, vma); > mmgrab(alloc->vma_vm_mm); > > return 0; > @@ -754,10 +781,10 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc) > int buffers, page_count; > struct binder_buffer *buffer; > > - BUG_ON(alloc->vma); > - > buffers = 0; > mutex_lock(&alloc->mutex); > + BUG_ON(alloc->vma); > + > while ((n = rb_first(&alloc->allocated_buffers))) { > buffer = rb_entry(n, struct binder_buffer, rb_node); > > @@ -900,7 +927,7 @@ int binder_alloc_get_allocated_count(struct binder_alloc *alloc) > */ > void binder_alloc_vma_close(struct binder_alloc *alloc) > { > - WRITE_ONCE(alloc->vma, NULL); > + binder_alloc_set_vma(alloc, NULL); > } > > /** > @@ -935,7 +962,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item, > > index = page - alloc->pages; > page_addr = (uintptr_t)alloc->buffer + index * PAGE_SIZE; > - vma = alloc->vma; > + vma = binder_alloc_get_vma(alloc); > if (vma) { > if (!mmget_not_zero(alloc->vma_vm_mm)) > goto err_mmget; > -- > 2.18.0.1017.ga543ac7ca45-goog > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: KASAN: null-ptr-deref Write in binder_update_page_range 2018-08-27 13:35 ` Martijn Coenen @ 2018-09-07 3:34 ` Minchan Kim 2018-09-12 7:18 ` Greg KH 0 siblings, 1 reply; 7+ messages in thread From: Minchan Kim @ 2018-09-07 3:34 UTC (permalink / raw) To: Martijn Coenen, gregkh Cc: Dae R. Jeong, Arve Hjønnevåg, Todd Kjos, open list:ANDROID DRIVERS, LKML Thanks, Martijn, Greg, could you have a look to pick up? On Mon, Aug 27, 2018 at 03:35:24PM +0200, Martijn Coenen wrote: > Thanks Minchan! > > On Thu, Aug 23, 2018 at 7:29 AM, Minchan Kim <minchan@kernel.org> wrote: > > Signed-off-by: Todd Kjos <tkjos@google.com> > > Signed-off-by: Minchan Kim <minchan@kernel.org> > Reviewed-by: Martijn Coenen <maco@android.com> > > > --- > > drivers/android/binder_alloc.c | 43 +++++++++++++++++++++++++++------- > > 1 file changed, 35 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c > > index 3f3b7b253445..64fd96eada31 100644 > > --- a/drivers/android/binder_alloc.c > > +++ b/drivers/android/binder_alloc.c > > @@ -332,6 +332,35 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate, > > return vma ? -ENOMEM : -ESRCH; > > } > > > > + > > +static inline void binder_alloc_set_vma(struct binder_alloc *alloc, > > + struct vm_area_struct *vma) > > +{ > > + if (vma) > > + alloc->vma_vm_mm = vma->vm_mm; > > + /* > > + * If we see alloc->vma is not NULL, buffer data structures set up > > + * completely. Look at smp_rmb side binder_alloc_get_vma. > > + * We also want to guarantee new alloc->vma_vm_mm is always visible > > + * if alloc->vma is set. > > + */ > > + smp_wmb(); > > + alloc->vma = vma; > > +} > > + > > +static inline struct vm_area_struct *binder_alloc_get_vma( > > + struct binder_alloc *alloc) > > +{ > > + struct vm_area_struct *vma = NULL; > > + > > + if (alloc->vma) { > > + /* Look at description in binder_alloc_set_vma */ > > + smp_rmb(); > > + vma = alloc->vma; > > + } > > + return vma; > > +} > > + > > static struct binder_buffer *binder_alloc_new_buf_locked( > > struct binder_alloc *alloc, > > size_t data_size, > > @@ -348,7 +377,7 @@ static struct binder_buffer *binder_alloc_new_buf_locked( > > size_t size, data_offsets_size; > > int ret; > > > > - if (alloc->vma == NULL) { > > + if (!binder_alloc_get_vma(alloc)) { > > binder_alloc_debug(BINDER_DEBUG_USER_ERROR, > > "%d: binder_alloc_buf, no vma\n", > > alloc->pid); > > @@ -723,9 +752,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc, > > buffer->free = 1; > > binder_insert_free_buffer(alloc, buffer); > > alloc->free_async_space = alloc->buffer_size / 2; > > - barrier(); > > - alloc->vma = vma; > > - alloc->vma_vm_mm = vma->vm_mm; > > + binder_alloc_set_vma(alloc, vma); > > mmgrab(alloc->vma_vm_mm); > > > > return 0; > > @@ -754,10 +781,10 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc) > > int buffers, page_count; > > struct binder_buffer *buffer; > > > > - BUG_ON(alloc->vma); > > - > > buffers = 0; > > mutex_lock(&alloc->mutex); > > + BUG_ON(alloc->vma); > > + > > while ((n = rb_first(&alloc->allocated_buffers))) { > > buffer = rb_entry(n, struct binder_buffer, rb_node); > > > > @@ -900,7 +927,7 @@ int binder_alloc_get_allocated_count(struct binder_alloc *alloc) > > */ > > void binder_alloc_vma_close(struct binder_alloc *alloc) > > { > > - WRITE_ONCE(alloc->vma, NULL); > > + binder_alloc_set_vma(alloc, NULL); > > } > > > > /** > > @@ -935,7 +962,7 @@ enum lru_status binder_alloc_free_page(struct list_head *item, > > > > index = page - alloc->pages; > > page_addr = (uintptr_t)alloc->buffer + index * PAGE_SIZE; > > - vma = alloc->vma; > > + vma = binder_alloc_get_vma(alloc); > > if (vma) { > > if (!mmget_not_zero(alloc->vma_vm_mm)) > > goto err_mmget; > > -- > > 2.18.0.1017.ga543ac7ca45-goog > > > > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: KASAN: null-ptr-deref Write in binder_update_page_range 2018-09-07 3:34 ` Minchan Kim @ 2018-09-12 7:18 ` Greg KH 0 siblings, 0 replies; 7+ messages in thread From: Greg KH @ 2018-09-12 7:18 UTC (permalink / raw) To: Minchan Kim Cc: Martijn Coenen, open list:ANDROID DRIVERS, Arve Hjønnevåg, Todd Kjos, LKML, Dae R. Jeong On Fri, Sep 07, 2018 at 12:34:19PM +0900, Minchan Kim wrote: > Thanks, Martijn, > > Greg, could you have a look to pick up? Now queued up, thanks. greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-09-12 7:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-22 6:07 KASAN: null-ptr-deref Write in binder_update_page_range Dae R. Jeong 2018-08-23 5:29 ` Minchan Kim 2018-08-23 10:03 ` Dae R. Jeong 2018-08-24 0:36 ` Minchan Kim 2018-08-27 13:35 ` Martijn Coenen 2018-09-07 3:34 ` Minchan Kim 2018-09-12 7:18 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).