| Message ID | tencent_3744B76B9760E6DA33798369C96563B2C405@qq.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | autofs: fix null deref in autofs_fill_super | expand |
On Tue, 2023-11-14 at 11:52 +0800, Edward Adam Davis wrote: > [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+662f87a8ef490f45fa64@syzkaller.appspotmail.com > Signed-off-by: Edward Adam Davis <eadavis@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; 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
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...
On 14/11/23 12:41, Al Viro wrote: > 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: 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 > 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...
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.
On 14/11/23 23:26, Al Viro wrote: > 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. 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
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...
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 > > Al, currently going through audit of ->d_name/->d_parent uses and cursing > at the 600-odd places left to look through...
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;
[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+662f87a8ef490f45fa64@syzkaller.appspotmail.com Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- fs/autofs/inode.c | 3 +++ 1 file changed, 3 insertions(+)