Re: [syzbot] kernel BUG in binder_alloc_deferred_release
From: Liam Howlett
Date: Mon Jun 20 2022 - 22:08:57 EST
* syzbot <syzbot+58b51ac2b04e388ab7b0@xxxxxxxxxxxxxxxxxxxxxxxxx> [220619 21:47]:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: 6012273897fe Add linux-next specific files for 20220615
> git tree: linux-next
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=16596feff00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=b4154677977b1776
> dashboard link: https://syzkaller.appspot.com/bug?extid=58b51ac2b04e388ab7b0
> compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1024e510080000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=113e407ff00000
>
> The issue was bisected to:
>
> commit 42086abba43463fbf495cb80173600284b4c4e8c
> Author: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
> Date: Tue Jun 14 19:00:27 2022 +0000
>
> mm: start tracking VMAs with maple tree
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=10ef827ff00000
> final oops: https://syzkaller.appspot.com/x/report.txt?x=12ef827ff00000
> console output: https://syzkaller.appspot.com/x/log.txt?x=14ef827ff00000
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+58b51ac2b04e388ab7b0@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: 42086abba434 ("mm: start tracking VMAs with maple tree")
>
> ------------[ cut here ]------------
> kernel BUG at drivers/android/binder_alloc.c:820!
> invalid opcode: 0000 [#1] PREEMPT SMP KASAN
> CPU: 0 PID: 2934 Comm: kworker/0:3 Not tainted 5.19.0-rc2-next-20220615-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Workqueue: events binder_deferred_func
> RIP: 0010:binder_alloc_deferred_release+0x602/0x780 drivers/android/binder_alloc.c:820
> Code: c6 80 16 c7 8a 48 c7 c7 80 b6 48 8d e8 57 86 05 fd 31 ff 89 c5 89 c6 e8 fc ba 5b fa 85 ed 74 c6 e9 78 57 55 02 e8 9e be 5b fa <0f> 0b c7 44 24 20 00 00 00 00 e9 27 ff ff ff e8 8a be 5b fa 48 89
> RSP: 0018:ffffc9000de07bc8 EFLAGS: 00010293
> RAX: 0000000000000000 RBX: ffff8880519971e0 RCX: 0000000000000000
> RDX: ffff888026b0d7c0 RSI: ffffffff871f04b2 RDI: ffff888051997270
> RBP: ffff888051997000 R08: 0000000000000000 R09: ffffffff8dbcac17
> R10: fffffbfff1b79582 R11: 0000000000000000 R12: ffff888147653c60
> R13: ffff8880519972d8 R14: ffff888147653d10 R15: dffffc0000000000
> FS: 0000000000000000(0000) GS:ffff8880b9a00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f99ddec19c1 CR3: 0000000077a01000 CR4: 00000000003506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <TASK>
> binder_free_proc drivers/android/binder.c:4766 [inline]
...
This task has alloc->vma set to a freed VMA. What is happening is that
I've added an allocation later in the mmap_region() code which is being
failed by the fuzzer. This failure means that the vma recorded in the
binder code via the call_mmap() is going to be freed in the
mmap_region() failure path, but the binder code does not know of the
failure.
I was going to move my preallocation above the call_mmap() to avoid this
failure, but that will basically mask the issue(s) in binder. There are
two other possibilities that could cause the same binder failure. The
first being a rather unlikely arch_validate_flags() returning false.
The second is a successful merge of the allocated VMA
- also rare, but still possible.
The code records a VMA to be used later and drops the mmap_lock.
Shouldn't it record the *address* and look up the VMA when it is needed
to avoid using a freed pointer? There has been at least one other issue
with recording the VMA [1].
Anyways, the attached patch fixes the issue I've exposed by saving the
start address and looking it up. I've added lockdep checks for good
measures. It fixes the reproduces from this email, but may need more
oversight.
1. https://lore.kernel.org/lkml/20190301230606.8302-1-tkjos@xxxxxxxxxx/
Thanks,
Liam
From 13698dfe7179b1fa60136d09a02819e784d08969 Mon Sep 17 00:00:00 2001
From: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
Date: Mon, 20 Jun 2022 21:09:09 -0400
Subject: [PATCH] android: binder: Stop saving a pointer to the VMA
Do not record a pointer to a VMA outside of the mmap_lock for later use.
This is unsafe and there are a number of failure paths *after* the
recorded VMA pointer may be freed during setup. There is no callback to
the driver to clear the saved pointer from generic mm code.
Furthermore, the VMA pointer may become stale if any number of VMA
operations end up freeing the VMA so saving it was fragile to being
with.
Instead, change the binder_alloc struct to record the start address of
the VMA and use vma_lookup() to get the vma when needed. Add lockdep
mmap_lock checks on updates to the vma pointer to ensure the lock is
held and depend on that lock for synchronization of readers and writers
- which was already the case anyways, so the smp_wmb()/smp_rmb() was not
necessary.
Fixes: da1b9564e85b (android: binder: fix the race mmap and alloc_new_buf_locked)
Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
Cc: Minchan Kim <minchan@xxxxxxxxxx>
---
drivers/android/binder_alloc.c | 30 ++++++++++++++----------------
drivers/android/binder_alloc.h | 2 +-
2 files changed, 15 insertions(+), 17 deletions(-)
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index 51b502217d00..f555eebceef6 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -213,7 +213,7 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
if (mm) {
mmap_read_lock(mm);
- vma = alloc->vma;
+ vma = vma_lookup(mm, alloc->vma_addr);
}
if (!vma && need_mm) {
@@ -313,16 +313,15 @@ static int binder_update_page_range(struct binder_alloc *alloc, int allocate,
static inline void binder_alloc_set_vma(struct binder_alloc *alloc,
struct vm_area_struct *vma)
{
- if (vma)
+ unsigned long vm_start = 0;
+
+ if (vma) {
+ vm_start = vma->vm_start;
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;
+ }
+
+ mmap_assert_write_locked(alloc->vma_vm_mm);
+ alloc->vma_addr = vm_start;
}
static inline struct vm_area_struct *binder_alloc_get_vma(
@@ -330,11 +329,9 @@ static inline struct vm_area_struct *binder_alloc_get_vma(
{
struct vm_area_struct *vma = NULL;
- if (alloc->vma) {
- /* Look at description in binder_alloc_set_vma */
- smp_rmb();
- vma = alloc->vma;
- }
+ if (alloc->vma_addr)
+ vma = vma_lookup(alloc->vma_vm_mm, alloc->vma_addr);
+
return vma;
}
@@ -817,7 +814,8 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
buffers = 0;
mutex_lock(&alloc->mutex);
- BUG_ON(alloc->vma);
+ BUG_ON(alloc->vma_addr &&
+ vma_lookup(alloc->vma_vm_mm, alloc->vma_addr));
while ((n = rb_first(&alloc->allocated_buffers))) {
buffer = rb_entry(n, struct binder_buffer, rb_node);
diff --git a/drivers/android/binder_alloc.h b/drivers/android/binder_alloc.h
index 7dea57a84c79..1e4fd37af5e0 100644
--- a/drivers/android/binder_alloc.h
+++ b/drivers/android/binder_alloc.h
@@ -100,7 +100,7 @@ struct binder_lru_page {
*/
struct binder_alloc {
struct mutex mutex;
- struct vm_area_struct *vma;
+ unsigned long vma_addr;
struct mm_struct *vma_vm_mm;
void __user *buffer;
struct list_head buffers;
--
2.35.1