All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: Ian Kent <raven@themaw.net>
Cc: Edward Adam Davis <eadavis@qq.com>,
	syzbot+662f87a8ef490f45fa64@syzkaller.appspotmail.com,
	autofs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com
Subject: Re: [PATCH] autofs: fix null deref in autofs_fill_super
Date: Tue, 14 Nov 2023 04:41:10 +0000	[thread overview]
Message-ID: <20231114044110.GR1957730@ZenIV> (raw)
In-Reply-To: <4fcf49456c32087f5306e84c4a8df5b2bd9f4146.camel@themaw.net>

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

  reply	other threads:[~2023-11-14  4:41 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-14  1:02 [syzbot] [autofs?] general protection fault in autofs_fill_super syzbot
2023-11-14  2:17 ` [syzbot] [PATCH] Test np " syzbot
2023-11-14  3:52 ` [PATCH] autofs: fix null deref " Edward Adam Davis
2023-11-14  4:25   ` Ian Kent
2023-11-14  4:41     ` Al Viro [this message]
2023-11-14  8:30       ` Ian Kent
2023-11-14 15:26         ` Al Viro
2023-11-15  0:18           ` Ian Kent
2023-11-15  0:35             ` Al Viro
2023-11-15  1:06               ` Ian Kent
2023-11-14  5:48     ` [PATCH V2] autofs: fix null ptr " Edward Adam Davis
2023-11-16  9:51 ` [syzbot] [autofs?] general protection fault " syzbot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20231114044110.GR1957730@ZenIV \
    --to=viro@zeniv.linux.org.uk \
    --cc=autofs@vger.kernel.org \
    --cc=eadavis@qq.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=raven@themaw.net \
    --cc=syzbot+662f87a8ef490f45fa64@syzkaller.appspotmail.com \
    --cc=syzkaller-bugs@googlegroups.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.