[PATCH 1/3] file: fix close_range() for unshare+cloexec

[PATCH 1/3] file: fix close_range() for unshare+cloexec

[Christian Brauner]

From: Christian Brauner <christian.brauner@ubuntu.com>

syzbot reported a bug when putting the last reference to a tasks file
descriptor table. Debugging this showed we didn't recalculate the
current maximum fd number for CLOSE_RANGE_UNSHARE | CLOSE_RANGE_CLOEXEC
after we unshared the file descriptors table. So max_fd could exceed the
current fdtable maximum causing us to set excessive bits. As a concrete
example, let's say the user requested everything from fd 4 to ~0UL to be
closed and their current fdtable size is 256 with their highest open fd
being 4. With CLOSE_RANGE_UNSHARE the caller will end up with a new
fdtable which has room for 64 file descriptors since that is the lowest
fdtable size we accept. But now max_fd will still point to 255 and needs
to be adjusted. Fix this by retrieving the correct maximum fd value in
__range_cloexec().

Reported-by: syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com
Fixes: 582f1fb6b721 ("fs, close_range: add flag CLOSE_RANGE_CLOEXEC")
Fixes: fec8a6a69103 ("close_range: unshare all fds for CLOSE_RANGE_UNSHARE | CLOSE_RANGE_CLOEXEC")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Giuseppe Scrivano <gscrivan@redhat.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Cc: stable@vger.kernel.org
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
 fs/file.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/fs/file.c b/fs/file.c
index f3a4bac2cbe9..f633348029a5 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -629,17 +629,30 @@ int close_fd(unsigned fd)
 }
 EXPORT_SYMBOL(close_fd); /* for ksys_close() */
 
+/**
+ * last_fd - return last valid index into fd table
+ * @cur_fds: files struct
+ *
+ * Context: Either rcu read lock or files_lock must be held.
+ *
+ * Returns: Last valid index into fdtable.
+ */
+static inline unsigned last_fd(struct fdtable *fdt)
+{
+	return fdt->max_fds - 1;
+}
+
 static inline void __range_cloexec(struct files_struct *cur_fds,
 				   unsigned int fd, unsigned int max_fd)
 {
 	struct fdtable *fdt;
 
-	if (fd > max_fd)
-		return;
-
+	/* make sure we're using the correct maximum value */
 	spin_lock(&cur_fds->file_lock);
 	fdt = files_fdtable(cur_fds);
-	bitmap_set(fdt->close_on_exec, fd, max_fd - fd + 1);
+	max_fd = min(last_fd(fdt), max_fd);
+	if (fd <= max_fd)
+		bitmap_set(fdt->close_on_exec, fd, max_fd - fd + 1);
 	spin_unlock(&cur_fds->file_lock);
 }
 
-- 
2.27.0

Re: [syzbot] KASAN: null-ptr-deref Read in filp_close (2)

[syzbot]

syzbot has found a reproducer for the following issue on:

HEAD commit:    7fef2edf sd: don't mess with SD_MINORS for CONFIG_DEBUG_BL..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=178919b0300000
kernel config:  https://syzkaller.appspot.com/x/.config?x=20276914ec6ad813
dashboard link: https://syzkaller.appspot.com/bug?extid=283ce5a46486d6acdbaf
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=120220f2300000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=115f37b4300000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com

==================================================================
BUG: KASAN: use-after-free in instrument_atomic_read include/linux/instrumented.h:71 [inline]
BUG: KASAN: use-after-free in atomic64_read include/asm-generic/atomic-instrumented.h:605 [inline]
BUG: KASAN: use-after-free in atomic_long_read include/asm-generic/atomic-long.h:29 [inline]
BUG: KASAN: use-after-free in filp_close+0x22/0x170 fs/open.c:1306
Read of size 8 at addr ffff888025a40a78 by task syz-executor493/8445

CPU: 1 PID: 8445 Comm: syz-executor493 Not tainted 5.14.0-rc1-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:105
 print_address_description.constprop.0.cold+0x6c/0x309 mm/kasan/report.c:233
 __kasan_report mm/kasan/report.c:419 [inline]
 kasan_report.cold+0x83/0xdf mm/kasan/report.c:436
 check_region_inline mm/kasan/generic.c:183 [inline]
 kasan_check_range+0x13d/0x180 mm/kasan/generic.c:189
 instrument_atomic_read include/linux/instrumented.h:71 [inline]
 atomic64_read include/asm-generic/atomic-instrumented.h:605 [inline]
 atomic_long_read include/asm-generic/atomic-long.h:29 [inline]
 filp_close+0x22/0x170 fs/open.c:1306
 close_fd+0x5c/0x80 fs/file.c:628
 __do_sys_close fs/open.c:1331 [inline]
 __se_sys_close fs/open.c:1329 [inline]
 __x64_sys_close+0x2f/0xa0 fs/open.c:1329
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae
RIP: 0033:0x4021b3
Code: c7 c2 c0 ff ff ff f7 d8 64 89 02 48 c7 c0 ff ff ff ff eb ba 0f 1f 00 64 8b 04 25 18 00 00 00 85 c0 75 14 b8 03 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 45 c3 0f 1f 40 00 48 83 ec 18 89 7c 24 0c e8
RSP: 002b:00007ffe62cc73e8 EFLAGS: 00000246 ORIG_RAX: 0000000000000003
RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 00000000004021b3
RDX: 0000000020000000 RSI: 0000000000000005 RDI: 0000000000000004
RBP: 00007ffe62cc73f8 R08: 0000000000000004 R09: 00000000004aa000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007ffe62cc7400
R13: 0000000000000000 R14: 00000000004ad018 R15: 0000000000400488

Allocated by task 8445:
 kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
 kasan_set_track mm/kasan/common.c:46 [inline]
 set_alloc_info mm/kasan/common.c:434 [inline]
 __kasan_slab_alloc+0x84/0xa0 mm/kasan/common.c:467
 kasan_slab_alloc include/linux/kasan.h:253 [inline]
 slab_post_alloc_hook mm/slab.h:512 [inline]
 slab_alloc_node mm/slub.c:2981 [inline]
 slab_alloc mm/slub.c:2989 [inline]
 kmem_cache_alloc+0x216/0x3a0 mm/slub.c:2994
 kmem_cache_zalloc include/linux/slab.h:711 [inline]
 __alloc_file+0x21/0x280 fs/file_table.c:101
 alloc_empty_file+0x6d/0x170 fs/file_table.c:150
 path_openat+0xde/0x27f0 fs/namei.c:3493
 do_filp_open+0x1aa/0x400 fs/namei.c:3534
 do_sys_openat2+0x16d/0x420 fs/open.c:1204
 do_sys_open fs/open.c:1220 [inline]
 __do_sys_creat fs/open.c:1294 [inline]
 __se_sys_creat fs/open.c:1288 [inline]
 __x64_sys_creat+0xc9/0x120 fs/open.c:1288
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Freed by task 8445:
 kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
 kasan_set_track+0x1c/0x30 mm/kasan/common.c:46
 kasan_set_free_info+0x20/0x30 mm/kasan/generic.c:360
 ____kasan_slab_free mm/kasan/common.c:366 [inline]
 ____kasan_slab_free mm/kasan/common.c:328 [inline]
 __kasan_slab_free+0xfb/0x130 mm/kasan/common.c:374
 kasan_slab_free include/linux/kasan.h:229 [inline]
 slab_free_hook mm/slub.c:1650 [inline]
 slab_free_freelist_hook+0xdf/0x240 mm/slub.c:1675
 slab_free mm/slub.c:3235 [inline]
 kfree+0xeb/0x650 mm/slub.c:4295
 put_fs_context+0x3fb/0x650 fs/fs_context.c:454
 fscontext_release+0x4c/0x60 fs/fsopen.c:73
 __fput+0x288/0x920 fs/file_table.c:280
 task_work_run+0xdd/0x1a0 kernel/task_work.c:164
 tracehook_notify_resume include/linux/tracehook.h:189 [inline]
 exit_to_user_mode_loop kernel/entry/common.c:175 [inline]
 exit_to_user_mode_prepare+0x27e/0x290 kernel/entry/common.c:209
 __syscall_exit_to_user_mode_work kernel/entry/common.c:291 [inline]
 syscall_exit_to_user_mode+0x19/0x60 kernel/entry/common.c:302
 do_syscall_64+0x42/0xb0 arch/x86/entry/common.c:86
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Last potentially related work creation:
 kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
 kasan_record_aux_stack+0xe5/0x110 mm/kasan/generic.c:348
 __call_rcu kernel/rcu/tree.c:3029 [inline]
 call_rcu+0xb1/0x750 kernel/rcu/tree.c:3109
 task_work_run+0xdd/0x1a0 kernel/task_work.c:164
 tracehook_notify_resume include/linux/tracehook.h:189 [inline]
 exit_to_user_mode_loop kernel/entry/common.c:175 [inline]
 exit_to_user_mode_prepare+0x27e/0x290 kernel/entry/common.c:209
 __syscall_exit_to_user_mode_work kernel/entry/common.c:291 [inline]
 syscall_exit_to_user_mode+0x19/0x60 kernel/entry/common.c:302
 do_syscall_64+0x42/0xb0 arch/x86/entry/common.c:86
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Second to last potentially related work creation:
 kasan_save_stack+0x1b/0x40 mm/kasan/common.c:38
 kasan_record_aux_stack+0xe5/0x110 mm/kasan/generic.c:348
 task_work_add+0x3a/0x190 kernel/task_work.c:38
 fput_many.part.0+0xbb/0x170 fs/file_table.c:341
 fput_many fs/file_table.c:336 [inline]
 fput+0x3b/0x50 fs/file_table.c:357
 path_openat+0x19bd/0x27f0 fs/namei.c:3516
 do_filp_open+0x1aa/0x400 fs/namei.c:3534
 do_sys_openat2+0x16d/0x420 fs/open.c:1204
 do_sys_open fs/open.c:1220 [inline]
 __do_sys_open fs/open.c:1228 [inline]
 __se_sys_open fs/open.c:1224 [inline]
 __x64_sys_open+0x119/0x1c0 fs/open.c:1224
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

The buggy address belongs to the object at ffff888025a40a00
 which belongs to the cache filp of size 464
The buggy address is located 120 bytes inside of
 464-byte region [ffff888025a40a00, ffff888025a40bd0)
The buggy address belongs to the page:
page:ffffea0000969000 refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x25a40
head:ffffea0000969000 order:1 compound_mapcount:0
flags: 0xfff00000010200(slab|head|node=0|zone=1|lastcpupid=0x7ff)
raw: 00fff00000010200 0000000000000000 0000000b00000001 ffff8880109c4780
raw: 0000000000000000 00000000800c000c 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
page_owner tracks the page as allocated
page last allocated via order 1, migratetype Unmovable, gfp_mask 0xd20c0(__GFP_IO|__GFP_FS|__GFP_NOWARN|__GFP_NORETRY|__GFP_COMP|__GFP_NOMEMALLOC), pid 4875, ts 15466439710, free_ts 15379402342
 prep_new_page mm/page_alloc.c:2433 [inline]
 get_page_from_freelist+0xa72/0x2f80 mm/page_alloc.c:4166
 __alloc_pages+0x1b2/0x500 mm/page_alloc.c:5374
 alloc_pages+0x18c/0x2a0 mm/mempolicy.c:2244
 alloc_slab_page mm/slub.c:1713 [inline]
 allocate_slab+0x32b/0x4c0 mm/slub.c:1853
 new_slab mm/slub.c:1916 [inline]
 new_slab_objects mm/slub.c:2662 [inline]
 ___slab_alloc+0x4ba/0x820 mm/slub.c:2825
 __slab_alloc.constprop.0+0xa7/0xf0 mm/slub.c:2865
 slab_alloc_node mm/slub.c:2947 [inline]
 slab_alloc mm/slub.c:2989 [inline]
 kmem_cache_alloc+0x372/0x3a0 mm/slub.c:2994
 kmem_cache_zalloc include/linux/slab.h:711 [inline]
 __alloc_file+0x21/0x280 fs/file_table.c:101
 alloc_empty_file+0x6d/0x170 fs/file_table.c:150
 path_openat+0xde/0x27f0 fs/namei.c:3493
 do_filp_open+0x1aa/0x400 fs/namei.c:3534
 do_sys_openat2+0x16d/0x420 fs/open.c:1204
 do_sys_open fs/open.c:1220 [inline]
 __do_sys_open fs/open.c:1228 [inline]
 __se_sys_open fs/open.c:1224 [inline]
 __x64_sys_open+0x119/0x1c0 fs/open.c:1224
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae
page last free stack trace:
 reset_page_owner include/linux/page_owner.h:24 [inline]
 free_pages_prepare mm/page_alloc.c:1343 [inline]
 free_pcp_prepare+0x2c5/0x780 mm/page_alloc.c:1394
 free_unref_page_prepare mm/page_alloc.c:3329 [inline]
 free_unref_page+0x19/0x690 mm/page_alloc.c:3408
 qlink_free mm/kasan/quarantine.c:146 [inline]
 qlist_free_all+0x5a/0xc0 mm/kasan/quarantine.c:165
 kasan_quarantine_reduce+0x180/0x200 mm/kasan/quarantine.c:272
 __kasan_slab_alloc+0x8e/0xa0 mm/kasan/common.c:444
 kasan_slab_alloc include/linux/kasan.h:253 [inline]
 slab_post_alloc_hook mm/slab.h:512 [inline]
 slab_alloc_node mm/slub.c:2981 [inline]
 slab_alloc mm/slub.c:2989 [inline]
 __kmalloc+0x1f4/0x330 mm/slub.c:4133
 kmalloc include/linux/slab.h:596 [inline]
 tomoyo_add_entry security/tomoyo/common.c:2031 [inline]
 tomoyo_supervisor+0xce8/0xf00 security/tomoyo/common.c:2103
 tomoyo_audit_path_log security/tomoyo/file.c:168 [inline]
 tomoyo_path_permission security/tomoyo/file.c:587 [inline]
 tomoyo_path_permission+0x270/0x3a0 security/tomoyo/file.c:573
 tomoyo_path_perm+0x2f0/0x400 security/tomoyo/file.c:838
 security_inode_getattr+0xcf/0x140 security/security.c:1332
 vfs_getattr fs/stat.c:139 [inline]
 vfs_statx+0x164/0x390 fs/stat.c:207
 vfs_fstatat fs/stat.c:225 [inline]
 vfs_lstat include/linux/fs.h:3386 [inline]
 __do_sys_newlstat+0x91/0x110 fs/stat.c:380
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x44/0xae

Memory state around the buggy address:
 ffff888025a40900: 00 00 00 00 00 00 00 00 00 00 fc fc fc fc fc fc
 ffff888025a40980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
>ffff888025a40a00: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
                                                                ^
 ffff888025a40a80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
 ffff888025a40b00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================

Re: [syzbot] KASAN: null-ptr-deref Read in filp_close (2)

[Linus Torvalds]

On Mon, Jul 12, 2021 at 9:12 PM syzbot
<syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com> wrote:
>
> syzbot has found a reproducer for the following issue on:

Hmm.

This issue is reported to have been already fixed:

    Fix commit: 9b5b8722 file: fix close_range() for unshare+cloexec

and that fix is already in the reported HEAD commit:

> HEAD commit:    7fef2edf sd: don't mess with SD_MINORS for CONFIG_DEBUG_BL..

and the oops report clearly is from that:

> CPU: 1 PID: 8445 Comm: syz-executor493 Not tainted 5.14.0-rc1-syzkaller #0

so the alleged fix is already there.

So clearly commit 9b5b872215fe ("file: fix close_range() for
unshare+cloexec") does *NOT* fix the issue.

This was originally bisected to that 582f1fb6b721 ("fs, close_range:
add flag CLOSE_RANGE_CLOEXEC") in

     https://syzkaller.appspot.com/bug?id=1bef50bdd9622a1969608d1090b2b4a588d0c6ac

which is where the "fix" is from.

It would probably be good if sysbot made this kind of "hey, it was
reported fixed, but it's not" very clear.

The KASAN report looks like a use-after-free, and that "use" is
actually the sanity check that the file count is non-zero, so it's
really a "struct file *" that has already been free'd.

That bogus free is a regular close() system call

>  filp_close+0x22/0x170 fs/open.c:1306
>  close_fd+0x5c/0x80 fs/file.c:628
>  __do_sys_close fs/open.c:1331 [inline]
>  __se_sys_close fs/open.c:1329 [inline]

And it was opened by a "creat()" system call:

> Allocated by task 8445:
>  __alloc_file+0x21/0x280 fs/file_table.c:101
>  alloc_empty_file+0x6d/0x170 fs/file_table.c:150
>  path_openat+0xde/0x27f0 fs/namei.c:3493
>  do_filp_open+0x1aa/0x400 fs/namei.c:3534
>  do_sys_openat2+0x16d/0x420 fs/open.c:1204
>  do_sys_open fs/open.c:1220 [inline]
>  __do_sys_creat fs/open.c:1294 [inline]
>  __se_sys_creat fs/open.c:1288 [inline]
>  __x64_sys_creat+0xc9/0x120 fs/open.c:1288
>  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
>  do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
>  entry_SYSCALL_64_after_hwframe+0x44/0xae

But it has apparently already been closed from a workqueue:

> Freed by task 8445:
>  __fput+0x288/0x920 fs/file_table.c:280
>  task_work_run+0xdd/0x1a0 kernel/task_work.c:164

So it's some kind of confusion and re-use of a struct file pointer.

Which is certainly consistent with the "fix" in 9b5b872215fe ("file:
fix close_range() for unshare+cloexec"), but it very much looks like
that fix was incomplete and not the full story.

Some fdtable got re-allocated? The fix that wasn't a fix ends up
re-checking the maximum file number under the file_lock, but there's
clearly something else going on too.

Christian?

                Linus

Re: [syzbot] KASAN: null-ptr-deref Read in filp_close (2)

[Christian Brauner]

On Tue, Jul 13, 2021 at 11:49:14AM -0700, Linus Torvalds wrote:
> On Mon, Jul 12, 2021 at 9:12 PM syzbot
> <syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com> wrote:
> >
> > syzbot has found a reproducer for the following issue on:
> 
> Hmm.
> 
> This issue is reported to have been already fixed:
> 
>     Fix commit: 9b5b8722 file: fix close_range() for unshare+cloexec
> 
> and that fix is already in the reported HEAD commit:
> 
> > HEAD commit:    7fef2edf sd: don't mess with SD_MINORS for CONFIG_DEBUG_BL..
> 
> and the oops report clearly is from that:
> 
> > CPU: 1 PID: 8445 Comm: syz-executor493 Not tainted 5.14.0-rc1-syzkaller #0
> 
> so the alleged fix is already there.
> 
> So clearly commit 9b5b872215fe ("file: fix close_range() for
> unshare+cloexec") does *NOT* fix the issue.
> 
> This was originally bisected to that 582f1fb6b721 ("fs, close_range:
> add flag CLOSE_RANGE_CLOEXEC") in
> 
>      https://syzkaller.appspot.com/bug?id=1bef50bdd9622a1969608d1090b2b4a588d0c6ac
> 
> which is where the "fix" is from.
> 
> It would probably be good if sysbot made this kind of "hey, it was
> reported fixed, but it's not" very clear.
> 
> The KASAN report looks like a use-after-free, and that "use" is
> actually the sanity check that the file count is non-zero, so it's
> really a "struct file *" that has already been free'd.
> 
> That bogus free is a regular close() system call
> 
> >  filp_close+0x22/0x170 fs/open.c:1306
> >  close_fd+0x5c/0x80 fs/file.c:628
> >  __do_sys_close fs/open.c:1331 [inline]
> >  __se_sys_close fs/open.c:1329 [inline]
> 
> And it was opened by a "creat()" system call:
> 
> > Allocated by task 8445:
> >  __alloc_file+0x21/0x280 fs/file_table.c:101
> >  alloc_empty_file+0x6d/0x170 fs/file_table.c:150
> >  path_openat+0xde/0x27f0 fs/namei.c:3493
> >  do_filp_open+0x1aa/0x400 fs/namei.c:3534
> >  do_sys_openat2+0x16d/0x420 fs/open.c:1204
> >  do_sys_open fs/open.c:1220 [inline]
> >  __do_sys_creat fs/open.c:1294 [inline]
> >  __se_sys_creat fs/open.c:1288 [inline]
> >  __x64_sys_creat+0xc9/0x120 fs/open.c:1288
> >  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> >  do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> >  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> But it has apparently already been closed from a workqueue:
> 
> > Freed by task 8445:
> >  __fput+0x288/0x920 fs/file_table.c:280
> >  task_work_run+0xdd/0x1a0 kernel/task_work.c:164
> 
> So it's some kind of confusion and re-use of a struct file pointer.
> 
> Which is certainly consistent with the "fix" in 9b5b872215fe ("file:
> fix close_range() for unshare+cloexec"), but it very much looks like
> that fix was incomplete and not the full story.
> 
> Some fdtable got re-allocated? The fix that wasn't a fix ends up
> re-checking the maximum file number under the file_lock, but there's
> clearly something else going on too.
> 
> Christian?

Looking into this now.

I have to say I'm very confused by the syzkaller report here.

If I go to

https://syzkaller.appspot.com/bug?extid=283ce5a46486d6acdbaf

which is the original link in the report it shows me

android-54	KASAN: use-after-free Read in filp_close	C			2	183d	183d	0/1	upstream: reported C repro on 2021/01/11 12:38

which seems to indicate that this happened on an Android specific 5.4
kernel?

But ok, so I click on the link "upstream: reported C repro on 2021/01/11 12:38"
which takes me to a google group

https://groups.google.com/g/syzkaller-android-bugs/c/FQj0qcRSy_M/m/wrY70QFzBAAJ

which again strongly indicates that this is an Android specific kernel?

HEAD commit: c9951e5d Merge 5.4.88 into android12-5.4
git tree: android12-5.4

but then I can click on the dashboard link for that crash report and it
takes me to:

https://syzkaller.appspot.com/bug?extid=53897bcb31b82c7a08fe

which seems to be the upstream report?

So I'm a bit confused whether I'm even looking at the correct bug report
but I'll just give the repro a try and see what's going on.

Christian

Re: [syzbot] KASAN: null-ptr-deref Read in filp_close (2)

[Christian Brauner]

On Wed, Jul 14, 2021 at 09:59:25AM +0200, Christian Brauner wrote:
> On Tue, Jul 13, 2021 at 11:49:14AM -0700, Linus Torvalds wrote:
> > On Mon, Jul 12, 2021 at 9:12 PM syzbot
> > <syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com> wrote:
> > >
> > > syzbot has found a reproducer for the following issue on:
> > 
> > Hmm.
> > 
> > This issue is reported to have been already fixed:
> > 
> >     Fix commit: 9b5b8722 file: fix close_range() for unshare+cloexec
> > 
> > and that fix is already in the reported HEAD commit:
> > 
> > > HEAD commit:    7fef2edf sd: don't mess with SD_MINORS for CONFIG_DEBUG_BL..
> > 
> > and the oops report clearly is from that:
> > 
> > > CPU: 1 PID: 8445 Comm: syz-executor493 Not tainted 5.14.0-rc1-syzkaller #0
> > 
> > so the alleged fix is already there.
> > 
> > So clearly commit 9b5b872215fe ("file: fix close_range() for
> > unshare+cloexec") does *NOT* fix the issue.
> > 
> > This was originally bisected to that 582f1fb6b721 ("fs, close_range:
> > add flag CLOSE_RANGE_CLOEXEC") in
> > 
> >      https://syzkaller.appspot.com/bug?id=1bef50bdd9622a1969608d1090b2b4a588d0c6ac
> > 
> > which is where the "fix" is from.
> > 
> > It would probably be good if sysbot made this kind of "hey, it was
> > reported fixed, but it's not" very clear.
> > 
> > The KASAN report looks like a use-after-free, and that "use" is
> > actually the sanity check that the file count is non-zero, so it's
> > really a "struct file *" that has already been free'd.
> > 
> > That bogus free is a regular close() system call
> > 
> > >  filp_close+0x22/0x170 fs/open.c:1306
> > >  close_fd+0x5c/0x80 fs/file.c:628
> > >  __do_sys_close fs/open.c:1331 [inline]
> > >  __se_sys_close fs/open.c:1329 [inline]
> > 
> > And it was opened by a "creat()" system call:
> > 
> > > Allocated by task 8445:
> > >  __alloc_file+0x21/0x280 fs/file_table.c:101
> > >  alloc_empty_file+0x6d/0x170 fs/file_table.c:150
> > >  path_openat+0xde/0x27f0 fs/namei.c:3493
> > >  do_filp_open+0x1aa/0x400 fs/namei.c:3534
> > >  do_sys_openat2+0x16d/0x420 fs/open.c:1204
> > >  do_sys_open fs/open.c:1220 [inline]
> > >  __do_sys_creat fs/open.c:1294 [inline]
> > >  __se_sys_creat fs/open.c:1288 [inline]
> > >  __x64_sys_creat+0xc9/0x120 fs/open.c:1288
> > >  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > >  do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> > >  entry_SYSCALL_64_after_hwframe+0x44/0xae
> > 
> > But it has apparently already been closed from a workqueue:
> > 
> > > Freed by task 8445:
> > >  __fput+0x288/0x920 fs/file_table.c:280
> > >  task_work_run+0xdd/0x1a0 kernel/task_work.c:164
> > 
> > So it's some kind of confusion and re-use of a struct file pointer.
> > 
> > Which is certainly consistent with the "fix" in 9b5b872215fe ("file:
> > fix close_range() for unshare+cloexec"), but it very much looks like
> > that fix was incomplete and not the full story.
> > 
> > Some fdtable got re-allocated? The fix that wasn't a fix ends up
> > re-checking the maximum file number under the file_lock, but there's
> > clearly something else going on too.
> > 
> > Christian?
> 
> Looking into this now.
> 
> I have to say I'm very confused by the syzkaller report here.
> 
> If I go to
> 
> https://syzkaller.appspot.com/bug?extid=283ce5a46486d6acdbaf
> 
> which is the original link in the report it shows me
> 
> android-54	KASAN: use-after-free Read in filp_close	C			2	183d	183d	0/1	upstream: reported C repro on 2021/01/11 12:38
> 
> which seems to indicate that this happened on an Android specific 5.4
> kernel?
> 
> But ok, so I click on the link "upstream: reported C repro on 2021/01/11 12:38"
> which takes me to a google group
> 
> https://groups.google.com/g/syzkaller-android-bugs/c/FQj0qcRSy_M/m/wrY70QFzBAAJ
> 
> which again strongly indicates that this is an Android specific kernel?
> 
> HEAD commit: c9951e5d Merge 5.4.88 into android12-5.4
> git tree: android12-5.4
> 
> but then I can click on the dashboard link for that crash report and it
> takes me to:
> 
> https://syzkaller.appspot.com/bug?extid=53897bcb31b82c7a08fe
> 
> which seems to be the upstream report?
> 
> So I'm a bit confused whether I'm even looking at the correct bug report
> but I'll just give the repro a try and see what's going on.

Ok, reproduced and I think I found the issue. It's not related to
close_fd((), I think it's caused by a UAF when FSCONFIG_SET_FD is with
the key "source" and a valid fd passed through "aux".

Briefly, fs_parameter is a union:

struct fs_parameter {
	const char		*key;		/* Parameter name */
	enum fs_value_type	type:8;		/* The type of value here */
	union {
		char		*string;
		void		*blob;
		struct filename	*name;
		struct file	*file;
	};
	size_t	size;
	int	dirfd;
};

and cgroup1_parse_param is copying out param->string when the param's
key is "source" without verifying that param->type is actually
fs_value_is_string.

I'll explain in detail in the commit once I've confirmed and tested that
my suspicion is correct.

Christian

Re: [syzbot] KASAN: null-ptr-deref Read in filp_close (2)

[Dmitry Vyukov]

On Wed, 14 Jul 2021 at 09:59, Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Tue, Jul 13, 2021 at 11:49:14AM -0700, Linus Torvalds wrote:
> > On Mon, Jul 12, 2021 at 9:12 PM syzbot
> > <syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com> wrote:
> > >
> > > syzbot has found a reproducer for the following issue on:
> >
> > Hmm.
> >
> > This issue is reported to have been already fixed:
> >
> >     Fix commit: 9b5b8722 file: fix close_range() for unshare+cloexec
> >
> > and that fix is already in the reported HEAD commit:
> >
> > > HEAD commit:    7fef2edf sd: don't mess with SD_MINORS for CONFIG_DEBUG_BL..
> >
> > and the oops report clearly is from that:
> >
> > > CPU: 1 PID: 8445 Comm: syz-executor493 Not tainted 5.14.0-rc1-syzkaller #0
> >
> > so the alleged fix is already there.
> >
> > So clearly commit 9b5b872215fe ("file: fix close_range() for
> > unshare+cloexec") does *NOT* fix the issue.
> >
> > This was originally bisected to that 582f1fb6b721 ("fs, close_range:
> > add flag CLOSE_RANGE_CLOEXEC") in
> >
> >      https://syzkaller.appspot.com/bug?id=1bef50bdd9622a1969608d1090b2b4a588d0c6ac
> >
> > which is where the "fix" is from.
> >
> > It would probably be good if sysbot made this kind of "hey, it was
> > reported fixed, but it's not" very clear.
> >
> > The KASAN report looks like a use-after-free, and that "use" is
> > actually the sanity check that the file count is non-zero, so it's
> > really a "struct file *" that has already been free'd.
> >
> > That bogus free is a regular close() system call
> >
> > >  filp_close+0x22/0x170 fs/open.c:1306
> > >  close_fd+0x5c/0x80 fs/file.c:628
> > >  __do_sys_close fs/open.c:1331 [inline]
> > >  __se_sys_close fs/open.c:1329 [inline]
> >
> > And it was opened by a "creat()" system call:
> >
> > > Allocated by task 8445:
> > >  __alloc_file+0x21/0x280 fs/file_table.c:101
> > >  alloc_empty_file+0x6d/0x170 fs/file_table.c:150
> > >  path_openat+0xde/0x27f0 fs/namei.c:3493
> > >  do_filp_open+0x1aa/0x400 fs/namei.c:3534
> > >  do_sys_openat2+0x16d/0x420 fs/open.c:1204
> > >  do_sys_open fs/open.c:1220 [inline]
> > >  __do_sys_creat fs/open.c:1294 [inline]
> > >  __se_sys_creat fs/open.c:1288 [inline]
> > >  __x64_sys_creat+0xc9/0x120 fs/open.c:1288
> > >  do_syscall_x64 arch/x86/entry/common.c:50 [inline]
> > >  do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
> > >  entry_SYSCALL_64_after_hwframe+0x44/0xae
> >
> > But it has apparently already been closed from a workqueue:
> >
> > > Freed by task 8445:
> > >  __fput+0x288/0x920 fs/file_table.c:280
> > >  task_work_run+0xdd/0x1a0 kernel/task_work.c:164
> >
> > So it's some kind of confusion and re-use of a struct file pointer.
> >
> > Which is certainly consistent with the "fix" in 9b5b872215fe ("file:
> > fix close_range() for unshare+cloexec"), but it very much looks like
> > that fix was incomplete and not the full story.
> >
> > Some fdtable got re-allocated? The fix that wasn't a fix ends up
> > re-checking the maximum file number under the file_lock, but there's
> > clearly something else going on too.
> >
> > Christian?
>
> Looking into this now.
>
> I have to say I'm very confused by the syzkaller report here.
>
> If I go to
>
> https://syzkaller.appspot.com/bug?extid=283ce5a46486d6acdbaf
>
> which is the original link in the report it shows me
>
> android-54      KASAN: use-after-free Read in filp_close        C                       2       183d    183d    0/1     upstream: reported C repro on 2021/01/11 12:38
>
> which seems to indicate that this happened on an Android specific 5.4
> kernel?

Hi Christian,

That's "similar bugs" section. In this section syzbot shows previous
similar bugs and similar bugs on other kernels (lts, android).

"KASAN: use-after-free Read in filp_close" also happened on Android
tree, if you click on the link, you can see crashes and reproducers on
the android tree:
https://syzkaller.appspot.com/bug?id=255edc9d4f00a881d3bf68b87d09a8b7843409e7

If you are interested only in upstream crashes/reproducers, then
ignore the "similar bugs" section.


> But ok, so I click on the link "upstream: reported C repro on 2021/01/11 12:38"
> which takes me to a google group
>
> https://groups.google.com/g/syzkaller-android-bugs/c/FQj0qcRSy_M/m/wrY70QFzBAAJ
>
> which again strongly indicates that this is an Android specific kernel?
>
> HEAD commit: c9951e5d Merge 5.4.88 into android12-5.4
> git tree: android12-5.4
>
> but then I can click on the dashboard link for that crash report and it
> takes me to:
>
> https://syzkaller.appspot.com/bug?extid=53897bcb31b82c7a08fe
>
> which seems to be the upstream report?
>
> So I'm a bit confused whether I'm even looking at the correct bug report
> but I'll just give the repro a try and see what's going on.
>
> Christian
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/20210714075925.jtlfrhhuj4bzff3m%40wittgenstein.

Re: [syzbot] KASAN: null-ptr-deref Read in filp_close (2)

[Christian Brauner]

On Mon, Jul 12, 2021 at 09:12:20PM -0700, syzbot wrote:
> syzbot has found a reproducer for the following issue on:
> 
> HEAD commit:    7fef2edf sd: don't mess with SD_MINORS for CONFIG_DEBUG_BL..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=178919b0300000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=20276914ec6ad813
> dashboard link: https://syzkaller.appspot.com/bug?extid=283ce5a46486d6acdbaf
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=120220f2300000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=115f37b4300000
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com
> 
> ==================================================================
> BUG: KASAN: use-after-free in instrument_atomic_read include/linux/instrumented.h:71 [inline]
> BUG: KASAN: use-after-free in atomic64_read include/asm-generic/atomic-instrumented.h:605 [inline]
> BUG: KASAN: use-after-free in atomic_long_read include/asm-generic/atomic-long.h:29 [inline]
> BUG: KASAN: use-after-free in filp_close+0x22/0x170 fs/open.c:1306
> Read of size 8 at addr ffff888025a40a78 by task syz-executor493/8445

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/ 595fac5cecba71935450ec431eb8dfa963cf45fe 

Re: [syzbot] KASAN: null-ptr-deref Read in filp_close (2)

[Christian Brauner]

On Mon, Jul 12, 2021 at 09:12:20PM -0700, syzbot wrote:
> syzbot has found a reproducer for the following issue on:
> 
> HEAD commit:    7fef2edf sd: don't mess with SD_MINORS for CONFIG_DEBUG_BL..
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=178919b0300000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=20276914ec6ad813
> dashboard link: https://syzkaller.appspot.com/bug?extid=283ce5a46486d6acdbaf
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=120220f2300000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=115f37b4300000
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/ syzbot+283ce5a46486d6acdbaf 

Re: [syzbot] KASAN: null-ptr-deref Read in filp_close (2)

[syzbot]

> On Mon, Jul 12, 2021 at 09:12:20PM -0700, syzbot wrote:
>> syzbot has found a reproducer for the following issue on:
>> 
>> HEAD commit:    7fef2edf sd: don't mess with SD_MINORS for CONFIG_DEBUG_BL..
>> git tree:       upstream
>> console output: https://syzkaller.appspot.com/x/log.txt?x=178919b0300000
>> kernel config:  https://syzkaller.appspot.com/x/.config?x=20276914ec6ad813
>> dashboard link: https://syzkaller.appspot.com/bug?extid=283ce5a46486d6acdbaf
>> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=120220f2300000
>> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=115f37b4300000
>> 
>> IMPORTANT: if you fix the issue, please add the following tag to the commit:
>> Reported-by: syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com
>
> #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/ syzbot+283ce5a46486d6acdbaf 

"syzbot+283ce5a46486d6acdbaf" does not look like a valid git branch or commit.

Re: [syzbot] KASAN: null-ptr-deref Read in filp_close (2)

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

failed to checkout kernel repo https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/ on commit 595fac5cecba71935450ec431eb8dfa963cf45fe: failed to run ["git" "checkout" "595fac5cecba71935450ec431eb8dfa963cf45fe"]: exit status 128
fatal: reference is not a tree: 595fac5cecba71935450ec431eb8dfa963cf45fe



Tested on:

commit:         [unknown 
git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/ 595fac5cecba71935450ec431eb8dfa963cf45fe
dashboard link: https://syzkaller.appspot.com/bug?extid=283ce5a46486d6acdbaf
compiler:       

Re: [syzbot] KASAN: null-ptr-deref Read in filp_close (2)

[Christian Brauner]

On Wed, Jul 14, 2021 at 03:51:57PM +0200, Christian Brauner wrote:
> On Mon, Jul 12, 2021 at 09:12:20PM -0700, syzbot wrote:
> > syzbot has found a reproducer for the following issue on:
> > 
> > HEAD commit:    7fef2edf sd: don't mess with SD_MINORS for CONFIG_DEBUG_BL..
> > git tree:       upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=178919b0300000
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=20276914ec6ad813
> > dashboard link: https://syzkaller.appspot.com/bug?extid=283ce5a46486d6acdbaf
> > syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=120220f2300000
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=115f37b4300000
> > 
> > IMPORTANT: if you fix the issue, please add the following tag to the commit:
> > Reported-by: syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com
> > 
> > ==================================================================
> > BUG: KASAN: use-after-free in instrument_atomic_read include/linux/instrumented.h:71 [inline]
> > BUG: KASAN: use-after-free in atomic64_read include/asm-generic/atomic-instrumented.h:605 [inline]
> > BUG: KASAN: use-after-free in atomic_long_read include/asm-generic/atomic-long.h:29 [inline]
> > BUG: KASAN: use-after-free in filp_close+0x22/0x170 fs/open.c:1306
> > Read of size 8 at addr ffff888025a40a78 by task syz-executor493/8445
> 
> #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/ 595fac5cecba71935450ec431eb8dfa963cf45fe 

Hm, git.kernel.org doesn't seem to have caught up. Let's try:

#syz test: https://gitlab.com/brauner/linux.git 595fac5cecba71935450ec431eb8dfa963cf45fe 

Re: [syzbot] KASAN: null-ptr-deref Read in filp_close (2)

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+283ce5a46486d6acdbaf@syzkaller.appspotmail.com

Tested on:

commit:         595fac5c cgroup: verify that source is a string
git tree:       https://gitlab.com/brauner/linux.git
kernel config:  https://syzkaller.appspot.com/x/.config?x=20276914ec6ad813
dashboard link: https://syzkaller.appspot.com/bug?extid=283ce5a46486d6acdbaf
compiler:       

Note: testing is done by a robot and is best-effort only.