[PATCH] binder: add mutex_lock for mmap and NULL when free

[PATCH] binder: add mutex_lock for mmap and NULL when free

[Kassey Li]

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 <quic_yingangl@quicinc.com>
---
 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

Re: [PATCH] binder: add mutex_lock for mmap and NULL when free

[Greg KH]

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 <quic_yingangl@quicinc.com>
> ---
>  drivers/android/binder_alloc.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

What commit id does this fix?

thanks,

greg k-h

Re: [PATCH] binder: add mutex_lock for mmap and NULL when free

[Kassey Li]

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 <quic_yingangl@quicinc.com>
>> ---
>>   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

Re: [PATCH] binder: add mutex_lock for mmap and NULL when free

[Greg KH]

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 <quic_yingangl@quicinc.com>
> > > ---
> > >   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

Re: [PATCH] binder: add mutex_lock for mmap and NULL when free

[Kassey Li]

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 <quic_yingangl@quicinc.com>
>>>> ---
>>>>    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 <quic_yingangl@quicinc.com>
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 <quic_yingangl@quicinc.com>

> 
> thanks,
> 
> greg k-h

Re: [PATCH] binder: add mutex_lock for mmap and NULL when free

[Greg KH]

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 <quic_yingangl@quicinc.com>
> > > > > ---
> > > > >    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 <quic_yingangl@quicinc.com>
> 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 <quic_yingangl@quicinc.com>

Looks better than before, thanks.

greg k-h

Re: [PATCH] binder: add mutex_lock for mmap and NULL when free

[Kassey Li]

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 <quic_yingangl@quicinc.com>
>>>>>> ---
>>>>>>     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 <quic_yingangl@quicinc.com>
>> 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 <quic_yingangl@quicinc.com>
> 
> Looks better than before, thanks.
> 
> greg k-h