[syzbot] [autofs?] general protection fault in autofs_fill_super

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 4bbdb725a36b Merge tag 'iommu-updates-v6.7' of git://git.k..
git tree: upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=15dc14a8e80000
kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=662f87a8ef490f45fa64
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14384a7b680000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/fd459eb1acfc/disk-4bbdb725.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/d784829734e1/vmlinux-4bbdb725.xz
kernel image: https://storage.googleapis.com/syzbot-assets/db2c9e9ae9c3/bzImage-4bbdb725.xz

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

Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 e1 20 00 00 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 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffe4723a6f8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007fae5d99bf80 RCX: 00007fae5d87cae9
RDX: 0000000020000040 RSI: 0000000020000380 RDI: 0000000000000000
RBP: 00007ffe4723a750 R08: 0000000020000400 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000002
R13: 00000000000009f9 R14: 00007fae5d99bf80 R15: 00007fae5d99bf80
</TASK>
general protection fault, probably for non-canonical address 0xdffffc0000000000: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
CPU: 0 PID: 5098 Comm: syz-executor.0 Not tainted 6.6.0-syzkaller-15601-g4bbdb725a36b #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023
RIP: 0010:autofs_fill_super+0x47d/0xb50 fs/autofs/inode.c:334
Code: 03 60 d7 9f 8b 4d 8d 67 04 48 8b 14 24 48 89 d0 48 c1 e8 03 42 0f b6 04 30 84 c0 0f 85 58 03 00 00 8b 1a 4c 89 e0 48 c1 e8 03 <42> 0f b6 04 30 84 c0 4c 89 f5 0f 85 61 03 00 00 41 89 5f 04 4d 8d
RSP: 0018:ffffc90003bafc90 EFLAGS: 00010247
RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff88807b190000
RDX: ffff8880136beac0 RSI: ffffffff8bbdc620 RDI: ffffffff8bbdc5e0
RBP: ffff8880206d0580 R08: ffffffff8da2aa13 R09: 1ffffffff1b45542
R10: dffffc0000000000 R11: fffffbfff1b45543 R12: 0000000000000004
R13: ffff8880206d0500 R14: dffffc0000000000 R15: 0000000000000000
FS: 0000555555cbc480(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffe47239e08 CR3: 0000000024f32000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
vfs_get_super fs/super.c:1338 [inline]
get_tree_nodev+0xb4/0x140 fs/super.c:1357
vfs_get_tree+0x8c/0x280 fs/super.c:1771
do_new_mount+0x28f/0xae0 fs/namespace.c:3337
do_mount fs/namespace.c:3677 [inline]
__do_sys_mount fs/namespace.c:3886 [inline]
__se_sys_mount+0x2d9/0x3c0 fs/namespace.c:3863
do_syscall_x64 arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x63/0x6b
RIP: 0033:0x7fae5d87cae9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 e1 20 00 00 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 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffe4723a6f8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 00007fae5d99bf80 RCX: 00007fae5d87cae9
RDX: 0000000020000040 RSI: 0000000020000380 RDI: 0000000000000000
RBP: 00007ffe4723a750 R08: 0000000020000400 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000002
R13: 00000000000009f9 R14: 00007fae5d99bf80 R15: 00007fae5d99bf80
</TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:autofs_fill_super+0x47d/0xb50 fs/autofs/inode.c:334
Code: 03 60 d7 9f 8b 4d 8d 67 04 48 8b 14 24 48 89 d0 48 c1 e8 03 42 0f b6 04 30 84 c0 0f 85 58 03 00 00 8b 1a 4c 89 e0 48 c1 e8 03 <42> 0f b6 04 30 84 c0 4c 89 f5 0f 85 61 03 00 00 41 89 5f 04 4d 8d
RSP: 0018:ffffc90003bafc90 EFLAGS: 00010247
RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffff88807b190000
RDX: ffff8880136beac0 RSI: ffffffff8bbdc620 RDI: ffffffff8bbdc5e0
RBP: ffff8880206d0580 R08: ffffffff8da2aa13 R09: 1ffffffff1b45542
R10: dffffc0000000000 R11: fffffbfff1b45543 R12: 0000000000000004
R13: ffff8880206d0500 R14: dffffc0000000000 R15: 0000000000000000
FS: 0000555555cbc480(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffe47239e08 CR3: 0000000024f32000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
----------------
Code disassembly (best guess):
0: 28 00 sub %al,(%rax)
2: 00 00 add %al,(%rax)
4: 75 05 jne 0xb
6: 48 83 c4 28 add $0x28,%rsp
a: c3 ret
b: e8 e1 20 00 00 call 0x20f1
10: 90 nop
11: 48 89 f8 mov %rdi,%rax
14: 48 89 f7 mov %rsi,%rdi
17: 48 89 d6 mov %rdx,%rsi
1a: 48 89 ca mov %rcx,%rdx
1d: 4d 89 c2 mov %r8,%r10
20: 4d 89 c8 mov %r9,%r8
23: 4c 8b 4c 24 08 mov 0x8(%rsp),%r9
28: 0f 05 syscall
* 2a: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax <-- trapping instruction
30: 73 01 jae 0x33
32: c3 ret
33: 48 c7 c1 b0 ff ff ff mov $0xffffffffffffffb0,%rcx
3a: f7 d8 neg %eax
3c: 64 89 01 mov %eax,%fs:(%rcx)
3f: 48 rex.W


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

If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Edward Adam Davis]

please test null ptr in autofs_fill_super

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 4bbdb725a36b

diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c
index a5083d447a62..f2e89a444edf 100644
--- a/fs/autofs/inode.c
+++ b/fs/autofs/inode.c
@@ -331,6 +331,9 @@ static int autofs_fill_super(struct super_block *s, struct fs_context *fc)
goto fail;

root_inode = autofs_get_inode(s, S_IFDIR | 0755);
+ if (!root_inode)
+ goto fail;
+
root_inode->i_uid = ctx->uid;
root_inode->i_gid = ctx->gid;

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+662f87...@syzkaller.appspotmail.com

Tested on:

commit: 4bbdb725 Merge tag 'iommu-updates-v6.7' of git://git.k..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=17a6c75b680000

kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=662f87a8ef490f45fa64
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=17bb3be0e80000

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

[Edward Adam Davis]

[Syz logs]

KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
CPU: 0 PID: 5098 Comm: syz-executor.0 Not tainted 6.6.0-syzkaller-15601-g4bbdb725a36b #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023
RIP: 0010:autofs_fill_super+0x47d/0xb50 fs/autofs/inode.c:334

[pid 5095] mount(NULL, "./file1", "autofs", 0, "fd=0x0000000000000000") = -1 ENOMEM (Cannot allocate memory)

[Analysis]
autofs_get_inode() will return null, when memory cannot be allocated.

[Fix]
Confirm that root_inde is not null before using it.

Reported-and-tested-by: syzbot+662f87...@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <ead...@qq.com>
---
fs/autofs/inode.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c
index a5083d447a62..f2e89a444edf 100644
--- a/fs/autofs/inode.c
+++ b/fs/autofs/inode.c
@@ -331,6 +331,9 @@ static int autofs_fill_super(struct super_block *s, struct fs_context *fc)
goto fail;

root_inode = autofs_get_inode(s, S_IFDIR | 0755);
+ if (!root_inode)
+ goto fail;
+
root_inode->i_uid = ctx->uid;
root_inode->i_gid = ctx->gid;

--
2.25.1

[Al Viro]

On Tue, Nov 14, 2023 at 12:25:35PM +0800, Ian Kent wrote:
> >         root_inode = autofs_get_inode(s, S_IFDIR | 0755);
> > +       if (!root_inode)
> > +               goto fail;
>

> Yes, I think this is the only thing it could be.
>
> There's one small problem though, it leaks the dentry info. ino,
> allocated just above. I think this should goto label fail_ino instead.
>
> Note that once the root dentry is allocated then the ino struct will
> be freed when the dentry is freed so ino doesn't need to be freed.

There's a simpler solution:

root_inode = autofs_get_inode(s, S_IFDIR | 0755);

if (root_inode) {

root_inode->i_uid = ctx->uid;
root_inode->i_gid = ctx->gid;
}

root = d_make_root(root_inode);
if (!root)
goto fail_ino;

d_make_root(NULL) will quietly return NULL, which is all you
need. FWIW, I would probably bring the rest of initialization
root_inode->i_fop = &autofs_root_operations;
root_inode->i_op = &autofs_dir_inode_operations;
in there as well.

Incidentally, why bother with separate fail_dput thing? Shove it
into ->s_root and return ret - autofs_kill_sb() will take care
of dropping it...

How about the following:

static int autofs_fill_super(struct super_block *s, struct fs_context *fc)

{
struct autofs_fs_context *ctx = fc->fs_private;
struct autofs_sb_info *sbi = s->s_fs_info;
struct inode *root_inode;
struct autofs_info *ino;

pr_debug("starting up, sbi = %p\n", sbi);

sbi->sb = s;
s->s_blocksize = 1024;
s->s_blocksize_bits = 10;
s->s_magic = AUTOFS_SUPER_MAGIC;
s->s_op = &autofs_sops;
s->s_d_op = &autofs_dentry_operations;
s->s_time_gran = 1;

/*
* Get the root inode and dentry, but defer checking for errors.
*/
ino = autofs_new_ino(sbi);
if (!ino)
return -ENOMEM;

root_inode = autofs_get_inode(s, S_IFDIR | 0755);

if (root_inode) {

root_inode->i_uid = ctx->uid;
root_inode->i_gid = ctx->gid;

root_inode->i_fop = &autofs_root_operations;
root_inode->i_op = &autofs_dir_inode_operations;
}
s->s_root = d_make_root(root_inode);
if (unlikely(!s->s_root)) {
autofs_free_ino(ino);
return -ENOMEM;
}
s->s_root->d_fsdata = ino;

if (ctx->pgrp_set) {
sbi->oz_pgrp = find_get_pid(ctx->pgrp);
if (!sbi->oz_pgrp) {
int ret = invalf(fc, "Could not find process group %d",
ctx->pgrp);
return ret;
}
} else {
sbi->oz_pgrp = get_task_pid(current, PIDTYPE_PGID);
}

if (autofs_type_trigger(sbi->type))
managed_dentry_set_managed(s->s_root);

pr_debug("pipe fd = %d, pgrp = %u\n",
sbi->pipefd, pid_nr(sbi->oz_pgrp));

sbi->flags &= ~AUTOFS_SBI_CATATONIC;
return 0;
}

No gotos, no labels to keep track of...

[Ian Kent]

Yes, I think this is the only thing it could be.

There's one small problem though, it leaks the dentry info. ino,
allocated just above. I think this should goto label fail_ino instead.

Note that once the root dentry is allocated then the ino struct will
be freed when the dentry is freed so ino doesn't need to be freed.

Ian

[Edward Adam Davis]

[Syz logs]
KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]

CPU: 0 PID: 5098 Comm: syz-executor.0 Not tainted 6.6.0-syzkaller-15601-g4bbdb725a36b #0

Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023
RIP: 0010:autofs_fill_super+0x47d/0xb50 fs/autofs/inode.c:334

[pid 5095] mount(NULL, "./file1", "autofs", 0, "fd=0x0000000000000000") = -1 ENOMEM (Cannot allocate memory)

[Analysis]
autofs_get_inode() will return null, when memory cannot be allocated.

[Fix]

Confirm that root_inode is not null before using it.

Reported-and-tested-by: syzbot+662f87...@syzkaller.appspotmail.com
Signed-off-by: Edward Adam Davis <ead...@qq.com>
---
fs/autofs/inode.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/fs/autofs/inode.c b/fs/autofs/inode.c
index a5083d447a62..f2e89a444edf 100644
--- a/fs/autofs/inode.c
+++ b/fs/autofs/inode.c
@@ -331,6 +331,9 @@ static int autofs_fill_super(struct super_block *s, struct fs_context *fc)
goto fail;

root_inode = autofs_get_inode(s, S_IFDIR | 0755);
+ if (!root_inode)

+ goto fail_ino;
+

root_inode->i_uid = ctx->uid;
root_inode->i_gid = ctx->gid;

--
2.25.1

[Ian Kent]

Yes, I think so, AFAICS so far it looks like everything is covered.

I'll look around a bit longer, I need to go over that mount API code

again anyway ...


I'll prepare a patch, the main thing that I was concerned about was

whether the cause really was NULL root_inode but Edward more or less

tested that.


Ian

[Al Viro]

On Tue, Nov 14, 2023 at 04:30:25PM +0800, Ian Kent wrote:

> I'll prepare a patch, the main thing that I was concerned about was
>
> whether the cause really was NULL root_inode but Edward more or less
>
> tested that.

One thing: that was a massaged copy of the variant in my local tree, so
this

> > managed_dentry_set_managed(s->s_root);

might be worth an explanation; mainline has __managed_dentry_set_managed()
here, and yes, it is safe since nothing can access it yet, but... it's
not worth skipping on spin_lock/spin_unlock for ->d_flags update here.

[Ian Kent]

Sorry, do you mean explanation of why we are not skipping the spin lock

or why we are setting automount trigger flags on the autofs root dentry?


Being a trigger mount (type direct or offset) they do need the flags, the

mount is mounted over the trigger.


I guess that including the locking is not going to make much difference.

I don't remember now but it was probably done because there may be many

mounts (potentially several thousand) being done and I wanted to get rid

of anything that wasn't needed.


Ian

[Al Viro]

On Wed, Nov 15, 2023 at 08:18:33AM +0800, Ian Kent wrote:

> I guess that including the locking is not going to make much difference.
>
> I don't remember now but it was probably done because there may be many
>
> mounts (potentially several thousand) being done and I wanted to get rid
>
> of anything that wasn't needed.

Seeing that lock in question is not going to be contended... ;-)

Seriously, though - the fewer complications we have in the locking rules,
the better.

Al, currently going through audit of ->d_name/->d_parent uses and cursing
at the 600-odd places left to look through...

[Ian Kent]

On 15/11/23 08:35, Al Viro wrote:
> On Wed, Nov 15, 2023 at 08:18:33AM +0800, Ian Kent wrote:
>
>> I guess that including the locking is not going to make much difference.
>>
>> I don't remember now but it was probably done because there may be many
>>
>> mounts (potentially several thousand) being done and I wanted to get rid
>>
>> of anything that wasn't needed.
> Seeing that lock in question is not going to be contended... ;-)
>
> Seriously, though - the fewer complications we have in the locking rules,
> the better.

Sure, no problem, I'll make that change.

I do need to do a compile and test it actually ok works so it's

taking a little while.


Ian

[syzbot]

syzbot has bisected this issue to:

commit e6ec453bd0f03a60a80f00f95ae2eaa260faa3c2
Author: Ian Kent <ra...@themaw.net>
Date: Fri Sep 22 04:12:14 2023 +0000

autofs: convert autofs to use the new mount api

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=12d04220e80000
start commit: 4bbdb725a36b Merge tag 'iommu-updates-v6.7' of git://git.k..
git tree: upstream
final oops: https://syzkaller.appspot.com/x/report.txt?x=11d04220e80000
console output: https://syzkaller.appspot.com/x/log.txt?x=16d04220e80000

kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=662f87a8ef490f45fa64

syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14384a7b680000

Reported-by: syzbot+662f87...@syzkaller.appspotmail.com
Fixes: e6ec453bd0f0 ("autofs: convert autofs to use the new mount api")

For information about bisection process see: https://goo.gl/tpsmEJ#bisection