* [PATCH] binder: fix atomic sleep when get extended error
@ 2022-05-17 17:52 Schspa Shi
2022-05-17 19:19 ` Carlos Llamas
0 siblings, 1 reply; 3+ messages in thread
From: Schspa Shi @ 2022-05-17 17:52 UTC (permalink / raw)
To: gregkh, arve, tkjos, maco, joel, brauner, hridya, surenb, cmllamas
Cc: linux-kernel, schspa, syzbot+46fff6434a7f968ecb39
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)
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] binder: fix atomic sleep when get extended error
2022-05-17 17:52 [PATCH] binder: fix atomic sleep when get extended error Schspa Shi
@ 2022-05-17 19:19 ` Carlos Llamas
2022-05-18 1:29 ` Schspa Shi
0 siblings, 1 reply; 3+ messages in thread
From: Carlos Llamas @ 2022-05-17 19:19 UTC (permalink / raw)
To: Schspa Shi
Cc: gregkh, arve, tkjos, maco, joel, brauner, hridya, surenb,
linux-kernel, syzbot+46fff6434a7f968ecb39
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:
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;
}
--
Carlos Llamas
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] binder: fix atomic sleep when get extended error
2022-05-17 19:19 ` Carlos Llamas
@ 2022-05-18 1:29 ` Schspa Shi
0 siblings, 0 replies; 3+ messages in thread
From: Schspa Shi @ 2022-05-18 1:29 UTC (permalink / raw)
To: Carlos Llamas
Cc: gregkh, arve, tkjos, maco, joel, brauner, hridya, surenb,
linux-kernel, syzbot+46fff6434a7f968ecb39
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-05-18 1:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-17 17:52 [PATCH] binder: fix atomic sleep when get extended error Schspa Shi
2022-05-17 19:19 ` Carlos Llamas
2022-05-18 1:29 ` Schspa Shi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.