* 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).