lkml.org 
[lkml]   [2022]   [May]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] binder: fix atomic sleep when get extended error
Carlos Llamas <cmllamas@google.com> writes:

> On Wed, May 18, 2022 at 01:52:25AM +0800, Schspa Shi wrote:
>> binder_inner_proc_lock(thread->proc) is a spin lock, copy_to_user can't
>> be called with in this lock.
>>
>> Copy it as a local variable, and check the id to make sure the user space
>> gets the latest error message
>>
>> Reported-by: syzbot+46fff6434a7f968ecb39@syzkaller.appspotmail.com
>> Fixes: bd32889e841c ("binder: add BINDER_GET_EXTENDED_ERROR ioctl")
>> Signed-off-by: Schspa Shi <schspa@gmail.com>
>> ---
>> drivers/android/binder.c | 13 ++++++++++---
>> 1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index d9253b2a7bd9..5f2e1fa3da74 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -5163,13 +5163,20 @@ static int binder_ioctl_get_extended_error(struct binder_thread *thread,
>> void __user *ubuf)
>> {
>> struct binder_extended_error *ee = &thread->ee;
>> + struct binder_extended_error eeb;
>>
>> binder_inner_proc_lock(thread->proc);
>> - if (copy_to_user(ubuf, ee, sizeof(*ee))) {
>> - binder_inner_proc_unlock(thread->proc);
>> +retry:
>> + eeb = *ee;
>> + binder_inner_proc_unlock(thread->proc);
>> + if (copy_to_user(ubuf, &eeb, sizeof(eeb)))
>> return -EFAULT;
>> - }
>>
>> + binder_inner_proc_lock(thread->proc);
>> + if (eeb.id != ee->id) {
>> + /* retry to get newest error info */
>> + goto retry;
>> + }
>> ee->id = 0;
>> ee->command = BR_OK;
>> ee->param = 0;
>> --
>> 2.24.3 (Apple Git-128)
>>
>
> Oops! Thank you for your patch. In this case the local copy would be
> enough, no need for the retry logic as this is already taken care of.
> You could also leverage the binder_set_extended_error() to reset the
> thread->ee. Would you mind sending these updates? I was thinking
> something like this:

Yes, I have sent a v2 patch for this, please review it.


>
> drivers/android/binder.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index 83facfa1a5c3..f92021cd384b 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -5163,19 +5163,16 @@ static int binder_ioctl_get_freezer_info(
> static int binder_ioctl_get_extended_error(struct binder_thread *thread,
> void __user *ubuf)
> {
> - struct binder_extended_error *ee = &thread->ee;
> + struct binder_extended_error ee;
>
> binder_inner_proc_lock(thread->proc);
> - if (copy_to_user(ubuf, ee, sizeof(*ee))) {
> - binder_inner_proc_unlock(thread->proc);
> - return -EFAULT;
> - }
> -
> - ee->id = 0;
> - ee->command = BR_OK;
> - ee->param = 0;
> + ee = thread->ee;
> + binder_set_extended_error(&thread->ee, 0, BR_OK, 0);
> binder_inner_proc_unlock(thread->proc);
>
> + if (copy_to_user(ubuf, &ee, sizeof(ee)))
> + return -EFAULT;
> +
> return 0;
> }

--
BRs
Schspa Shi

\
 
 \ /
  Last update: 2022-05-18 03:30    [W:0.040 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site