KASAN: use-after-free Write in fsnotify_detach_connector_from_object

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit: 7ae77150 Merge tag 'powerpc-5.8-1' of git://git.kernel.org..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=120b26c1100000
kernel config: https://syzkaller.appspot.com/x/.config?x=d195fe572fb15312
dashboard link: https://syzkaller.appspot.com/bug?extid=7d2debdcdb3cb93c1e5e
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1724b246100000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14ceb3de100000

The bug was bisected to:

commit 76313c70c52f930af4afd21684509ca52297ea71
Author: Eric W. Biederman <ebie...@xmission.com>
Date: Wed Feb 19 16:37:15 2020 +0000

uml: Create a private mount of proc for mconsole

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=117c4912100000
final crash: https://syzkaller.appspot.com/x/report.txt?x=137c4912100000
console output: https://syzkaller.appspot.com/x/log.txt?x=157c4912100000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+7d2deb...@syzkaller.appspotmail.com
Fixes: 76313c70c52f ("uml: Create a private mount of proc for mconsole")

==================================================================
BUG: KASAN: use-after-free in atomic64_inc include/asm-generic/atomic-instrumented.h:1049 [inline]
BUG: KASAN: use-after-free in atomic_long_inc include/asm-generic/atomic-long.h:160 [inline]
BUG: KASAN: use-after-free in fsnotify_detach_connector_from_object+0x25e/0x380 fs/notify/mark.c:185
Write of size 8 at addr ffff88809fd7e7c0 by task syz-executor972/8021

CPU: 1 PID: 8021 Comm: syz-executor972 Not tainted 5.7.0-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
print_address_description.constprop.0.cold+0xd3/0x413 mm/kasan/report.c:383
__kasan_report mm/kasan/report.c:513 [inline]
kasan_report.cold+0x1f/0x37 mm/kasan/report.c:530
check_memory_region_inline mm/kasan/generic.c:186 [inline]
check_memory_region+0x141/0x190 mm/kasan/generic.c:192
atomic64_inc include/asm-generic/atomic-instrumented.h:1049 [inline]
atomic_long_inc include/asm-generic/atomic-long.h:160 [inline]
fsnotify_detach_connector_from_object+0x25e/0x380 fs/notify/mark.c:185
fsnotify_put_mark+0x367/0x580 fs/notify/mark.c:250
fsnotify_clear_marks_by_group+0x33f/0x490 fs/notify/mark.c:764
fsnotify_destroy_group+0xc9/0x300 fs/notify/group.c:61
inotify_release+0x33/0x40 fs/notify/inotify/inotify_user.c:271
__fput+0x33e/0x880 fs/file_table.c:281
task_work_run+0xf4/0x1b0 kernel/task_work.c:123
exit_task_work include/linux/task_work.h:22 [inline]
do_exit+0xb3f/0x2de0 kernel/exit.c:806
do_group_exit+0x125/0x340 kernel/exit.c:904
__do_sys_exit_group kernel/exit.c:915 [inline]
__se_sys_exit_group kernel/exit.c:913 [inline]
__x64_sys_exit_group+0x3a/0x50 kernel/exit.c:913
do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295
entry_SYSCALL_64_after_hwframe+0x49/0xb3
RIP: 0033:0x445448
Code: Bad RIP value.
RSP: 002b:00007ffe48521018 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 0000000000445448
RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
RBP: 00000000004cca90 R08: 00000000000000e7 R09: ffffffffffffffd0
R10: 00007ffe48521060 R11: 0000000000000246 R12: 0000000000000001
R13: 00000000006e0340 R14: 0000000000000007 R15: 000000000000002d

Allocated by task 8026:
save_stack+0x1b/0x40 mm/kasan/common.c:48
set_track mm/kasan/common.c:56 [inline]
__kasan_kmalloc mm/kasan/common.c:494 [inline]
__kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:467
kmem_cache_alloc_trace+0x153/0x7d0 mm/slab.c:3551
kmalloc include/linux/slab.h:555 [inline]
kzalloc include/linux/slab.h:669 [inline]
alloc_super+0x52/0x9d0 fs/super.c:203
sget_fc+0x13f/0x790 fs/super.c:530
vfs_get_super+0x6d/0x2d0 fs/super.c:1186
vfs_get_tree+0x89/0x2f0 fs/super.c:1547
do_new_mount fs/namespace.c:2874 [inline]
do_mount+0x1306/0x1b40 fs/namespace.c:3199
__do_sys_mount fs/namespace.c:3409 [inline]
__se_sys_mount fs/namespace.c:3386 [inline]
__x64_sys_mount+0x18f/0x230 fs/namespace.c:3386
do_syscall_64+0xf6/0x7d0 arch/x86/entry/common.c:295
entry_SYSCALL_64_after_hwframe+0x49/0xb3

Freed by task 23:
save_stack+0x1b/0x40 mm/kasan/common.c:48
set_track mm/kasan/common.c:56 [inline]
kasan_set_free_info mm/kasan/common.c:316 [inline]
__kasan_slab_free+0xf7/0x140 mm/kasan/common.c:455
__cache_free mm/slab.c:3426 [inline]
kfree+0x109/0x2b0 mm/slab.c:3757
process_one_work+0x965/0x16a0 kernel/workqueue.c:2268
worker_thread+0x96/0xe20 kernel/workqueue.c:2414
kthread+0x388/0x470 kernel/kthread.c:268
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:351

The buggy address belongs to the object at ffff88809fd7e000
which belongs to the cache kmalloc-4k of size 4096
The buggy address is located 1984 bytes inside of
4096-byte region [ffff88809fd7e000, ffff88809fd7f000)
The buggy address belongs to the page:
page:ffffea00027f5f80 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 head:ffffea00027f5f80 order:1 compound_mapcount:0
flags: 0xfffe0000010200(slab|head)
raw: 00fffe0000010200 ffffea000247aa88 ffffea000242ef08 ffff8880aa002000
raw: 0000000000000000 ffff88809fd7e000 0000000100000001 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff88809fd7e680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff88809fd7e700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff88809fd7e780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff88809fd7e800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff88809fd7e880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================


---
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

[Eric W. Biederman]

syzbot <syzbot+7d2deb...@syzkaller.appspotmail.com> writes:

> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: 7ae77150 Merge tag 'powerpc-5.8-1' of git://git.kernel.org..
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=120b26c1100000
> kernel config: https://syzkaller.appspot.com/x/.config?x=d195fe572fb15312
> dashboard link: https://syzkaller.appspot.com/bug?extid=7d2debdcdb3cb93c1e5e
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1724b246100000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=14ceb3de100000
>
> The bug was bisected to:

That bisection can not be correct. The commit only added code, and the
code that was added is not in any of the call traces. Further
the failure on the final commit was different than the other commits
in your bisection.

I will believe commit 69879c01a0c3f70e0887cfb4d9ff439814361e46 ("proc:
Remove the now unnecessary internal mount of proc") is the point at
which things start failing for your reproducer. That is the change
that makes it possible to actually unmount proc, and for it's super
block to be freed.

Now I don't know why fsnotify is holding on after a filesystem has been
unmounted. At first glance this looks like a bug in inotify.

It looks like your reproducer is doing:

mkdir ./file
mount -t proc ./file
inotify_init()
inotify_add_watch(./file, ...);
umount(./file)
...
exit(0);
<kaboom>

Then after the exit inotify is falling over because the filesystem has
already been unmounted.

Can anyone who is more familiar with inotify/fsnotify give a clue why
the unmount of the filesystem is not clearing the watch?

Is it a generic bug or is there something proc is not doing?

Eric

[Eric W. Biederman]

Recently syzbot reported that unmounting proc when there is an ongoing
inotify watch on the root directory of proc could result in a use
after free when the watch is removed after the unmount of proc
when the watcher exits.

Commit 69879c01a0c3 ("proc: Remove the now unnecessary internal mount
of proc") made it easier to unmount proc and allowed syzbot to see the
problem, but looking at the code it has been around for a long time.

Looking at the code the fsnotify watch should have been removed by
fsnotify_sb_delete in generic_shutdown_super. Unfortunately the inode
was allocated with new_inode_pseudo instead of new_inode so the inode
was not on the sb->s_inodes list. Which prevented
fsnotify_unmount_inodes from finding the inode and removing the watch
as well as made it so the "VFS: Busy inodes after unmount" warning
could not find the inodes to warn about them.

Make all of the inodes in proc visible to generic_shutdown_super,
and fsnotify_sb_delete by using new_inode instead of new_inode_pseudo.
The only functional difference is that new_inode places the inodes
on the sb->s_inodes list.

I wrote a small test program and I can verify that without changes it
can trigger this issue, and by replacing new_inode_pseudo with
new_inode the issues goes away.

Cc: sta...@vger.kernel.org
Link: https://lkml.kernel.org/r/000000000000d7...@google.com
Reported-by: syzbot+7d2deb...@syzkaller.appspotmail.com
Fixes: 0097875bd415 ("proc: Implement /proc/thread-self to point at the directory of the current thread")
Fixes: 021ada7dff22 ("procfs: switch /proc/self away from proc_dir_entry")
Fixes: 51f0885e5415 ("vfs,proc: guarantee unique inodes in /proc")
Signed-off-by: "Eric W. Biederman" <ebie...@xmission.com>
---
fs/proc/inode.c | 2 +-
fs/proc/self.c | 2 +-
fs/proc/thread_self.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index f40c2532c057..28d6105e908e 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -617,7 +617,7 @@ const struct inode_operations proc_link_inode_operations = {

struct inode *proc_get_inode(struct super_block *sb, struct proc_dir_entry *de)
{
- struct inode *inode = new_inode_pseudo(sb);
+ struct inode *inode = new_inode(sb);

if (inode) {
inode->i_ino = de->low_ino;
diff --git a/fs/proc/self.c b/fs/proc/self.c
index ca5158fa561c..72cd69bcaf4a 100644
--- a/fs/proc/self.c
+++ b/fs/proc/self.c
@@ -43,7 +43,7 @@ int proc_setup_self(struct super_block *s)
inode_lock(root_inode);
self = d_alloc_name(s->s_root, "self");
if (self) {
- struct inode *inode = new_inode_pseudo(s);
+ struct inode *inode = new_inode(s);
if (inode) {
inode->i_ino = self_inum;
inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
diff --git a/fs/proc/thread_self.c b/fs/proc/thread_self.c
index ac284f409568..a553273fbd41 100644
--- a/fs/proc/thread_self.c
+++ b/fs/proc/thread_self.c
@@ -43,7 +43,7 @@ int proc_setup_thread_self(struct super_block *s)
inode_lock(root_inode);
thread_self = d_alloc_name(s->s_root, "thread-self");
if (thread_self) {
- struct inode *inode = new_inode_pseudo(s);
+ struct inode *inode = new_inode(s);
if (inode) {
inode->i_ino = thread_self_inum;
inode->i_mtime = inode->i_atime = inode->i_ctime = current_time(inode);
--
2.20.1

[Jan Kara]

Thanks for analysing this! I agree with the analysis and the patch looks
good to me. You can add:

Reviewed-by: Jan Kara <ja...@suse.cz>

Honza

--
Jan Kara <ja...@suse.com>
SUSE Labs, CR