WARNING in binder_transaction_buffer_release (2)

[Todd Kjos]

On Thu, Aug 6, 2020 at 9:09 AM Jann Horn <jannh at google.com> wrote:
>
> On Thu, Aug 6, 2020 at 1:19 PM syzbot
> <syzbot+e113a0b970b7b3f394ba at syzkaller.appspotmail.com> wrote:
> > syzbot suspects this issue was fixed by commit:
> >
> > commit 4b836a1426cb0f1ef2a6e211d7e553221594f8fc
> > Author: Jann Horn <jannh at google.com>
> > Date:   Mon Jul 27 12:04:24 2020 +0000
> >
> >     binder: Prevent context manager from incrementing ref 0
> [...]
> > dashboard link: https://syzkaller.appspot.com/bug?extid=e113a0b970b7b3f394ba
> [...]
> > If the result looks correct, please mark the issue as fixed by replying with:
> >
> > #syz fix: binder: Prevent context manager from incrementing ref 0
>
> I think this issue still exists, syzbot probably just hit it in a
> weird way that doesn't work anymore.
>
> This warning:
>
> case BINDER_TYPE_FD: {
>         /*
>          * No need to close the file here since user-space
>          * closes it for for successfully delivered
>          * transactions. For transactions that weren't
>          * delivered, the new fd was never allocated so
>          * there is no need to close and the fput on the
>          * file is done when the transaction is torn
>          * down.
>          */
>         WARN_ON(failed_at &&
>                 proc->tsk == current->group_leader);
> } break;
>
> can be false-positive if the sender and recipient of the transaction
> are associated with the same task_struct. But there isn't really any
> reason why you wouldn't be able to have sender and recipient in the
> same process, as long as the binder_proc is different.
> (binder_transaction() has a weird check that refuses transactions to
> handle 0 based on task_struct equality - which IMO doesn't really make
> sense -, but transactions to other handles can happen just fine even
> if both ends are in the same task_struct.)
>
> Maybe the best fix is just to rip out that WARN_ON()?

Yes, probably so.