On Thu, 2015-03-19 at 20:14 -0500, Eric W. Biederman wrote:
> Ian Kent <ra...@themaw.net> writes:
>
> 2> On Thu, 2015-03-19 at 19:47 +0000, Al Viro wrote:
> >> On Tue, Mar 17, 2015 at 10:45:09AM +0800, Ian Kent wrote:
> >> > From: Ian Kent <ik...@redhat.com>
> >> >
> >> > The mnt_namespace definition will be needed by the usermode helper
> >> > contained execution implementation, move it to include/linux/mount.h.
> >>
> >> I really don't like that. AFAICS, the root of the evil is that fscking
> >> nsproxy keeps a pointer to mnt_namespace while all it really needs to
> >> know about is the address of ns_common field embedded into it. Let's
> >> see...
> >
> > Thought that might be the case.
> >
> >>
> >> fs/namespace.c:697: struct mnt_namespace *ns =
> >> current->nsproxy->mnt_ns;
> >> fs/namespace.c:770: return mnt->mnt_ns == current->nsproxy->mnt_ns;
> >> fs/namespace.c:1502: return
> >> ns_capable(current->nsproxy->mnt_ns->user_ns, CAP_SYS_ADMIN);
> >> fs/namespace.c:1587: return current->nsproxy->mnt_ns->seq >=
> >> mnt_ns->seq;
> >> fs/namespace.c:2293: struct user_namespace *user_ns =
> >> current->nsproxy->mnt_ns->user_ns;
> >> fs/namespace.c:2961: touch_mnt_namespace(current->nsproxy->mnt_ns);
> >> fs/namespace.c:3003: init_task.nsproxy->mnt_ns = ns;
> >> fs/namespace.c:3101: ns_root.mnt = ¤t->nsproxy->mnt_ns->root->mnt;
> >> fs/namespace.c:3119: struct mnt_namespace *ns =
> >> current->nsproxy->mnt_ns;
> >> fs/namespace.c:3159: ns = &nsproxy->mnt_ns->ns;
> >> fs/namespace.c:3187: put_mnt_ns(nsproxy->mnt_ns);
> >> fs/namespace.c:3188: nsproxy->mnt_ns = mnt_ns;
> >> fs/pnode.c:282: user_ns = current->nsproxy->mnt_ns->user_ns;
> >> fs/proc_namespace.c:246: if (!nsp || !nsp->mnt_ns) {
> >> fs/proc_namespace.c:251: ns = nsp->mnt_ns;
> >> include/linux/nsproxy.h:33: struct mnt_namespace *mnt_ns;
> >> kernel/nsproxy.c:37: .mnt_ns = NULL,
> >> kernel/nsproxy.c:70: new_nsp->mnt_ns = copy_mnt_ns(flags,
> >> tsk->nsproxy->mnt_ns, user_ns, new_fs);
> >> kernel/nsproxy.c:71: if (IS_ERR(new_nsp->mnt_ns)) {
> >> kernel/nsproxy.c:72: err = PTR_ERR(new_nsp->mnt_ns);
> >> kernel/nsproxy.c:113: if (new_nsp->mnt_ns)
> >> kernel/nsproxy.c:114: put_mnt_ns(new_nsp->mnt_ns);
> >> kernel/nsproxy.c:160: if (ns->mnt_ns)
> >> kernel/nsproxy.c:161: put_mnt_ns(ns->mnt_ns);
> >>
> >> OK, so we need
> >>
> >> a) add in fs/mount.h
> >> static inline struct mnt_namespace *current_mnt_ns(void)
> >> {
> >> return current->nsproxy->mnt_ns;
> >> }
> >>
> >> and replace a lot of open-coded instances.
> >>
> >> b) lift to_mnt_ns() into fs/mount.h (as static inline)
> >>
> >> c) switch put_mnt_ns() to struct ns_common *, replacing put_mnt_ns(ns)
> >> with put_mnt_ns(&ns->ns) and using to_mnt_ns() in the body to recover
> >> the original.
> >>
> >> d) make copy_mnt_ns() take and return struct ns_common * (same treatment
> >> as for
> >> put_mnt_ns() in (c); replace
> >> new_nsp->mnt_ns = copy_mnt_ns(flags, tsk->nsproxy->mnt_ns,
> >> user_ns, new_fs);
> >> if (IS_ERR(new_nsp->mnt_ns)) {
> >> err = PTR_ERR(new_nsp->mnt_ns);
> >> goto out_ns;
> >> }
> >> with
> >> ns = copy_mnt_ns(flags, &tsk->nsproxy->mnt_ns->ns, user_ns, new_fs);
> >> if (IS_ERR(ns)) {
> >> err = PTR_ERR(ns);
> >> goto out_ns;
> >> }
> >> new_nsp->mnt_ns = to_mnt_ns(ns);
> >>
> >> e) replace struct mnt_namespace *mnt_ns with struct ns_common *__mnt_ns in
> >> nsproxy.h, deal with users of mnt_ns by
> >> * everywhere in the tree replace put_mnt_ns(&...->mnt_ns->ns) with
> >> put_mnt_ns(...->__mnt_ns) (i.e. modify the lines modified by (c) again).
> >> * in fs/namespace.c replace
> >> init_task.nsproxy->mnt_ns = ns;
> >> with
> >> init_task.nsproxy->__mnt_ns = &ns->ns;
> >> replace
> >> ns = &nsproxy->mnt_ns->ns;
> >> with
> >> ns = nsproxy->__mnt_ns;
> >> replace
> >> nsproxy->mnt_ns = mnt_ns;
> >> with
> >> nsproxy>__mnt_ns = &mnt_ns->ns;
> >> * in fs/proc_namespace.c replace
> >> ns = nsp->mnt_ns;
> >> with
> >> ns = to_mnt_ns(nsp->__mnt_ns);
> >> and
> >> if (!nsp || !nsp->mnt_ns) {
> >> with
> >> if (!nsp || !nsp->__mnt_ns) {
> >> * in kernel/nsproxy.c: replace
> >> ns = copy_mnt_ns(flags, &tsk->nsproxy->mnt_ns->ns, user_ns, new_fs);
> >> added in (d) with
> >> ns = copy_mnt_ns(flags, tsk->nsproxy->__mnt_ns, user_ns, new_fs);
> >> and
> >> new_nsp->mnt_ns = to_mnt_ns(ns);
> >> with
> >> new_nsp->__mnt_ns = ns;
> >> The rest of mnt_ns in that file get replaced by __mnt_ns.
> >> * in current_mnt_ns() replace ...->mnt_ns with to_mnt_ns(...->__mnt_ns)
> >>
> >> Do you want me to push such a series in vfs.git? After such 5 commits we
> >> have
> >> no pointers to struct mnt_namespace in nsproxy.h *and* no need for your
> >> patches
> >> to dig into ->mnt_ns->ns - we already have that as ->__mnt_ns.
> >
> > Yes please, I'd be more confident if you did this than me, there's
> > already enough to worry about with the series.
>
> Given that this patchset is a security hole waiting to happen I don't
> see why Al should bother unless there are good reasons to do this
> otherwise.
Having now got to your mail now, I agree.
>
> Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
