Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks

[Al Viro <viro@zeniv.linux.org.uk>]

On Mon, Dec 30, 2019 at 06:29:59PM +1100, Aleksa Sarai wrote:
> On 2019-12-30, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > On 2019-12-30, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > > On Mon, Dec 30, 2019 at 04:20:35PM +1100, Aleksa Sarai wrote:
> > > 
> > > > A reasonably detailed explanation of the issues is provided in the patch
> > > > itself, but the full traces produced by both the oopses and deadlocks is
> > > > included below (it makes little sense to include them in the commit since we
> > > > are disabling this feature, not directly fixing the bugs themselves).
> > > > 
> > > > I've posted this as an RFC on whether this feature should be allowed at
> > > > all (and if anyone knows of legitimate uses for it), or if we should
> > > > work on fixing these other kernel bugs that it exposes.
> > > 
> > > Umm...  Are all of those traces
> > > 	a) reproducible on mainline and
> > 
> > This was on viro/for-next, I'll retry it on v5.5-rc4.
> 
> The NULL deref oops is reproducible on v5.5-rc4. Strangely it seems
> harder to reproduce than on viro/for-next (I kept reproducing it there
> by accident), but I'll double-check if that really is the case.
> 
> The simplest reproducer is (using the attached programs and .config):
> 
>   ln -s . link
>   sudo ./umount_symlink link

FWIW, the problem with that reproducer is that we *CAN'T* resolve that
path.  Look: you have /proc/self/fd/3 resolve to ./link.  OK, you've
asked to follow that.  Got ./link, which is a symlink, so we need to
follow it further.  Relative to what, though?

The meaning of symlink is dependent upon the directory you find it in.
And we don't have any here.

The bug is in mountpoint_last() - we have
        if (unlikely(nd->last_type != LAST_NORM)) {
                error = handle_dots(nd, nd->last_type);
                if (error)
                        return error;
                path.dentry = dget(nd->path.dentry);
        } else {
                path.dentry = d_lookup(dir, &nd->last);
                if (!path.dentry) {
                        /*
                         * No cached dentry. Mounted dentries are pinned in the
                         * cache, so that means that this dentry is probably
                         * a symlink or the path doesn't actually point
                         * to a mounted dentry.
                         */
                        path.dentry = lookup_slow(&nd->last, dir,
                                             nd->flags | LOOKUP_NO_REVAL);
                        if (IS_ERR(path.dentry))
                                return PTR_ERR(path.dentry);
                }
        }
        if (d_flags_negative(smp_load_acquire(&path.dentry->d_flags))) {
                dput(path.dentry);
                return -ENOENT;
        }
        path.mnt = nd->path.mnt;
        return step_into(nd, &path, 0, d_backing_inode(path.dentry), 0);
in there, and that ends up with step_into() called in case of LAST_DOT/LAST_DOTDOT
(where it's harmless) *AND* in case of LAST_BIND.  Where it very much isn't.

I'm not sure if you have caught anything else, but we really, really should *NOT*
consider the LAST_BIND as "maybe we should follow the result" material.  So
at least the following is needed; could you check if anything else remains
with that applied?

diff --git a/fs/namei.c b/fs/namei.c
index d6c91d1e88cb..d4fbbda8a7ff 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2656,10 +2656,7 @@ mountpoint_last(struct nameidata *nd)
 	nd->flags &= ~LOOKUP_PARENT;
 
 	if (unlikely(nd->last_type != LAST_NORM)) {
-		error = handle_dots(nd, nd->last_type);
-		if (error)
-			return error;
-		path.dentry = dget(nd->path.dentry);
+		return handle_dots(nd, nd->last_type);
 	} else {
 		path.dentry = d_lookup(dir, &nd->last);
 		if (!path.dentry) {