linux-kernel - Re: WARNING in binder_transaction_buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG48ez0Rdut=HxSyKUCiCVU=zZJPR7YXcOfL3tRBXXwUS0iKxw@mail.gmail.com>
Date:   Thu, 6 Aug 2020 18:08:36 +0200
From:   Jann Horn <jannh@...gle.com>
To:     syzbot <syzbot+e113a0b970b7b3f394ba@...kaller.appspotmail.com>,
        Arve Hjønnevåg <arve@...roid.com>,
        "Joel Fernandes (Google)" <joel@...lfernandes.org>,
        Martijn Coenen <maco@...roid.com>,
        Todd Kjos <tkjos@...roid.com>
Cc:     Christian Brauner <christian@...uner.io>,
        "open list:ANDROID DRIVERS" <devel@...verdev.osuosl.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        kernel list <linux-kernel@...r.kernel.org>,
        syzkaller-bugs <syzkaller-bugs@...glegroups.com>
Subject: Re: WARNING in binder_transaction_buffer_release (2)

On Thu, Aug 6, 2020 at 1:19 PM syzbot
<syzbot+e113a0b970b7b3f394ba@...kaller.appspotmail.com> wrote:
> syzbot suspects this issue was fixed by commit:
>
> commit 4b836a1426cb0f1ef2a6e211d7e553221594f8fc
> Author: Jann Horn <jannh@...gle.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()?