Enforce alloc->mutex in binder_alloc_mmap_handler when add
the entry to list.
Assign the freed pages/page_ptr to NULL to catch possible
use after free with NULL pointer access.
Signed-off-by: Kassey Li <[email protected]>
---
drivers/android/binder_alloc.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/android/binder_alloc.c b/drivers/android/binder_alloc.c
index e3db8297095a..c7d126e04343 100644
--- a/drivers/android/binder_alloc.c
+++ b/drivers/android/binder_alloc.c
@@ -775,6 +775,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
}
buffer->user_data = alloc->buffer;
+ mutex_lock(&alloc->mutex);
list_add(&buffer->entry, &alloc->buffers);
buffer->free = 1;
binder_insert_free_buffer(alloc, buffer);
@@ -782,7 +783,7 @@ int binder_alloc_mmap_handler(struct binder_alloc *alloc,
/* Signal binder_alloc is fully initialized */
binder_alloc_set_vma(alloc, vma);
-
+ mutex_unlock(&alloc->mutex);
return 0;
err_alloc_buf_struct_failed:
@@ -856,9 +857,11 @@ void binder_alloc_deferred_release(struct binder_alloc *alloc)
__func__, alloc->pid, i, page_addr,
on_lru ? "on lru" : "active");
__free_page(alloc->pages[i].page_ptr);
+ alloc->pages[i].page_ptr = NULL;
page_count++;
}
kfree(alloc->pages);
+ alloc->pages = NULL;
}
mutex_unlock(&alloc->mutex);
if (alloc->mm)
--
2.25.1
On Sat, Oct 07, 2023 at 11:40:46AM +0800, Kassey Li wrote:
> Enforce alloc->mutex in binder_alloc_mmap_handler when add
> the entry to list.
>
> Assign the freed pages/page_ptr to NULL to catch possible
> use after free with NULL pointer access.
>
> Signed-off-by: Kassey Li <[email protected]>
> ---
> drivers/android/binder_alloc.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
What commit id does this fix?
thanks,
greg k-h
On 2023/10/7 14:44, Greg KH wrote:
> On Sat, Oct 07, 2023 at 11:40:46AM +0800, Kassey Li wrote:
>> Enforce alloc->mutex in binder_alloc_mmap_handler when add
>> the entry to list.
>>
>> Assign the freed pages/page_ptr to NULL to catch possible
>> use after free with NULL pointer access.
>>
>> Signed-off-by: Kassey Li <[email protected]>
>> ---
>> drivers/android/binder_alloc.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> What commit id does this fix?
there is no specific commit id this change going to fix.
it is a follow up for commit
19c987241ca1216a51118b2bd0185b8bc5081783 binder: separate out
binder_alloc functions (mutex lock added for list access in alloc/free)
f2517eb76f1f2f7f89761f9db2b202e89931738c android: binder: Add global
lru shrinker to binder (set page->page_ptr = NULL;)
the background to raise this change that we are easy hit below crash
in monkey test:
where a wrong end is passing to
binder_update_page_range, thus calculate a weird index
for
page = &alloc->pages[index]
then causing the crash:
Unable to handle kernel paging request at virtual address 01ffff8724ade180
here this change will make sure the list access with mutex locked, and
freed pointer to NULL to easy the debug for such kind of issues.
pc : list_lru_add+0x30/0x11c
lr : list_lru_add+0x30/0x11c
sp : ffffffc0374bb9c0
x29: ffffffc0374bb9c0 x28: ffffff890dc82000 x27: ffffff89df890000
x26: ffffffdf957a86f8 x25: 000ffffff0b72e0c x24: ffffffdf965a4208
x23: ffffffdf95585008 x22: ffffff89d6170000 x21: ffffffdf965a4208
x20: 01ffff8724ade180 x19: ffffff8022340580 x18: ffffffdf957bae60
x17: 00000000529c6ef0 x16: 00000000529c6ef0 x15: 00000000226956ae
x14: 00000000e8903d35 x13: 00000000ebb92cf6 x12: ffffff89df890b50
x11: 00000000000000e0 x10: 0000000000000000 x9 : 0000000000000080
x8 : 00000000000000c0 x7 : 0000000000000000 x6 : ffffffdf93764970
x5 : 0000000000000000 x4 : 0000000000000000 x3 : 0000000000000000
x2 : 0000000000000001 x1 : ffffffdf94717c05 x0 : 0000000000000005
Call trace:
list_lru_add+0x30/0x11c
binder_update_page_range+0x154/0x994
binder_free_buf_locked+0x120/0x214
binder_alloc_free_buf+0xf4/0x118
binder_free_buf+0x284/0x390
binder_ioctl_write_read+0x1050/0x2d50
binder_ioctl+0x4f0/0x21c0
__arm64_sys_ioctl+0xa8/0xe4
invoke_syscall+0x58/0x114
el0_svc_common+0x94/0x118
do_el0_svc+0x2c/0xb8
el0_svc+0x30/0xb0
el0t_64_sync_handler+0x68/0xb4
el0t_64_sync+0x1a0/0x1a4
crash> list 0xFFFFFF890D063800 -s binder_buffer -x
ffffff890d063800
struct binder_buffer {
entry = {
next = 0xffffff8a6c0fde00,
prev = 0xffffff88e2e56ab0
},
rb_node = {
__rb_parent_color = 0xffffff895461a811,
rb_right = 0x0,
rb_left = 0x0
},
free = 0x0,
clear_on_free = 0x0,
allow_user_free = 0x0,
async_transaction = 0x0,
oneway_spam_suspect = 0x0,
debug_id = 0x5d16463,
transaction = 0x0,
target_node = 0xffffff898724b000,
data_size = 0x68,
offsets_size = 0x8,
extra_buffers_size = 0x0,
user_data = 0x7e63364000,
pid = 0x4950
}
ffffff8a6c0fde00
struct binder_buffer {
entry = {
next = 0xffffff895461a800,
prev = 0xffffff890d063800
},
rb_node = {
__rb_parent_color = 0xffffff895461a811,
rb_right = 0x0,
rb_left = 0xffffff89ebbcd610
},
free = 0x0,
clear_on_free = 0x0,
allow_user_free = 0x0,
async_transaction = 0x0,
oneway_spam_suspect = 0x0,
debug_id = 0x5d16467,
transaction = 0xffffff8a0435a300,
target_node = 0xffffff8978905b00,
data_size = 0x84,
offsets_size = 0x0,
extra_buffers_size = 0x0,
user_data = 0xffffff89d6171a00, // bad address
pid = 0x5584
}
>
> thanks,
>
> greg k-h
On Sat, Oct 07, 2023 at 07:07:40PM +0800, Kassey Li wrote:
>
>
> On 2023/10/7 14:44, Greg KH wrote:
> > On Sat, Oct 07, 2023 at 11:40:46AM +0800, Kassey Li wrote:
> > > Enforce alloc->mutex in binder_alloc_mmap_handler when add
> > > the entry to list.
> > >
> > > Assign the freed pages/page_ptr to NULL to catch possible
> > > use after free with NULL pointer access.
> > >
> > > Signed-off-by: Kassey Li <[email protected]>
> > > ---
> > > drivers/android/binder_alloc.c | 5 ++++-
> > > 1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > What commit id does this fix?
>
> there is no specific commit id this change going to fix.
>
> it is a follow up for commit
> 19c987241ca1216a51118b2bd0185b8bc5081783 binder: separate out binder_alloc
> functions (mutex lock added for list access in alloc/free)
> f2517eb76f1f2f7f89761f9db2b202e89931738c android: binder: Add global lru
> shrinker to binder (set page->page_ptr = NULL;)
>
> the background to raise this change that we are easy hit below crash in
> monkey test:
>
> where a wrong end is passing to
> binder_update_page_range, thus calculate a weird index
> for
> page = &alloc->pages[index]
Obviously it is a fix for some commit, please list that here.
thanks,
greg k-h
On 2023/10/7 19:18, Greg KH wrote:
> On Sat, Oct 07, 2023 at 07:07:40PM +0800, Kassey Li wrote:
>>
>>
>> On 2023/10/7 14:44, Greg KH wrote:
>>> On Sat, Oct 07, 2023 at 11:40:46AM +0800, Kassey Li wrote:
>>>> Enforce alloc->mutex in binder_alloc_mmap_handler when add
>>>> the entry to list.
>>>>
>>>> Assign the freed pages/page_ptr to NULL to catch possible
>>>> use after free with NULL pointer access.
>>>>
>>>> Signed-off-by: Kassey Li <[email protected]>
>>>> ---
>>>> drivers/android/binder_alloc.c | 5 ++++-
>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> What commit id does this fix?
>>
>> there is no specific commit id this change going to fix.
>>
>> it is a follow up for commit
>> 19c987241ca1216a51118b2bd0185b8bc5081783 binder: separate out binder_alloc
>> functions (mutex lock added for list access in alloc/free)
>> f2517eb76f1f2f7f89761f9db2b202e89931738c android: binder: Add global lru
>> shrinker to binder (set page->page_ptr = NULL;)
>>
>> the background to raise this change that we are easy hit below crash in
>> monkey test:
>>
>> where a wrong end is passing to
>> binder_update_page_range, thus calculate a weird index
>> for
>> page = &alloc->pages[index]
>
> Obviously it is a fix for some commit, please list that here.
ok, please kindly review this patch description according your
suggest, i can re-send v2 patch again.
commit 16aaeb8556ff4eb75823c56773ee82b06bac44a0 (HEAD -> master)
Author: Kassey Li <[email protected]>
Date: Thu Sep 28 10:42:52 2023 +0800
binder: add mutex_lock for mmap and NULL when free
-Enforce alloc->mutex in binder_alloc_mmap_handler when add
the entry to list.
-Assign the freed pages/page_ptr to NULL to catch possible
use after free with NULL pointer access.
Fixes: 19c987241ca1 ("binder: separate out binder_alloc functions")
Fixes: f2517eb76f1f ("android: binder: Add global lru shrinker to
binder")
Signed-off-by: Kassey Li <[email protected]>
>
> thanks,
>
> greg k-h
On Sat, Oct 07, 2023 at 07:34:05PM +0800, Kassey Li wrote:
>
>
> On 2023/10/7 19:18, Greg KH wrote:
> > On Sat, Oct 07, 2023 at 07:07:40PM +0800, Kassey Li wrote:
> > >
> > >
> > > On 2023/10/7 14:44, Greg KH wrote:
> > > > On Sat, Oct 07, 2023 at 11:40:46AM +0800, Kassey Li wrote:
> > > > > Enforce alloc->mutex in binder_alloc_mmap_handler when add
> > > > > the entry to list.
> > > > >
> > > > > Assign the freed pages/page_ptr to NULL to catch possible
> > > > > use after free with NULL pointer access.
> > > > >
> > > > > Signed-off-by: Kassey Li <[email protected]>
> > > > > ---
> > > > > drivers/android/binder_alloc.c | 5 ++++-
> > > > > 1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > What commit id does this fix?
> > >
> > > there is no specific commit id this change going to fix.
> > >
> > > it is a follow up for commit
> > > 19c987241ca1216a51118b2bd0185b8bc5081783 binder: separate out binder_alloc
> > > functions (mutex lock added for list access in alloc/free)
> > > f2517eb76f1f2f7f89761f9db2b202e89931738c android: binder: Add global lru
> > > shrinker to binder (set page->page_ptr = NULL;)
> > >
> > > the background to raise this change that we are easy hit below crash in
> > > monkey test:
> > >
> > > where a wrong end is passing to
> > > binder_update_page_range, thus calculate a weird index
> > > for
> > > page = &alloc->pages[index]
> >
> > Obviously it is a fix for some commit, please list that here.
>
> ok, please kindly review this patch description according your suggest, i
> can re-send v2 patch again.
>
> commit 16aaeb8556ff4eb75823c56773ee82b06bac44a0 (HEAD -> master)
> Author: Kassey Li <[email protected]>
> Date: Thu Sep 28 10:42:52 2023 +0800
>
> binder: add mutex_lock for mmap and NULL when free
>
> -Enforce alloc->mutex in binder_alloc_mmap_handler when add
> the entry to list.
>
> -Assign the freed pages/page_ptr to NULL to catch possible
> use after free with NULL pointer access.
Odd indentation :(
> Fixes: 19c987241ca1 ("binder: separate out binder_alloc functions")
> Fixes: f2517eb76f1f ("android: binder: Add global lru shrinker to
> binder")
> Signed-off-by: Kassey Li <[email protected]>
Looks better than before, thanks.
greg k-h
On 2023/10/7 19:37, Greg KH wrote:
> On Sat, Oct 07, 2023 at 07:34:05PM +0800, Kassey Li wrote:
>>
>>
>> On 2023/10/7 19:18, Greg KH wrote:
>>> On Sat, Oct 07, 2023 at 07:07:40PM +0800, Kassey Li wrote:
>>>>
>>>>
>>>> On 2023/10/7 14:44, Greg KH wrote:
>>>>> On Sat, Oct 07, 2023 at 11:40:46AM +0800, Kassey Li wrote:
>>>>>> Enforce alloc->mutex in binder_alloc_mmap_handler when add
>>>>>> the entry to list.
>>>>>>
>>>>>> Assign the freed pages/page_ptr to NULL to catch possible
>>>>>> use after free with NULL pointer access.
>>>>>>
>>>>>> Signed-off-by: Kassey Li <[email protected]>
>>>>>> ---
>>>>>> drivers/android/binder_alloc.c | 5 ++++-
>>>>>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> What commit id does this fix?
>>>>
>>>> there is no specific commit id this change going to fix.
>>>>
>>>> it is a follow up for commit
>>>> 19c987241ca1216a51118b2bd0185b8bc5081783 binder: separate out binder_alloc
>>>> functions (mutex lock added for list access in alloc/free)
>>>> f2517eb76f1f2f7f89761f9db2b202e89931738c android: binder: Add global lru
>>>> shrinker to binder (set page->page_ptr = NULL;)
>>>>
>>>> the background to raise this change that we are easy hit below crash in
>>>> monkey test:
>>>>
>>>> where a wrong end is passing to
>>>> binder_update_page_range, thus calculate a weird index
>>>> for
>>>> page = &alloc->pages[index]
>>>
>>> Obviously it is a fix for some commit, please list that here.
>>
>> ok, please kindly review this patch description according your suggest, i
>> can re-send v2 patch again.
>>
>> commit 16aaeb8556ff4eb75823c56773ee82b06bac44a0 (HEAD -> master)
>> Author: Kassey Li <[email protected]>
>> Date: Thu Sep 28 10:42:52 2023 +0800
>>
>> binder: add mutex_lock for mmap and NULL when free
>>
>> -Enforce alloc->mutex in binder_alloc_mmap_handler when add
>> the entry to list.
>>
>> -Assign the freed pages/page_ptr to NULL to catch possible
>> use after free with NULL pointer access.
>
> Odd indentation :(
thanks for the quick suggest and review, based on this version, will do
below and send patch v2.
./scripts/checkpatch.pl
0001-binder-add-mutex_lock-for-mmap-and-NULL-when-free.patch
total: 0 errors, 0 warnings, 26 lines checked
0001-binder-add-mutex_lock-for-mmap-and-NULL-when-free.patch has no
obvious style problems and is ready for submission.
>
>> Fixes: 19c987241ca1 ("binder: separate out binder_alloc functions")
>> Fixes: f2517eb76f1f ("android: binder: Add global lru shrinker to
>> binder")
>> Signed-off-by: Kassey Li <[email protected]>
>
> Looks better than before, thanks.
>
> greg k-h