linux-kernel - Re: [PATCH] autofs: fix null deref in autofs_fill

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e2654c2c-947a-60e5-7b86-9a13590f6211@themaw.net>
Date:   Tue, 14 Nov 2023 16:30:25 +0800
From:   Ian Kent <raven@...maw.net>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Edward Adam Davis <eadavis@...com>,
        syzbot+662f87a8ef490f45fa64@...kaller.appspotmail.com,
        autofs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] autofs: fix null deref in autofs_fill_super

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