linux-kernel - Re: [git pull] vfs fixes

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: <20170403074653.GN29622@ZenIV.linux.org.uk>
Date:   Mon, 3 Apr 2017 08:46:53 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Ian Kent <raven@...maw.net>
Subject: Re: [git pull] vfs fixes

On Mon, Apr 03, 2017 at 01:00:56AM -0500, Eric W. Biederman wrote:

> Refereshing my memory.  d_automount mounts things and is what
> is used for nfs referrals.  d_manage blocks waiting for
> an automounts to complete or expire.  follow_down just calls d_manage,
> follow_manage calls both d_manage and d_automount as appropriate.

D'oh...  Right.  What's more, by that point getting back to original
state on error is needed.

> If the concern is nfs referral points calling follow_down is wrong and
> what is wanted is follow_managed.

... except that follow_managed() takes nameidata and there is no way in
hell we are letting that animal out of fs/namei.c ever again.  Too low-level.

> The intent of the logic in mntns_install is to just pick a reasonable
> looking place somewhere in that mount namespace to use as a root
> directory.  I arbitrarily picked the top of the mount stack on "/".  Which
> is typically used as the root directory.  If people really care where
> their root is they save a directory file descriptor off somewhere and
> call chroot.  So there is a little wiggle room exactly what the code
> does.

Hmm...  If anything, I'm tempted to add LOOKUP_DOWN that would have
path_lookupat() do
	if (unlikely(flags & LOOKUP_DOWN)) {
		struct path path = nd->path;
		dget(nd->path.dentry);
		err = follow_managed(&path, nd);
		if (unlikely(err < 0))
			terminate_walk(nd);
			return err;
		}
		path_to_nameidate(&path, nd);
	}
right after path_init().  Then your stuff would've turned into

        get_mnt_ns(mnt_ns);
        old_mnt_ns = nsproxy->mnt_ns;
        nsproxy->mnt_ns = mnt_ns;

	/* Find the root */
	err = vfs_path_lookup(mnt_ns->root->mnt.mnt_root, &mnt_ns->root->mnt,
				"/", LOOKUP_DOWN, &root);
	if (err) {
		/* revert to old namespace */
		nsproxy->mnt_ns = old_mnt_ns;
		put_mnt_ns(mnt_ns);
		return err;
	}

        /* Update the pwd and root */
        set_fs_pwd(fs, &root);
        set_fs_root(fs, &root);

        path_put(&root);
	put_mnt_ns(old_mnt_ns);
	return 0;

This is absolutely untested, and I won't get around to testing it until
tomorrow, but something along those lines would IMO be better than
exposing a trimmed-down follow_managed(), not to mention struct nameidata
itself...