Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
From: Al Viro <viro@zeniv.linux.org.uk>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	David Howells <dhowells@redhat.com>,
	linux-api@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	torvalds@linux-foundation.org,
	linux-security-module@vger.kernel.org
Subject: Re: [PATCH 02/10] vfs: syscall: Add move_mount(2) to move mounts around
Date: Mon, 8 Jul 2019 21:21:24 +0100
Message-ID: <20190708202124.GX17978@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20190708180132.GU17978@ZenIV.linux.org.uk>

On Mon, Jul 08, 2019 at 07:01:32PM +0100, Al Viro wrote:
> On Mon, Jul 08, 2019 at 12:12:21PM -0500, Eric W. Biederman wrote:
> 
> > Al you do realize that the TOCTOU you are talking about comes the system
> > call API.  TOMOYO can only be faulted for not playing in their own
> > sandbox and not reaching out and fixing the vfs implementation details.

PS: the fact that mount(2) has been overloaded to hell and back (including
MS_MOVE, which goes back to v2.5.0.5) predates the introduction of ->sb_mount()
and LSM in general (2.5.27).  MS_BIND is 2.4.0-test9pre2.

In all the years since the introduction of ->sb_mount() I've seen zero
questions from LSM folks regarding a sane place for those checks.  What I have
seen was "we want it immediately upon the syscall entry, let the module
figure out what to do" in reply to several times I tried to tell them "folks,
it's called in a bad place; you want the checks applied to objects, not to
raw string arguments".

As it is, we have easily bypassable checks on mount(2) (by way of ->sb_mount();
there are other hooks also in the game for remounts and new mounts).

I see no point whatsoever trying to duplicate ->sb_mount() on the top level
of move_mount(2).  When and if sane checks are agreed upon for that thing,
they certainly should be used both for MS_MOVE case of mount(2) and for
move_mount(2).  And that'll come for free from calling those in do_move_mount().
They won't be the first thing called in mount(2) - we demultiplex first,
decide that we have a move and do pathname resolution on source.  And that's
precisely what we need to avoid the TOCTOU there.

I'm sorry, but this "run the hook at the very beginning, the modules know
better what they want, just give them as close to syscall arguments as
possible before even looking at the flags" model is wrong, plain and simple.

As for the MS_MOVE checks, the arguments are clear enough (two struct path *,
same as what we pass to do_move_mount()).  AFAICS, only tomoyo and
apparmor are trying to do anything for MS_MOVE in ->sb_mount(), and both
look like it should be easy enough to implement what said checks intend
to do (probably - assuming that aa_move_mount() doesn't depend upon
having its kern_path() inside the __begin_current_label_crit_section()/
__end_current_label_crit_section() pair; looks like it shouldn't be,
but that's for apparmor folks to decide).

That's really for LSM folks, though - I've given up on convincing
(or even getting a rationale out of) them on anything related to hook
semantics years ago.

  parent reply index

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-19 17:08 [PATCH 00/10] VFS: Provide new mount UAPI David Howells
2019-02-19 17:08 ` [PATCH 01/10] vfs: syscall: Add open_tree(2) to reference or clone a mount David Howells
2019-02-19 17:08 ` [PATCH 02/10] vfs: syscall: Add move_mount(2) to move mounts around David Howells
2019-02-20 12:32   ` Alan Jenkins
2019-02-20 12:41     ` Alan Jenkins
2019-02-20 16:23   ` Jann Horn
2019-07-08 12:02   ` Tetsuo Handa
2019-07-08 13:18     ` Al Viro
2019-07-08 17:12       ` Eric W. Biederman
2019-07-08 18:01         ` Al Viro
2019-07-08 18:13           ` Al Viro
2019-07-08 20:21           ` Al Viro [this message]
2019-07-09  0:13             ` Eric W. Biederman
2019-07-09 10:51               ` Tetsuo Handa
2019-07-22 10:12                 ` Tetsuo Handa
2019-07-23  4:16                   ` John Johansen
2019-07-23 13:45                     ` Tetsuo Handa
2019-08-06 10:43                       ` Tetsuo Handa
2019-08-09 15:44                         ` [PATCH] LSM: Disable move_mount() syscall when TOMOYO or AppArmor is enabled Tetsuo Handa
2019-08-22  3:51                         ` [RFC][PATCH] fix d_absolute_path() interplay with fsmount() Al Viro
2019-08-30 10:11                           ` Tetsuo Handa
2019-07-23 21:45             ` [PATCH 02/10] vfs: syscall: Add move_mount(2) to move mounts around James Morris
2019-07-23 23:30               ` Al Viro
2019-02-19 17:08 ` [PATCH 03/10] teach move_mount(2) to work with OPEN_TREE_CLONE David Howells
2019-02-20 18:59   ` Alan Jenkins
2019-02-26 17:45   ` Alan Jenkins
2019-02-19 17:08 ` [PATCH 04/10] Make anon_inodes unconditional David Howells
2019-02-19 17:09 ` [PATCH 05/10] vfs: syscall: Add fsopen() to prepare for superblock creation David Howells
2019-02-19 17:09 ` [PATCH 06/10] vfs: Implement logging through fs_context David Howells
2019-02-19 17:09 ` [PATCH 07/10] vfs: syscall: Add fsconfig() for configuring and managing a context David Howells
2019-02-19 17:09 ` [PATCH 08/10] vfs: syscall: Add fsmount() to create a mount for a superblock David Howells
2019-02-19 17:09 ` [PATCH 09/10] vfs: syscall: Add fspick() to select a superblock for reconfiguration David Howells
2019-02-19 17:09 ` [PATCH 10/10] vfs: Add a sample program for the new mount API David Howells

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190708202124.GX17978@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=dhowells@redhat.com \
    --cc=ebiederm@xmission.com \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git