linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* WARNING in binder_transaction_buffer_release (2)
@ 2020-05-27  8:54 syzbot
  2020-08-06 11:19 ` syzbot
  0 siblings, 1 reply; 4+ messages in thread
From: syzbot @ 2020-05-27  8:54 UTC (permalink / raw)
  To: arve, christian, devel, gregkh, joel, linux-kernel, maco,
	syzkaller-bugs, tkjos, tkjos

Hello,

syzbot found the following crash on:

HEAD commit:    44456565 Merge tag 'io_uring-5.7-2020-05-22' of git://git...
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=12990cba100000
kernel config:  https://syzkaller.appspot.com/x/.config?x=b3368ce0cc5f5ace
dashboard link: https://syzkaller.appspot.com/bug?extid=e113a0b970b7b3f394ba
compiler:       gcc (GCC) 9.0.0 20181231 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=165b01e2100000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14706016100000

The bug was bisected to:

commit 44d8047f1d87adc2fd7eccc88533794f6d88c15e
Author: Todd Kjos <tkjos@android.com>
Date:   Tue Aug 28 20:46:25 2018 +0000

    binder: use standard functions to allocate fds

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=134e254a100000
final crash:    https://syzkaller.appspot.com/x/report.txt?x=10ce254a100000
console output: https://syzkaller.appspot.com/x/log.txt?x=174e254a100000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+e113a0b970b7b3f394ba@syzkaller.appspotmail.com
Fixes: 44d8047f1d87 ("binder: use standard functions to allocate fds")

------------[ cut here ]------------
WARNING: CPU: 1 PID: 7071 at drivers/android/binder.c:2348 binder_transaction_buffer_release+0x601/0x8a0 drivers/android/binder.c:2348
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 7071 Comm: syz-executor142 Not tainted 5.7.0-rc6-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:77 [inline]
 dump_stack+0x188/0x20d lib/dump_stack.c:118
 panic+0x2e3/0x75c kernel/panic.c:221
 __warn.cold+0x2f/0x35 kernel/panic.c:582
 report_bug+0x27b/0x2f0 lib/bug.c:195
 fixup_bug arch/x86/kernel/traps.c:175 [inline]
 fixup_bug arch/x86/kernel/traps.c:170 [inline]
 do_error_trap+0x12b/0x220 arch/x86/kernel/traps.c:267
 do_invalid_op+0x32/0x40 arch/x86/kernel/traps.c:286
 invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1027
RIP: 0010:binder_transaction_buffer_release+0x601/0x8a0 drivers/android/binder.c:2348
Code: bb fd 31 ff 41 89 c5 89 c6 e8 bb ff 81 fb 45 85 ed 0f 85 f9 4b 01 00 48 8d 45 40 48 89 44 24 28 e9 fa fa ff ff e8 2f fe 81 fb <0f> 0b e9 87 fc ff ff e8 23 fe 81 fb 4c 8b 44 24 20 48 89 d8 45 31
RSP: 0018:ffffc900018e7620 EFLAGS: 00010293
RAX: ffff8880a12da5c0 RBX: 0000000000000058 RCX: 1ffff1101425b55b
RDX: 0000000000000000 RSI: ffffffff85f136f1 RDI: ffff88809db1c048
RBP: ffff88809290a080 R08: ffff8880a12da5c0 R09: fffff5200031cee7
R10: ffffc900018e7737 R11: fffff5200031cee6 R12: ffff88809187a040
R13: 0000000000000060 R14: ffff88809db1c000 R15: 0000000000000060
 binder_transaction+0x146d/0x6500 drivers/android/binder.c:3486
 binder_thread_write+0x818/0x2560 drivers/android/binder.c:3796
 binder_ioctl_write_read drivers/android/binder.c:4847 [inline]
 binder_ioctl+0x1008/0x1862 drivers/android/binder.c:5024
 vfs_ioctl fs/ioctl.c:47 [inline]
 ksys_ioctl+0x11a/0x180 fs/ioctl.c:771
 __do_sys_ioctl fs/ioctl.c:780 [inline]
 __se_sys_ioctl fs/ioctl.c:778 [inline]
 __x64_sys_ioctl+0x6f/0xb0 fs/ioctl.c:778
 do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295
 entry_SYSCALL_64_after_hwframe+0x49/0xb3
RIP: 0033:0x44b749
Code: e8 5c d9 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 bb d0 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f65c624dce8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00000000006ddc48 RCX: 000000000044b749
RDX: 0000000020000540 RSI: 00000000c0306201 RDI: 0000000000000003
RBP: 00000000006ddc40 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000006ddc4c
R13: 00007ffd9783511f R14: 00007f65c624e9c0 R15: 00000000006ddc4c
Kernel Offset: disabled
Rebooting in 86400 seconds..


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: WARNING in binder_transaction_buffer_release (2)
  2020-05-27  8:54 WARNING in binder_transaction_buffer_release (2) syzbot
@ 2020-08-06 11:19 ` syzbot
  2020-08-06 16:08   ` Jann Horn
  0 siblings, 1 reply; 4+ messages in thread
From: syzbot @ 2020-08-06 11:19 UTC (permalink / raw)
  To: arve, christian, devel, gregkh, jannh, joel, linux-kernel, maco,
	syzkaller-bugs, tkjos, tkjos

syzbot suspects this issue was fixed by commit:

commit 4b836a1426cb0f1ef2a6e211d7e553221594f8fc
Author: Jann Horn <jannh@google.com>
Date:   Mon Jul 27 12:04:24 2020 +0000

    binder: Prevent context manager from incrementing ref 0

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=10c84dec900000
start commit:   9cb1fd0e Linux 5.7-rc7
git tree:       upstream
kernel config:  https://syzkaller.appspot.com/x/.config?x=cca7550d53ffa599
dashboard link: https://syzkaller.appspot.com/bug?extid=e113a0b970b7b3f394ba
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1230353c100000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17fd535e100000

If the result looks correct, please mark the issue as fixed by replying with:

#syz fix: binder: Prevent context manager from incrementing ref 0

For information about bisection process see: https://goo.gl/tpsmEJ#bisection

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: WARNING in binder_transaction_buffer_release (2)
  2020-08-06 11:19 ` syzbot
@ 2020-08-06 16:08   ` Jann Horn
  2020-08-06 16:14     ` Todd Kjos
  0 siblings, 1 reply; 4+ messages in thread
From: Jann Horn @ 2020-08-06 16:08 UTC (permalink / raw)
  To: syzbot, Arve Hjønnevåg, Joel Fernandes (Google),
	Martijn Coenen, Todd Kjos
  Cc: Christian Brauner, open list:ANDROID DRIVERS, Greg Kroah-Hartman,
	kernel list, syzkaller-bugs

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: WARNING in binder_transaction_buffer_release (2)
  2020-08-06 16:08   ` Jann Horn
@ 2020-08-06 16:14     ` Todd Kjos
  0 siblings, 0 replies; 4+ messages in thread
From: Todd Kjos @ 2020-08-06 16:14 UTC (permalink / raw)
  To: Jann Horn
  Cc: syzbot, Arve Hjønnevåg, Joel Fernandes (Google),
	Martijn Coenen, Todd Kjos, Christian Brauner,
	open list:ANDROID DRIVERS, Greg Kroah-Hartman, kernel list,
	syzkaller-bugs

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2020-08-06 16:59 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27  8:54 WARNING in binder_transaction_buffer_release (2) syzbot
2020-08-06 11:19 ` syzbot
2020-08-06 16:08   ` Jann Horn
2020-08-06 16:14     ` Todd Kjos

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).