Messages in this thread | | | From | Schspa Shi <> | Date | Wed, 18 May 2022 09:29:45 +0800 | Subject | Re: [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
| |