[syzbot] WARNING: refcount bug in memfd_secret

[syzbot] WARNING: refcount bug in memfd_secret

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    9c0c4d24ac00 Merge tag 'block-5.15-2021-10-22' of git://gi..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=115a0328b00000
kernel config:  https://syzkaller.appspot.com/x/.config?x=59f3ef2b4077575
dashboard link: https://syzkaller.appspot.com/bug?extid=75639e6a0331cd61d3e2
compiler:       Debian clang version 11.0.1-2, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=13a035c2b00000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14ae869f300000

The issue was bisected to:

commit 110860541f443f950c1274f217a1a3e298670a33
Author: Jordy Zomer <jordy@jordyzomer.github.io>
Date:   Wed Sep 8 02:56:18 2021 +0000

    mm/secretmem: use refcount_t instead of atomic_t

bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=1433ea1cb00000
final oops:     https://syzkaller.appspot.com/x/report.txt?x=1633ea1cb00000
console output: https://syzkaller.appspot.com/x/log.txt?x=1233ea1cb00000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+75639e6a0331cd61d3e2@syzkaller.appspotmail.com
Fixes: 110860541f44 ("mm/secretmem: use refcount_t instead of atomic_t")

------------[ cut here ]------------
refcount_t: addition on 0; use-after-free.
WARNING: CPU: 1 PID: 6529 at lib/refcount.c:25 refcount_warn_saturate+0x13d/0x1a0 lib/refcount.c:25
Modules linked in:
CPU: 1 PID: 6529 Comm: syz-executor563 Not tainted 5.15.0-rc6-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:refcount_warn_saturate+0x13d/0x1a0 lib/refcount.c:25
Code: c7 80 e1 b3 8a 31 c0 e8 31 8e 3b fd 0f 0b eb a3 e8 b8 79 71 fd c6 05 43 86 b6 09 01 48 c7 c7 e0 e1 b3 8a 31 c0 e8 13 8e 3b fd <0f> 0b eb 85 e8 9a 79 71 fd c6 05 26 86 b6 09 01 48 c7 c7 40 e2 b3
RSP: 0018:ffffc900012dfed0 EFLAGS: 00010246
RAX: f8bc46cda99bde00 RBX: 0000000000000002 RCX: ffff88801e03d580
RDX: 0000000000000000 RSI: 0000000080000000 RDI: 0000000000000000
RBP: 0000000000000002 R08: ffffffff81695fe2 R09: ffffed10173a57a8
R10: ffffed10173a57a8 R11: 0000000000000000 R12: ffff888078595180
R13: ffff88806f5e8030 R14: 0000000000000003 R15: dffffc0000000000
FS:  00005555558b2300(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ff91ea21000 CR3: 000000007077c000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 __refcount_inc include/linux/refcount.h:250 [inline]
 refcount_inc include/linux/refcount.h:267 [inline]
 __do_sys_memfd_secret mm/secretmem.c:221 [inline]
 __se_sys_memfd_secret+0x2ea/0x350 mm/secretmem.c:194
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x44/0xd0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x7f09de6c7f79
Code: 28 c3 e8 2a 14 00 00 66 2e 0f 1f 84 00 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 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffe81d58a98 EFLAGS: 00000246 ORIG_RAX: 00000000000001bf
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f09de6c7f79
RDX: 00007f09de68ae83 RSI: 0000000000000012 RDI: 0000000000000000
RBP: 00007f09de68bf60 R08: 0000000000000000 R09: 0000000000000000
R10: 00000000ffffffff R11: 0000000000000246 R12: 00007f09de68bff0
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000


---
This report 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 issue. 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 issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

Re: [syzbot] WARNING: refcount bug in memfd_secret

[Linus Torvalds]

On Sat, Oct 23, 2021 at 9:35 AM syzbot
<syzbot+75639e6a0331cd61d3e2@syzkaller.appspotmail.com> wrote:
>
> syzbot found the following issue on:
>
> HEAD commit:    9c0c4d24ac00 Merge tag 'block-5.15-2021-10-22' of git://gi..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=115a0328b00000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=59f3ef2b4077575
> dashboard link: https://syzkaller.appspot.com/bug?extid=75639e6a0331cd61d3e2
> compiler:       Debian clang version 11.0.1-2, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=13a035c2b00000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14ae869f300000
>
> The issue was bisected to:
>
> commit 110860541f443f950c1274f217a1a3e298670a33

I think that commit is actually just buggy.

"secretmem_users" is not actually a reference count. There's no "magic
happens when it goes down to zero".

It's purely a count of the number of existing users, and incrementing
it from zero is not a probolem at all - it is in fact expected.

Sure, zero means "we can hibernate", so zero and overflow are somewhat
special, but not special enough to cause these kinds of issues.

I have reverted this commit in my tree, because honestly, the whole
"try to overflow exactly, and hibernate" threat model just isn't worth
this all.

If people really care, I can suggest

 - use "atomic_long_t" instead. Let's face it, 32-bit isn't
interesting any more, and 64-bit doesn't overflow.

 - make up some new "atomic_inc_nooverflow()" thing or whatever.

but for now this is just reverted.

             Linus

Re: [syzbot] WARNING: refcount bug in memfd_secret

[Matthew Wilcox]

On Sun, Oct 24, 2021 at 09:54:22AM -1000, Linus Torvalds wrote:
> On Sat, Oct 23, 2021 at 9:35 AM syzbot
> <syzbot+75639e6a0331cd61d3e2@syzkaller.appspotmail.com> wrote:
> >
> > syzbot found the following issue on:
> >
> > HEAD commit:    9c0c4d24ac00 Merge tag 'block-5.15-2021-10-22' of git://gi..
> > git tree:       upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=115a0328b00000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=59f3ef2b4077575
> > dashboard link: https://syzkaller.appspot.com/bug?extid=75639e6a0331cd61d3e2
> > compiler:       Debian clang version 11.0.1-2, GNU ld (GNU Binutils for Debian) 2.35.2
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=13a035c2b00000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14ae869f300000
> >
> > The issue was bisected to:
> >
> > commit 110860541f443f950c1274f217a1a3e298670a33
> 
> I think that commit is actually just buggy.
> 
> "secretmem_users" is not actually a reference count. There's no "magic
> happens when it goes down to zero".
> 
> It's purely a count of the number of existing users, and incrementing
> it from zero is not a probolem at all - it is in fact expected.
> 
> Sure, zero means "we can hibernate", so zero and overflow are somewhat
> special, but not special enough to cause these kinds of issues.
> 
> I have reverted this commit in my tree, because honestly, the whole
> "try to overflow exactly, and hibernate" threat model just isn't worth
> this all.
> 
> If people really care, I can suggest
> 
>  - use "atomic_long_t" instead. Let's face it, 32-bit isn't
> interesting any more, and 64-bit doesn't overflow.
> 
>  - make up some new "atomic_inc_nooverflow()" thing or whatever.
> 
> but for now this is just reverted.

There was a separate thread on an earlier version of this report.
https://lore.kernel.org/linux-mm/YXU7%2FiRjf9v77gon@casper.infradead.org/
I agree with you and suggested that if anybody really cares (I mean,
you need a multi-TB machine to produce this problem) that we simply do
what we did with the page refcount:

+++ b/mm/secretmem.c
@@ -203,6 +203,8 @@ SYSCALL_DEFINE1(memfd_secret, unsigned int, flags)

        if (flags & ~(SECRETMEM_FLAGS_MASK | O_CLOEXEC))
                return -EINVAL;
+       if (atomic_read(&secretmem_users) < 0)
+               return -ENFILE;

        fd = get_unused_fd_flags(flags & O_CLOEXEC);
        if (fd < 0)

Mike didn't particularly like that answer though.

Re: [syzbot] WARNING: refcount bug in memfd_secret

[Dmitry Vyukov]

On Sun, 24 Oct 2021 at 22:12, Matthew Wilcox <willy@infradead.org> wrote:
>
> On Sun, Oct 24, 2021 at 09:54:22AM -1000, Linus Torvalds wrote:
> > On Sat, Oct 23, 2021 at 9:35 AM syzbot
> > <syzbot+75639e6a0331cd61d3e2@syzkaller.appspotmail.com> wrote:
> > >
> > > syzbot found the following issue on:
> > >
> > > HEAD commit:    9c0c4d24ac00 Merge tag 'block-5.15-2021-10-22' of git://gi..
> > > git tree:       upstream
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=115a0328b00000
> > > kernel config:  https://syzkaller.appspot.com/x/.config?x=59f3ef2b4077575
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=75639e6a0331cd61d3e2
> > > compiler:       Debian clang version 11.0.1-2, GNU ld (GNU Binutils for Debian) 2.35.2
> > > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=13a035c2b00000
> > > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14ae869f300000
> > >
> > > The issue was bisected to:
> > >
> > > commit 110860541f443f950c1274f217a1a3e298670a33
> >
> > I think that commit is actually just buggy.
> >
> > "secretmem_users" is not actually a reference count. There's no "magic
> > happens when it goes down to zero".
> >
> > It's purely a count of the number of existing users, and incrementing
> > it from zero is not a probolem at all - it is in fact expected.
> >
> > Sure, zero means "we can hibernate", so zero and overflow are somewhat
> > special, but not special enough to cause these kinds of issues.
> >
> > I have reverted this commit in my tree, because honestly, the whole
> > "try to overflow exactly, and hibernate" threat model just isn't worth
> > this all.
> >
> > If people really care, I can suggest
> >
> >  - use "atomic_long_t" instead. Let's face it, 32-bit isn't
> > interesting any more, and 64-bit doesn't overflow.
> >
> >  - make up some new "atomic_inc_nooverflow()" thing or whatever.
> >
> > but for now this is just reverted.
>
> There was a separate thread on an earlier version of this report.

gcc and clang somehow produce different frames (I guess a tail call).
Need to fix parsing.

#syz dup: WARNING: refcount bug in sys_memfd_secret


> https://lore.kernel.org/linux-mm/YXU7%2FiRjf9v77gon@casper.infradead.org/
> I agree with you and suggested that if anybody really cares (I mean,
> you need a multi-TB machine to produce this problem) that we simply do
> what we did with the page refcount:
>
> +++ b/mm/secretmem.c
> @@ -203,6 +203,8 @@ SYSCALL_DEFINE1(memfd_secret, unsigned int, flags)
>
>         if (flags & ~(SECRETMEM_FLAGS_MASK | O_CLOEXEC))
>                 return -EINVAL;
> +       if (atomic_read(&secretmem_users) < 0)
> +               return -ENFILE;
>
>         fd = get_unused_fd_flags(flags & O_CLOEXEC);
>         if (fd < 0)
>
> Mike didn't particularly like that answer though.

Re: [syzbot] WARNING: refcount bug in memfd_secret

[Linus Torvalds]

On Sun, Oct 24, 2021 at 1:10 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> I agree with you and suggested that if anybody really cares (I mean,
> you need a multi-TB machine to produce this problem) that we simply do
> what we did with the page refcount:

Ack. That looks like the right thing to do.

> +       if (atomic_read(&secretmem_users) < 0)
> +               return -ENFILE;

If somebody sends me a proper patch with a sign-off etc, I'll happily
apply that two-liner. Hint hint.

              Linus