KASAN: use-after-free Read in __fput (3)

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 15bc20c6 Merge tag 'tty-5.9-rc3' of git://git.kernel.org/p..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=10b440d1900000
kernel config: https://syzkaller.appspot.com/x/.config?x=978db74cb30aa994
dashboard link: https://syzkaller.appspot.com/bug?extid=c282923e5da93549fa27
compiler: gcc (GCC) 10.1.0-syz 20200507
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16a5452e900000

The issue was bisected to:

commit a9ed4a6560b8562b7e2e2bed9527e88001f7b682
Author: Marc Zyngier <m...@kernel.org>
Date: Wed Aug 19 16:12:17 2020 +0000

epoll: Keep a reference on files added to the check list

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=147a02f2900000
final oops: https://syzkaller.appspot.com/x/report.txt?x=167a02f2900000
console output: https://syzkaller.appspot.com/x/log.txt?x=127a02f2900000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+c28292...@syzkaller.appspotmail.com
Fixes: a9ed4a6560b8 ("epoll: Keep a reference on files added to the check list")

==================================================================
BUG: KASAN: use-after-free in d_inode include/linux/dcache.h:522 [inline]
BUG: KASAN: use-after-free in fsnotify_parent include/linux/fsnotify.h:54 [inline]
BUG: KASAN: use-after-free in fsnotify_file include/linux/fsnotify.h:90 [inline]
BUG: KASAN: use-after-free in fsnotify_close include/linux/fsnotify.h:279 [inline]
BUG: KASAN: use-after-free in __fput+0x8ac/0x920 fs/file_table.c:267
Read of size 8 at addr ffff888087a020a8 by task syz-executor.2/11261

CPU: 0 PID: 11261 Comm: syz-executor.2 Not tainted 5.9.0-rc2-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+0x18f/0x20d lib/dump_stack.c:118
print_address_description.constprop.0.cold+0xae/0x497 mm/kasan/report.c:383
__kasan_report mm/kasan/report.c:513 [inline]
kasan_report.cold+0x1f/0x37 mm/kasan/report.c:530
d_inode include/linux/dcache.h:522 [inline]
fsnotify_parent include/linux/fsnotify.h:54 [inline]
fsnotify_file include/linux/fsnotify.h:90 [inline]
fsnotify_close include/linux/fsnotify.h:279 [inline]
__fput+0x8ac/0x920 fs/file_table.c:267
task_work_run+0xdd/0x190 kernel/task_work.c:141
tracehook_notify_resume include/linux/tracehook.h:188 [inline]
exit_to_user_mode_loop kernel/entry/common.c:140 [inline]
exit_to_user_mode_prepare+0x195/0x1c0 kernel/entry/common.c:167
syscall_exit_to_user_mode+0x59/0x2b0 kernel/entry/common.c:242
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x416f01
Code: 75 14 b8 03 00 00 00 0f 05 48 3d 01 f0 ff ff 0f 83 04 1b 00 00 c3 48 83 ec 08 e8 0a fc ff ff 48 89 04 24 b8 03 00 00 00 0f 05 <48> 8b 3c 24 48 89 c2 e8 53 fc ff ff 48 89 d0 48 83 c4 08 48 3d 01
RSP: 002b:00007fff215c6f90 EFLAGS: 00000293 ORIG_RAX: 0000000000000003
RAX: 0000000000000000 RBX: 0000000000000004 RCX: 0000000000416f01
RDX: 00000000000f4240 RSI: 0000000000000081 RDI: 0000000000000003
RBP: 0000000000000000 R08: 0000000001190358 R09: 0000000000000000
R10: 00007fff215c7070 R11: 0000000000000293 R12: 0000000001190360
R13: 0000000000000000 R14: ffffffffffffffff R15: 000000000118cf4c

Allocated by task 11262:
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
kasan_set_track mm/kasan/common.c:56 [inline]
__kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:461
slab_post_alloc_hook mm/slab.h:518 [inline]
slab_alloc mm/slab.c:3312 [inline]
kmem_cache_alloc+0x138/0x3a0 mm/slab.c:3482
__d_alloc+0x2a/0x950 fs/dcache.c:1709
d_alloc_pseudo+0x19/0x70 fs/dcache.c:1838
alloc_file_pseudo+0xc6/0x250 fs/file_table.c:226
sock_alloc_file+0x4f/0x190 net/socket.c:411
sock_map_fd net/socket.c:435 [inline]
__sys_socket+0x13d/0x200 net/socket.c:1524
__do_sys_socket net/socket.c:1529 [inline]
__se_sys_socket net/socket.c:1527 [inline]
__x64_sys_socket+0x6f/0xb0 net/socket.c:1527
do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x44/0xa9

Freed by task 11262:
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
kasan_set_track+0x1c/0x30 mm/kasan/common.c:56
kasan_set_free_info+0x1b/0x30 mm/kasan/generic.c:355
__kasan_slab_free+0xd8/0x120 mm/kasan/common.c:422
__cache_free mm/slab.c:3418 [inline]
kmem_cache_free.part.0+0x67/0x1f0 mm/slab.c:3693
__d_free fs/dcache.c:271 [inline]
dentry_free+0xde/0x160 fs/dcache.c:348
__dentry_kill+0x4c6/0x640 fs/dcache.c:593
dentry_kill fs/dcache.c:705 [inline]
dput+0x725/0xbc0 fs/dcache.c:878
__fput+0x3ab/0x920 fs/file_table.c:294
task_work_run+0xdd/0x190 kernel/task_work.c:141
tracehook_notify_resume include/linux/tracehook.h:188 [inline]
exit_to_user_mode_loop kernel/entry/common.c:140 [inline]
exit_to_user_mode_prepare+0x195/0x1c0 kernel/entry/common.c:167
syscall_exit_to_user_mode+0x59/0x2b0 kernel/entry/common.c:242
entry_SYSCALL_64_after_hwframe+0x44/0xa9

Last call_rcu():
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
kasan_record_aux_stack+0x82/0xb0 mm/kasan/generic.c:346
__call_rcu kernel/rcu/tree.c:2894 [inline]
call_rcu+0x14f/0x7e0 kernel/rcu/tree.c:2968
dentry_free+0xc3/0x160 fs/dcache.c:350
__dentry_kill+0x4c6/0x640 fs/dcache.c:593
shrink_dentry_list+0x144/0x480 fs/dcache.c:1141
shrink_dcache_parent+0x219/0x3c0 fs/dcache.c:1568
d_invalidate fs/dcache.c:1677 [inline]
d_invalidate+0x13f/0x280 fs/dcache.c:1662
proc_invalidate_siblings_dcache+0x43b/0x600 fs/proc/inode.c:150
release_task+0xc63/0x14d0 kernel/exit.c:221
wait_task_zombie kernel/exit.c:1088 [inline]
wait_consider_task+0x2fb3/0x3b20 kernel/exit.c:1315
do_wait_thread kernel/exit.c:1378 [inline]
do_wait+0x36a/0x9e0 kernel/exit.c:1449
kernel_wait4+0x14c/0x260 kernel/exit.c:1621
__do_sys_wait4+0x13f/0x150 kernel/exit.c:1649
do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x44/0xa9

Second to last call_rcu():
kasan_save_stack+0x1b/0x40 mm/kasan/common.c:48
kasan_record_aux_stack+0x82/0xb0 mm/kasan/generic.c:346
__call_rcu kernel/rcu/tree.c:2894 [inline]
call_rcu+0x14f/0x7e0 kernel/rcu/tree.c:2968
dentry_free+0xc3/0x160 fs/dcache.c:350
__dentry_kill+0x4c6/0x640 fs/dcache.c:593
dentry_kill fs/dcache.c:717 [inline]
dput+0x635/0xbc0 fs/dcache.c:878
handle_mounts fs/namei.c:1389 [inline]
step_into+0xc43/0x1990 fs/namei.c:1690
walk_component+0x171/0x6a0 fs/namei.c:1866
link_path_walk.part.0+0x6b8/0xc20 fs/namei.c:2183
link_path_walk fs/namei.c:2111 [inline]
path_lookupat+0xb7/0x830 fs/namei.c:2332
filename_lookup+0x19f/0x560 fs/namei.c:2366
user_path_at include/linux/namei.h:59 [inline]
do_faccessat+0x129/0x820 fs/open.c:423
do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
entry_SYSCALL_64_after_hwframe+0x44/0xa9

The buggy address belongs to the object at ffff888087a02040
which belongs to the cache dentry of size 312
The buggy address is located 104 bytes inside of
312-byte region [ffff888087a02040, ffff888087a02178)
The buggy address belongs to the page:
page:00000000b4e25e7c refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x87a02
flags: 0xfffe0000000200(slab)
raw: 00fffe0000000200 ffffea000212a688 ffffea00021e6a48 ffff88821bc47f00
raw: 0000000000000000 ffff888087a02040 000000010000000a 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff888087a01f80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff888087a02000: fc fc fc fc fc fc fc fc fa fb fb fb fb fb fb fb
>ffff888087a02080: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff888087a02100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fc
ffff888087a02180: fc fc fc fc fc fc fc 00 00 00 00 00 00 00 00 00
==================================================================


---
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 syzk...@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

[Hillf Danton]

Mon, 31 Aug 2020 16:08:16 -0700

Any light on how a9ed4a6560b8 ends up with __fput called twice is
highly appreciated.

[Al Viro]

On Tue, Sep 01, 2020 at 10:35:47AM +0800, Hillf Danton wrote:

> Any light on how a9ed4a6560b8 ends up with __fput called twice is
> highly appreciated.

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index e0decff22ae2..8107e06d7f6f 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1995,9 +1995,9 @@ static int ep_loop_check_proc(void *priv, void *cookie, int call_nests)
* during ep_insert().
*/
if (list_empty(&epi->ffd.file->f_tfile_llink)) {
- get_file(epi->ffd.file);
- list_add(&epi->ffd.file->f_tfile_llink,
- &tfile_check_list);
+ if (get_file_rcu(epi->ffd.file))
+ list_add(&epi->ffd.file->f_tfile_llink,
+ &tfile_check_list);
}
}
}

[Hillf Danton]

On Tue, 1 Sep 2020 04:19:45 +0100 Al Viro wrote:
> On Tue, Sep 01, 2020 at 10:35:47AM +0800, Hillf Danton wrote:
>
> > Any light on how a9ed4a6560b8 ends up with __fput called twice is
> > highly appreciated.
>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index e0decff22ae2..8107e06d7f6f 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c

Thanks so much for your patch!

> @@ -1995,9 +1995,9 @@ static int ep_loop_check_proc(void *priv, void *cookie, int call_nests)
> * during ep_insert().
> */
> if (list_empty(&epi->ffd.file->f_tfile_llink)) {
> - get_file(epi->ffd.file);
> - list_add(&epi->ffd.file->f_tfile_llink,
> - &tfile_check_list);
> + if (get_file_rcu(epi->ffd.file))
> + list_add(&epi->ffd.file->f_tfile_llink,
> + &tfile_check_list);
> }
> }
> }

Though having nothing to do with RCU, get_file_rcu() can be the
answer to why __fput gets called twice, with the bonus, however,
that shows it's operating on a file with zero f_count.

If that's true, this is papering again over the issue a9ed4a6560b8
was once trying to fix, without making it safe to dereference the
file still in the rbtree.

[Hillf Danton]

On Tue, 1 Sep 2020 15:38:47 +0800 Hillf Danton wrote:
> On Tue, 1 Sep 2020 04:19:45 +0100 Al Viro wrote:

> > On Tue, Sep 01, 2020 at 10:35:47AM +0800, Hillf Danton wrote:
> >
> > > Any light on how a9ed4a6560b8 ends up with __fput called twice is
> > > highly appreciated.
> >
> > diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> > index e0decff22ae2..8107e06d7f6f 100644
> > --- a/fs/eventpoll.c
> > +++ b/fs/eventpoll.c
>

> Thanks so much for your patch!
>

> > @@ -1995,9 +1995,9 @@ static int ep_loop_check_proc(void *priv, void *cookie, int call_nests)
> > * during ep_insert().
> > */
> > if (list_empty(&epi->ffd.file->f_tfile_llink)) {
> > - get_file(epi->ffd.file);
> > - list_add(&epi->ffd.file->f_tfile_llink,
> > - &tfile_check_list);
> > + if (get_file_rcu(epi->ffd.file))
> > + list_add(&epi->ffd.file->f_tfile_llink,
> > + &tfile_check_list);
> > }
> > }
> > }
>

> Though having nothing to do with RCU, get_file_rcu() can be the
> answer to why __fput gets called twice, with the bonus, however,
> that shows it's operating on a file with zero f_count.
>
> If that's true, this is papering again over the issue a9ed4a6560b8
> was once trying to fix, without making it safe to dereference the
> file still in the rbtree.

Below is my 2c for making it safe to access the rbtree entry and the
file tucked in it by bumping up the file count before adding epi into
rbtree.

--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -790,6 +790,7 @@ static int ep_remove(struct eventpoll *e
write_unlock_irq(&ep->lock);

wakeup_source_unregister(ep_wakeup_source(epi));
+ fput(file);
/*
* At this point it is safe to free the eventpoll item. Use the union
* field epi->rcu, since we are trying to minimize the size of
@@ -1510,6 +1511,7 @@ static int ep_insert(struct eventpoll *e
INIT_LIST_HEAD(&epi->fllink);
INIT_LIST_HEAD(&epi->pwqlist);
epi->ep = ep;
+ get_file(tfile); /* pair with fput() in ep_remove() */
ep_set_ffd(&epi->ffd, tfile, fd);
epi->event = *event;
epi->nwait = 0;
@@ -1613,6 +1615,7 @@ error_unregister:

error_create_wakeup_source:
kmem_cache_free(epi_cache, epi);
+ fput(tfile);

return error;
}

[Al Viro]

On Tue, Sep 01, 2020 at 04:43:09PM +0800, Hillf Danton wrote:

> Below is my 2c for making it safe to access the rbtree entry and the
> file tucked in it by bumping up the file count before adding epi into
> rbtree.

NAK. epitems, by design, do *NOT* pin files down. That's what
eventpoll_release() is about - those references are not counting and
epitems can be taken out by the final close.

It's a user-visible aspect of API. And the problem Marc's patch was
trying to solve had nothing to do with that - at the root it's
the lack of suitable exclusion and the atrocious way loop prevention
and reverse path count checks are done on watch insertion.

What happens there is that files that would have paths added by
EPOLL_CTL_ADD (including those via several intermediate epoll files)
are collected into a temporary list and then checked for excessive
reverse paths. The list is emptied before epoll_ctl() returns.

And that's _the_ list - if epoll_ctl decides to go there in the
first place, it grabs a system-wide mutex and holds it as long as
it's playing around. Everything would've worked, if not for the
fact that
* the bloody list goes through struct file instances. It's
a bad decision, for a lot of reasons.
* in particular, files can go away while epoll_ctl() is playing
around. The same system-wide mutex would've blocked their freeing (modulo
a narrow race in eventpoll_release()) *IF* they remained attached to
epitems. However, explicit EPOLL_CTL_DEL removing such a beast
in the middle of EPOLL_CTL_ADD checks will remove such epitems without
touching the same mutex.

Marc's patch tried to brute-force the protection of files in that
temporary list; what it had missed was the fact that a file on it could
have already been committed to getting killed - f_count already zero,
just hadn't gotten through the __fput() yet. In such cases we don't
want to do any reverse path checks for that sucker - it *is* going
away, so there's no point considering it. We can't blindly bump the
refcount, though, for obvious reasons.

That struct file can't get freed right under the code inserting it into
the list - the locks held at the moment (ep->mtx) are more than enough.
It's what can happen to it while on the list that is a problem.

Of course, as a long-term solution we want to have that crap with
temporary list going through struct file instances taken out and
moved to epitems themselves; the check does scan all epitems for
every file in the set and we bloody well could gather those into
the list at once. Then we'd only need to protect the list walking
vs. removals (with a spinlock, for all we care).

It's too invasive a change for -stable, though, and I'm still digging
through fs/eventpoll.c locking - it's much too convoluted and it
got no attention for a decade ;-/

[Hillf Danton]

Look forward to hearing anything from you about fixing the problem.

/Hillf

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
general protection fault in __sock_release

general protection fault, probably for non-canonical address 0xdffffc0000000002: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017]
CPU: 1 PID: 9832 Comm: syz-executor.2 Not tainted 5.9.0-rc3-syzkaller #0

Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011

RIP: 0010:__sock_release+0xbb/0x280 net/socket.c:596
Code: ff df 48 c1 ea 03 80 3c 02 00 0f 85 a5 01 00 00 48 b8 00 00 00 00 00 fc ff df 4c 8b 63 20 49 8d 7c 24 10 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 8e 01 00 00 48 89 df 41 ff 54 24 10 48 8d 7b 18
RSP: 0018:ffffc90009217e28 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: ffff888083cbca80 RCX: 1ffff92001242f67
RDX: 0000000000000002 RSI: 0000000000000004 RDI: 0000000000000010
RBP: ffff888083cbcc20 R08: ffff888083cbcc20 R09: ffff888083cbcc33
R10: ffffed1010797986 R11: 0000000000000000 R12: 0000000000000000
R13: ffff888083cbcaa0 R14: 0000000000000000 R15: ffff8880893cd100
FS: 00007f7b04f5c700(0000) GS:ffff8880ae700000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000000 CR3: 0000000097972000 CR4: 00000000001506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
sock_close+0x18/0x20 net/socket.c:1277
__fput+0x285/0x920 fs/file_table.c:281

task_work_run+0xdd/0x190 kernel/task_work.c:141
tracehook_notify_resume include/linux/tracehook.h:188 [inline]
exit_to_user_mode_loop kernel/entry/common.c:140 [inline]

exit_to_user_mode_prepare+0x1e1/0x200 kernel/entry/common.c:167
syscall_exit_to_user_mode+0x7e/0x2e0 kernel/entry/common.c:242
entry_SYSCALL_64_after_hwframe+0x44/0xa9
RIP: 0033:0x45d5b9
Code: 5d b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 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 2b b4 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f7b04f5bc78 EFLAGS: 00000246 ORIG_RAX: 00000000000000e9
RAX: 0000000000000000 RBX: 0000000000002ac0 RCX: 000000000045d5b9
RDX: 0000000000000004 RSI: 0000000000000001 RDI: 0000000000000005
RBP: 000000000118cf88 R08: 0000000000000000 R09: 0000000000000000
R10: 00000000200003c0 R11: 0000000000000246 R12: 000000000118cf4c
R13: 00007ffd1460e51f R14: 00007f7b04f5c9c0 R15: 000000000118cf4c
Modules linked in:
---[ end trace 034597df4e489585 ]---
RIP: 0010:__sock_release+0xbb/0x280 net/socket.c:596
Code: ff df 48 c1 ea 03 80 3c 02 00 0f 85 a5 01 00 00 48 b8 00 00 00 00 00 fc ff df 4c 8b 63 20 49 8d 7c 24 10 48 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 8e 01 00 00 48 89 df 41 ff 54 24 10 48 8d 7b 18
RSP: 0018:ffffc90009217e28 EFLAGS: 00010202
RAX: dffffc0000000000 RBX: ffff888083cbca80 RCX: 1ffff92001242f67
RDX: 0000000000000002 RSI: 0000000000000004 RDI: 0000000000000010
RBP: ffff888083cbcc20 R08: ffff888083cbcc20 R09: ffff888083cbcc33
R10: ffffed1010797986 R11: 0000000000000000 R12: 0000000000000000
R13: ffff888083cbcaa0 R14: 0000000000000000 R15: ffff8880893cd100
FS: 00007f7b04f5c700(0000) GS:ffff8880ae600000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000744138 CR3: 0000000097972000 CR4: 00000000001506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


Tested on:

commit: 9c7d619b Merge tag 'perf-tools-fixes-for-v5.9-2020-09-01' ..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=113481d1900000
kernel config: https://syzkaller.appspot.com/x/.config?x=3c5f6ce8d5b68299

[syzbot]

Auto-closing this bug as obsolete.
No recent activity, existing reproducers are no longer triggering the issue.