WARNING in binder_transaction_buffer_release (2)

[syzbot]

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 <tk...@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+e113a0...@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 syzk...@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

[syzbot]

syzbot suspects this issue was fixed by commit:

commit 4b836a1426cb0f1ef2a6e211d7e553221594f8fc
Author: Jann Horn <ja...@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

[Jann Horn]

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

[Todd Kjos]

Yes, probably so.