[ 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"")
Fixes: dd2283f2605e ("mm: mmap: zap pages with read mmap_sem in munmap")
Reported-by: Jann Horn <[email protected]>
Closes: https://lore.kernel.org/all/20230518144052.xkj6vmddccq4v66b@revolver
Cc: <[email protected]>
Cc: Minchan Kim <[email protected]>
Cc: Yang Shi <[email protected]>
Cc: Liam Howlett <[email protected]>
Signed-off-by: Carlos Llamas <[email protected]>
Acked-by: Todd Kjos <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
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 e7c9d466f8e8..662a2a2e2e84 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->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.40.1.698.g37aff9b760-goog
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. ]
I'm confused, is this for a stable release (and if so, which one), or is
this for Linus's tree?
thanks,
greg k-h
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?
thanks,
greg k-h
Is it possible that someone can help me resolve this issue I have with me phone
Sent from my iPhone
On Sat, May 20, 2023 at 10:54:15AM +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. ]
>
> I'm confused, is this for a stable release (and if so, which one), or is
> this for Linus's tree?
Sorry for the confusion. This is for Linus's tree.
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
On Sat, May 20, 2023 at 12:51:36PM +0000, Carlos Llamas wrote:
> 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.
Ah, no, that works, thanks, I'll queue it up.
greg k-h