Message-ID: <20181205161601.GW2217@ZenIV.linux.org.uk>
Date:   Wed, 5 Dec 2018 16:16:01 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Ondrej Mosnacek <omosnace@...hat.com>
Cc:     Paul Moore <paul@...l-moore.com>,
        Stephen Rothwell <sfr@...b.auug.org.au>,
        linux-next@...r.kernel.org,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>,
        David Howells <dhowells@...hat.com>, selinux@...r.kernel.org,
        linux-fsdevel@...r.kernel.org
Subject: Re: linux-next: manual merge of the selinux tree with the vfs tree

On Wed, Dec 05, 2018 at 10:37:56AM +0100, Ondrej Mosnacek wrote:

> I just tested the Q28 branch rebased onto a recent Fedora rawhide
> kernel (4.20.0-0.rc5.git0.1) and that code seems to be working fine.
> The submount test failed with Q28 and succeeds with Q28+fix, as
> expected. Also, the overlay tests failures are gone now (except for
> the 4 known failures from GH issue #43, since I had to rebase onto
> 4.20-rcX).
> 
> This is the commit that I used as the SELinux submount fix:
> https://gitlab.com/omos/linux-public/commit/47922f9c70a83008388b836c285f94c40da1af2b

FWIW, I'm none too happy about the fix.  Observations:
	* sb_get_tree (and sb_kern_mount past the "LSM: lift parsing
LSM options into the caller of ->sb_kern_mount()" in this series)
is equivalent to sb_set_mnt_opts() + (for userland mounts) an selinux-only
MAC check.  IOW, application of options (for LSMs that have those
in the first place) + actual "are we permitted to mount that?" check.
	* the second part should be done only for userland mounts -
not automounting, not kernel-internal ones, etc.
	* a very common pattern is "vfs_get_tree, vfs_create_mount if we
hadn't failed that, then unconditional put_fs_context".  So much that it
clearly deserves a helper - too much boilerplate as it is.
	* look at the callers of vfs_get_tree():
1) afs_mntpt_do_automount(): submount, helper fodder.  No MAC.
2) nfs_do_submount(): submount, standalone, but caller will very shortly follow
with the rest of the helper.  No MAC.
3) btrfs_mount_root(): helper fodder, cloned context, probably no point
in the actual MAC - we are in ->get_tree(), the caller will decide if
it wants to bother.
4) do_nfs4_mount(): NFS counterpart of the above.
5) mount_one_hugetlbfs(): kernel-internal, helper fodder, no MAC.
6) pid_ns_prepare_proc(): kernel-internal, helper fodder, no MAC.
7) mq_create_mount(): kernel-internal, helper fodder, no MAC.

8) do_new_mount(): almost a helper fodder, wants MAC (mount(2) guts)
9) fsopen(2): standalone, wants MAC (it's mount(2)-equivalent)
10) vfs_kern_mount(): that's a bit more complicated.  It is, again,
a helper fodder.  The need of MAC depends upon the caller, in theory.
Callers:
	simple_pin_fs() - kernel-internal, no MAC
	init_mount_tree() - no MAC, that's the absolute root and that, by
definition, is much too early in the boot (before initramfs, etc.) for
any LSM shite to be applicable.  In any case, it's done by the kernel.
	kern_mount() - kernel-internal, no MAC
	cpuset_get_tree() - part of ->get_tree(), caller will decide if they
want the damn MAC.
	vfs_submount() - automounts, presumably no MAC.

Conclusion: fuck the MAC in vfs_get_tree().  Let's just lift it into
do_new_mount()/fsopen().  The only thing we really need there is to
keep ->s_umount held (exclusive) until after the MAC.  So let vfs_get_tree()
return with fc->root->d_sb->s_umount locked and have mount_create_mount()
(which is _always_ preceded by successful vfs_get_tree()), unlock the
sucker.  In vfs_get_tree() we need to do sb_set_mnt_opts(), with the
rest of it (selinux-only) called by do_new_mount()/fsopen(2).  All there
is to it...

Powered by blists - more mailing lists