All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ian Kent <raven@themaw.net>
To: Al Viro <viro@zeniv.linux.org.uk>
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 16:30:25 +0800	[thread overview]
Message-ID: <e2654c2c-947a-60e5-7b86-9a13590f6211@themaw.net> (raw)
In-Reply-To: <20231114044110.GR1957730@ZenIV>

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

  reply	other threads:[~2023-11-14  8:30 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
2023-11-14  8:30       ` Ian Kent [this message]
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=e2654c2c-947a-60e5-7b86-9a13590f6211@themaw.net \
    --to=raven@themaw.net \
    --cc=autofs@vger.kernel.org \
    --cc=eadavis@qq.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=syzbot+662f87a8ef490f45fa64@syzkaller.appspotmail.com \
    --cc=syzkaller-bugs@googlegroups.com \
    --cc=viro@zeniv.linux.org.uk \
    /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.