linux-kernel - Re: [git pull] mount API series

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: <20181112205439.GF32577@ZenIV.linux.org.uk>
Date:   Mon, 12 Nov 2018 20:54:39 +0000
From:   Al Viro <viro@...IV.linux.org.uk>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Steven Whitehouse <swhiteho@...hat.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [git pull] mount API series

On Sun, Nov 11, 2018 at 08:07:20PM -0600, Eric W. Biederman wrote:
> Steven Whitehouse <swhiteho@...hat.com> writes:

> > Can you share some details of what this NULL dereference is? David and
> > Al have been working on the changes as requested by Linus later in
> > this thread, and they'd like to tidy up this issue too at the same
> > time if possible. We are not asking you to actually provide a fix, in
> > case you are too busy to do so, however it would be good to know what
> > the issue is so that we can make sure that it is resolved in the next
> > round of patches,
> 
> https://lore.kernel.org/lkml/87bm7n5k1r.fsf@xmission.com/

Thought it had been dealt with, but you are right - oops is still there
and obviously needs fixing.  However, looking at that place in mainline...
How does that thing manage to work?  Look:
        /* Notice when we are propagating across user namespaces */
        if (m->mnt_ns->user_ns != user_ns)
                type |= CL_UNPRIVILEGED;
        child = copy_tree(last_source, last_source->mnt.mnt_root, type);
        if (IS_ERR(child))
                return PTR_ERR(child);
        child->mnt.mnt_flags &= ~MNT_LOCKED;
        mnt_set_mountpoint(m, mp, child);
        last_dest = m;
        last_source = child;
OK, we'd created the copy to be attached to the next candidate mountpoint.
If we have e.g. a 4-element peer group, we'll start with what we'd been
asked to mount, then call that sucker three times, getting a copy for
each of those mountpoints.  Right?  Now, what happens if the 1st, 3rd and
4th members live in your namespace, with the second one being elsewhere?
We have
	source_mnt: that'll go on top of the 1st mountpoint
	copy of source_mnt: that'll go on top of the 2nd mountpoint
	copy of copy of source_mnt: that'll go on top of the 3rd mountpoint
	copy of copy of copy of source_mnt: that'll go on top of the 4th one
And AFAICS your logics there has just made sure that everything except the
source_mnt will have MNT_LOCK_... all over the place.  IOW, the effect of
CL_UNPRIVELEGED is cumulative.

How the hell does that code avoid this randomness?  Note had the members of
that peer group been in a different order, you would've gotten a different result.
What am I missing here?

Oops is a separate story, and a regression in its own right; it needs to be
fixed.  But I would really like to sort out the semantics of the existing
code in that area, so that we don't end up with patch conflicts.