Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <20220517175225.65283-1-schspa@gmail.com> <YoP1LSSzgg9sHJDW@google.com>
In-Reply-To: <YoP1LSSzgg9sHJDW@google.com>
From:   Schspa Shi <schspa@gmail.com>
Date:   Wed, 18 May 2022 09:29:45 +0800
Message-ID: <CAMA88TqDQH4c-wohARJbXN-jUHZYtFaFpbJ3dLzxG_HxvGuj5A@mail.gmail.com>
Subject: Re: [PATCH] binder: fix atomic sleep when get extended error
To:     Carlos Llamas <cmllamas@google.com>
Cc:     gregkh@linuxfoundation.org, arve@android.com, tkjos@android.com,
        maco@android.com, joel@joelfernandes.org, brauner@kernel.org,
        hridya@google.com, surenb@google.com, linux-kernel@vger.kernel.org,
        syzbot+46fff6434a7f968ecb39@syzkaller.appspotmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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