[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAMA88TqDQH4c-wohARJbXN-jUHZYtFaFpbJ3dLzxG_HxvGuj5A@mail.gmail.com>
Date: Wed, 18 May 2022 09:29:45 +0800
From: Schspa Shi <schspa@...il.com>
To: Carlos Llamas <cmllamas@...gle.com>
Cc: gregkh@...uxfoundation.org, arve@...roid.com, tkjos@...roid.com,
maco@...roid.com, joel@...lfernandes.org, brauner@...nel.org,
hridya@...gle.com, surenb@...gle.com, linux-kernel@...r.kernel.org,
syzbot+46fff6434a7f968ecb39@...kaller.appspotmail.com
Subject: Re: [PATCH] binder: fix atomic sleep when get extended error
Carlos Llamas <cmllamas@...gle.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@...kaller.appspotmail.com
>> Fixes: bd32889e841c ("binder: add BINDER_GET_EXTENDED_ERROR ioctl")
>> Signed-off-by: Schspa Shi <schspa@...il.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
Powered by blists - more mailing lists