Messages in this thread | | | Date | Tue, 14 Nov 2023 04:41:10 +0000 | From | Al Viro <> | Subject | Re: [PATCH] autofs: fix null deref in autofs_fill_super |
| |
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...
| |