Hello,
syzbot found the following crash on:
HEAD commit: 311ef88a Add linux-next specific files for 20191004
git tree: linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=16ce4899600000
kernel config: https://syzkaller.appspot.com/x/.config?x=db2e4361e48662f4
dashboard link: https://syzkaller.appspot.com/bug?extid=da4f525235510683d855
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17b672b9600000
The bug was bisected to:
commit 9d14545b05f9eed69fbd4af14b927a462324ea19
Author: Arnd Bergmann <[email protected]>
Date: Fri Aug 30 16:12:15 2019 +0000
Merge branch 'limits' of https://github.com/deepa-hub/vfs into y2038
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=16eeee2b600000
final crash: https://syzkaller.appspot.com/x/report.txt?x=15eeee2b600000
console output: https://syzkaller.appspot.com/x/log.txt?x=11eeee2b600000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: [email protected]
Fixes: 9d14545b05f9 ("Merge branch 'limits' of
https://github.com/deepa-hub/vfs into y2038")
==================================================================
BUG: KASAN: use-after-free in do_new_mount_fc fs/namespace.c:2773 [inline]
BUG: KASAN: use-after-free in do_new_mount fs/namespace.c:2825 [inline]
BUG: KASAN: use-after-free in do_mount+0x1b5f/0x1d10 fs/namespace.c:3143
Read of size 8 at addr ffff88809a505b28 by task syz-executor.4/13945
CPU: 1 PID: 13945 Comm: syz-executor.4 Not tainted 5.4.0-rc1-next-20191004
#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+0x172/0x1f0 lib/dump_stack.c:113
print_address_description.constprop.0.cold+0xd4/0x30b mm/kasan/report.c:374
__kasan_report.cold+0x1b/0x41 mm/kasan/report.c:506
kasan_report+0x12/0x20 mm/kasan/common.c:634
__asan_report_load8_noabort+0x14/0x20 mm/kasan/generic_report.c:132
do_new_mount_fc fs/namespace.c:2773 [inline]
do_new_mount fs/namespace.c:2825 [inline]
do_mount+0x1b5f/0x1d10 fs/namespace.c:3143
ksys_mount+0xdb/0x150 fs/namespace.c:3352
__do_sys_mount fs/namespace.c:3366 [inline]
__se_sys_mount fs/namespace.c:3363 [inline]
__x64_sys_mount+0xbe/0x150 fs/namespace.c:3363
do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x459a59
Code: fd b7 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 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f1c10834c78 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 0000000000459a59
RDX: 0000000020000a40 RSI: 00000000200005c0 RDI: 0000000000000000
RBP: 000000000075bf20 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f1c108356d4
R13: 00000000004c6291 R14: 00000000004db2f8 R15: 00000000ffffffff
Allocated by task 13945:
save_stack+0x23/0x90 mm/kasan/common.c:69
set_track mm/kasan/common.c:77 [inline]
__kasan_kmalloc mm/kasan/common.c:510 [inline]
__kasan_kmalloc.constprop.0+0xcf/0xe0 mm/kasan/common.c:483
kasan_slab_alloc+0xf/0x20 mm/kasan/common.c:518
slab_post_alloc_hook mm/slab.h:584 [inline]
slab_alloc mm/slab.c:3319 [inline]
kmem_cache_alloc+0x121/0x710 mm/slab.c:3483
kmem_cache_zalloc include/linux/slab.h:680 [inline]
alloc_vfsmnt+0x28/0x680 fs/namespace.c:177
vfs_create_mount+0x96/0x500 fs/namespace.c:940
do_new_mount_fc fs/namespace.c:2763 [inline]
do_new_mount fs/namespace.c:2825 [inline]
do_mount+0x17ae/0x1d10 fs/namespace.c:3143
ksys_mount+0xdb/0x150 fs/namespace.c:3352
__do_sys_mount fs/namespace.c:3366 [inline]
__se_sys_mount fs/namespace.c:3363 [inline]
__x64_sys_mount+0xbe/0x150 fs/namespace.c:3363
do_syscall_64+0xfa/0x760 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Freed by task 16:
save_stack+0x23/0x90 mm/kasan/common.c:69
set_track mm/kasan/common.c:77 [inline]
kasan_set_free_info mm/kasan/common.c:332 [inline]
__kasan_slab_free+0x102/0x150 mm/kasan/common.c:471
kasan_slab_free+0xe/0x10 mm/kasan/common.c:480
__cache_free mm/slab.c:3425 [inline]
kmem_cache_free+0x86/0x320 mm/slab.c:3693
free_vfsmnt+0x6f/0x90 fs/namespace.c:554
delayed_free_vfsmnt+0x16/0x20 fs/namespace.c:559
__rcu_reclaim kernel/rcu/rcu.h:222 [inline]
rcu_do_batch kernel/rcu/tree.c:2157 [inline]
rcu_core+0x581/0x1560 kernel/rcu/tree.c:2377
rcu_core_si+0x9/0x10 kernel/rcu/tree.c:2386
__do_softirq+0x262/0x98c kernel/softirq.c:292
The buggy address belongs to the object at ffff88809a505b00
which belongs to the cache mnt_cache of size 312
The buggy address is located 40 bytes inside of
312-byte region [ffff88809a505b00, ffff88809a505c38)
The buggy address belongs to the page:
page:ffffea0002694140 refcount:1 mapcount:0 mapping:ffff8880aa5a88c0
index:0x0
flags: 0x1fffc0000000200(slab)
raw: 01fffc0000000200 ffffea000274afc8 ffffea0002982688 ffff8880aa5a88c0
raw: 0000000000000000 ffff88809a505080 000000010000000a 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff88809a505a00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ffff88809a505a80: 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc fc
> ffff88809a505b00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff88809a505b80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff88809a505c00: fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc fc
==================================================================
---
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 [email protected].
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
From: Eric Biggers <[email protected]>
After do_add_mount() returns success, the caller doesn't hold a
reference to the 'struct mount' anymore. So it's invalid to access it
in mnt_warn_timestamp_expiry().
Fix it by instead passing the super_block and the mnt_flags. It's safe
to access the super_block because it's pinned by fs_context::root.
Reported-by: [email protected]
Fixes: f8b92ba67c5d ("mount: Add mount warning for impending timestamp expiry")
Signed-off-by: Eric Biggers <[email protected]>
---
fs/namespace.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/fs/namespace.c b/fs/namespace.c
index fe0e9e1410fe..7ef8edaaed69 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2466,12 +2466,11 @@ static void set_mount_attributes(struct mount *mnt, unsigned int mnt_flags)
unlock_mount_hash();
}
-static void mnt_warn_timestamp_expiry(struct path *mountpoint, struct vfsmount *mnt)
+static void mnt_warn_timestamp_expiry(struct path *mountpoint,
+ struct super_block *sb, int mnt_flags)
{
- struct super_block *sb = mnt->mnt_sb;
-
- if (!__mnt_is_readonly(mnt) &&
- (ktime_get_real_seconds() + TIME_UPTIME_SEC_MAX > sb->s_time_max)) {
+ if (!(mnt_flags & MNT_READONLY) && !sb_rdonly(sb) &&
+ (ktime_get_real_seconds() + TIME_UPTIME_SEC_MAX > sb->s_time_max)) {
char *buf = (char *)__get_free_page(GFP_KERNEL);
char *mntpath = buf ? d_path(mountpoint, buf, PAGE_SIZE) : ERR_PTR(-ENOMEM);
struct tm tm;
@@ -2512,7 +2511,7 @@ static int do_reconfigure_mnt(struct path *path, unsigned int mnt_flags)
set_mount_attributes(mnt, mnt_flags);
up_write(&sb->s_umount);
- mnt_warn_timestamp_expiry(path, &mnt->mnt);
+ mnt_warn_timestamp_expiry(path, sb, mnt_flags);
return ret;
}
@@ -2555,7 +2554,7 @@ static int do_remount(struct path *path, int ms_flags, int sb_flags,
up_write(&sb->s_umount);
}
- mnt_warn_timestamp_expiry(path, &mnt->mnt);
+ mnt_warn_timestamp_expiry(path, sb, mnt_flags);
put_fs_context(fc);
return err;
@@ -2770,7 +2769,7 @@ static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint,
return error;
}
- mnt_warn_timestamp_expiry(mountpoint, mnt);
+ mnt_warn_timestamp_expiry(mountpoint, sb, mnt_flags);
return error;
}
--
2.23.0
Thanks for the fix.
Would it be better to move the check and warning to a place where the
access is still safe?
-Deepa
> On Oct 9, 2019, at 12:19 AM, Eric Biggers <[email protected]> wrote:
>
> From: Eric Biggers <[email protected]>
On Wed, Oct 9, 2019 at 12:19 AM Eric Biggers <[email protected]> wrote:
>
> From: Eric Biggers <[email protected]>
>
> After do_add_mount() returns success, the caller doesn't hold a
> reference to the 'struct mount' anymore. So it's invalid to access it
> in mnt_warn_timestamp_expiry().
>
> Fix it by instead passing the super_block and the mnt_flags. It's safe
> to access the super_block because it's pinned by fs_context::root.
>
> Reported-by: [email protected]
> Fixes: f8b92ba67c5d ("mount: Add mount warning for impending timestamp expiry")
> Signed-off-by: Eric Biggers <[email protected]>
> ---
> fs/namespace.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index fe0e9e1410fe..7ef8edaaed69 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2466,12 +2466,11 @@ static void set_mount_attributes(struct mount *mnt, unsigned int mnt_flags)
> unlock_mount_hash();
> }
>
> -static void mnt_warn_timestamp_expiry(struct path *mountpoint, struct vfsmount *mnt)
> +static void mnt_warn_timestamp_expiry(struct path *mountpoint,
> + struct super_block *sb, int mnt_flags)
> {
> - struct super_block *sb = mnt->mnt_sb;
> -
> - if (!__mnt_is_readonly(mnt) &&
> - (ktime_get_real_seconds() + TIME_UPTIME_SEC_MAX > sb->s_time_max)) {
> + if (!(mnt_flags & MNT_READONLY) && !sb_rdonly(sb) &&
> + (ktime_get_real_seconds() + TIME_UPTIME_SEC_MAX > sb->s_time_max)) {
> char *buf = (char *)__get_free_page(GFP_KERNEL);
> char *mntpath = buf ? d_path(mountpoint, buf, PAGE_SIZE) : ERR_PTR(-ENOMEM);
> struct tm tm;
> @@ -2512,7 +2511,7 @@ static int do_reconfigure_mnt(struct path *path, unsigned int mnt_flags)
> set_mount_attributes(mnt, mnt_flags);
> up_write(&sb->s_umount);
>
> - mnt_warn_timestamp_expiry(path, &mnt->mnt);
> + mnt_warn_timestamp_expiry(path, sb, mnt_flags);
>
> return ret;
> }
> @@ -2555,7 +2554,7 @@ static int do_remount(struct path *path, int ms_flags, int sb_flags,
> up_write(&sb->s_umount);
> }
>
> - mnt_warn_timestamp_expiry(path, &mnt->mnt);
> + mnt_warn_timestamp_expiry(path, sb, mnt_flags);
>
> put_fs_context(fc);
> return err;
> @@ -2770,7 +2769,7 @@ static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint,
> return error;
> }
>
> - mnt_warn_timestamp_expiry(mountpoint, mnt);
> + mnt_warn_timestamp_expiry(mountpoint, sb, mnt_flags);
>
> return error;
> }
> --
> 2.23.0
>
On Sun, Oct 13, 2019 at 07:04:10PM -0700, Deepa Dinamani wrote:
> Thanks for the fix.
>
> Would it be better to move the check and warning to a place where the
> access is still safe?
>
> -Deepa
True, we could just do
diff --git a/fs/namespace.c b/fs/namespace.c
index fe0e9e1410fe..9f2ceb542f05 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2764,14 +2764,14 @@ static int do_new_mount_fc(struct fs_context *fc, struct path *mountpoint,
if (IS_ERR(mnt))
return PTR_ERR(mnt);
+ mnt_warn_timestamp_expiry(mountpoint, mnt);
+
error = do_add_mount(real_mount(mnt), mountpoint, mnt_flags);
if (error < 0) {
mntput(mnt);
return error;
}
- mnt_warn_timestamp_expiry(mountpoint, mnt);
-
return error;
}
But then the warning ("Mounted %s file system ...") is printed even if
do_add_mount() fails so nothing was actually mounted.
Though, it's just a warning message and I think failures here are rare, so maybe
we don't care. Al, what do you think?
- Eric