From: Djalal Harouni <tixxdz-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> To: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> Cc: Miklos Szeredi <miklos-sUDqSbJrdHQHWmgEVkV9KA@public.gmane.org>, Linux Containers <containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>, James Bottomley <James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>, Seth Forshee <seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>, Linux FS Devel <linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> Subject: Re: [PATCH review 02/13] mnt: Refactor fs_fully_visible into mount_too_revealing Date: Thu, 23 Jun 2016 23:23:12 +0200 [thread overview] Message-ID: <CAEiveUf=fnJwVGXLoVeau1_v1gMA1n_FjdjfDAPP47O+C6RkTA@mail.gmail.com> (raw) In-Reply-To: <874m8m4bky.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> On Tue, Jun 21, 2016 at 8:54 PM, Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: > Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> writes: > >> On Mon, Jun 20, 2016 at 10:21 AM, Eric W. Biederman >> <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote: >>> Replace the call of fs_fully_visible in do_new_mount from before the >>> new superblock is allocated with a call of mount_too_revealing after >>> the superblock is allocated. This winds up being a much better location >>> for maintainability of the code. >>> >>> The first change this enables is the replacement of FS_USERNS_VISIBLE >>> with SB_I_USERNS_VISIBLE. Moving the flag from struct filesystem_type >>> to sb_iflags on the superblock. >> >> Why is this useful? > > A couple of reasons. > - It helps clean up do_new_mount which is currently so overloaded by > special cases that it is difficult to see the core logic. > > - It makes the check about the actual superblock that is being mounted > rather than the superblock that might be mounted. > > - The practical place where being about the actual superblock that is > being mounted helps is that in "11/13 mnt: Simplify mount_too_revealing" > that removes the MNT_LOCK_NOSUID MNT_LOCK_NOEXEC and MNT_LOCK_NODEV > tests from the code, while verify that those tests are not needed > because the sb_iflags contains SB_I_NOEXEC and SB_I_NODEV. Yes, but it seems in that patch 11/13 the SB_I_NOEXEC and SB_I_NODEV flags are only enforced and checked in case 'user_ns != init_user_ns' so for init_user_ns we don't enforce it. Even if we set the flags and things are correct now, but as you have noted in your previous patches related to this we try to commit to never exec on procfs and sysfs... so maybe take that check on its own and move it before the init_user_ns one ? > - The conceptual change of testing once the superblock has been > generated makes changes like the one above much more sensible > and it helps untangle mount namespace versus superblock concerns. > > That last is a big part of what this patchset is about. When do we care > about the superblock and when do we care about the mount namespace. Historically fs_fully_visible() or mount_too_revealing() now gathered lot of security checks... so one may wonder about the implication of moving it after !?... yes having a clear context about superblocks and mount namespaces matters... but I'm not sure about the order. > Eric > Thank you! -- tixxdz http://opendz.org
WARNING: multiple messages have this Message-ID (diff)
From: Djalal Harouni <tixxdz@gmail.com> To: "Eric W. Biederman" <ebiederm@xmission.com> Cc: Andy Lutomirski <luto@amacapital.net>, Linux Containers <containers@lists.linux-foundation.org>, Linux FS Devel <linux-fsdevel@vger.kernel.org>, Miklos Szeredi <miklos@szeredi.hu>, James Bottomley <James.Bottomley@hansenpartnership.com>, Seth Forshee <seth.forshee@canonical.com>, "Serge E. Hallyn" <serge@hallyn.com> Subject: Re: [PATCH review 02/13] mnt: Refactor fs_fully_visible into mount_too_revealing Date: Thu, 23 Jun 2016 23:23:12 +0200 [thread overview] Message-ID: <CAEiveUf=fnJwVGXLoVeau1_v1gMA1n_FjdjfDAPP47O+C6RkTA@mail.gmail.com> (raw) In-Reply-To: <874m8m4bky.fsf@x220.int.ebiederm.org> On Tue, Jun 21, 2016 at 8:54 PM, Eric W. Biederman <ebiederm@xmission.com> wrote: > Andy Lutomirski <luto@amacapital.net> writes: > >> On Mon, Jun 20, 2016 at 10:21 AM, Eric W. Biederman >> <ebiederm@xmission.com> wrote: >>> Replace the call of fs_fully_visible in do_new_mount from before the >>> new superblock is allocated with a call of mount_too_revealing after >>> the superblock is allocated. This winds up being a much better location >>> for maintainability of the code. >>> >>> The first change this enables is the replacement of FS_USERNS_VISIBLE >>> with SB_I_USERNS_VISIBLE. Moving the flag from struct filesystem_type >>> to sb_iflags on the superblock. >> >> Why is this useful? > > A couple of reasons. > - It helps clean up do_new_mount which is currently so overloaded by > special cases that it is difficult to see the core logic. > > - It makes the check about the actual superblock that is being mounted > rather than the superblock that might be mounted. > > - The practical place where being about the actual superblock that is > being mounted helps is that in "11/13 mnt: Simplify mount_too_revealing" > that removes the MNT_LOCK_NOSUID MNT_LOCK_NOEXEC and MNT_LOCK_NODEV > tests from the code, while verify that those tests are not needed > because the sb_iflags contains SB_I_NOEXEC and SB_I_NODEV. Yes, but it seems in that patch 11/13 the SB_I_NOEXEC and SB_I_NODEV flags are only enforced and checked in case 'user_ns != init_user_ns' so for init_user_ns we don't enforce it. Even if we set the flags and things are correct now, but as you have noted in your previous patches related to this we try to commit to never exec on procfs and sysfs... so maybe take that check on its own and move it before the init_user_ns one ? > - The conceptual change of testing once the superblock has been > generated makes changes like the one above much more sensible > and it helps untangle mount namespace versus superblock concerns. > > That last is a big part of what this patchset is about. When do we care > about the superblock and when do we care about the mount namespace. Historically fs_fully_visible() or mount_too_revealing() now gathered lot of security checks... so one may wonder about the implication of moving it after !?... yes having a clear context about superblocks and mount namespaces matters... but I'm not sure about the order. > Eric > Thank you! -- tixxdz http://opendz.org
next prev parent reply other threads:[~2016-06-23 21:23 UTC|newest] Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-06-20 17:09 [PATCH review 0/13] Adding a userns owner to struct super_block Eric W. Biederman 2016-06-20 17:09 ` Eric W. Biederman [not found] ` <87fus77pns.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> 2016-06-20 17:21 ` [PATCH review 01/13] mnt: Account for MS_RDONLY in fs_fully_visible Eric W. Biederman 2016-06-20 17:21 ` Eric W. Biederman [not found] ` <20160620172130.15712-1-ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> 2016-06-20 17:21 ` [PATCH review 02/13] mnt: Refactor fs_fully_visible into mount_too_revealing Eric W. Biederman 2016-06-20 17:21 ` Eric W. Biederman [not found] ` <20160620172130.15712-2-ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> 2016-06-20 22:53 ` Andy Lutomirski 2016-06-20 22:53 ` Andy Lutomirski [not found] ` <CALCETrXv2aeP38AmUaRVMC+O-oeWKwDcy8fPfsOCu1f8mncZEA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-06-21 18:54 ` Eric W. Biederman 2016-06-21 18:54 ` Eric W. Biederman [not found] ` <874m8m4bky.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> 2016-06-23 21:23 ` Djalal Harouni [this message] 2016-06-23 21:23 ` Djalal Harouni 2016-06-24 6:56 ` Serge E. Hallyn 2016-06-24 6:56 ` Serge E. Hallyn 2016-06-22 19:40 ` Seth Forshee 2016-06-22 19:40 ` Seth Forshee 2016-06-20 17:21 ` [PATCH review 03/13] ipc: Initialize ipc_namespace->user_ns early Eric W. Biederman 2016-06-20 17:21 ` Eric W. Biederman [not found] ` <20160620172130.15712-3-ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> 2016-06-24 14:34 ` Djalal Harouni 2016-06-24 14:34 ` Djalal Harouni 2016-06-20 17:21 ` [PATCH review 04/13] vfs: Pass data, ns, and ns->userns to mount_ns Eric W. Biederman 2016-06-20 17:21 ` Eric W. Biederman 2016-06-20 17:21 ` [PATCH review 05/13] proc: Convert proc_mount to use mount_ns Eric W. Biederman 2016-06-20 17:21 ` Eric W. Biederman [not found] ` <20160620172130.15712-5-ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> 2016-06-23 11:50 ` Djalal Harouni 2016-06-23 11:50 ` Djalal Harouni 2016-06-20 17:21 ` [PATCH review 06/13] fs: Add user namespace member to struct super_block Eric W. Biederman 2016-06-20 17:21 ` Eric W. Biederman 2016-06-20 17:21 ` [PATCH review 07/13] mnt: Move the FS_USERNS_MOUNT check into sget_userns Eric W. Biederman 2016-06-20 17:21 ` Eric W. Biederman [not found] ` <20160620172130.15712-7-ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> 2016-06-22 19:43 ` Seth Forshee 2016-06-22 19:43 ` Seth Forshee 2016-06-20 17:21 ` [PATCH review 08/13] kernfs: The cgroup filesystem also benefits from SB_I_NOEXEC Eric W. Biederman 2016-06-20 17:21 ` Eric W. Biederman 2016-06-20 17:21 ` [PATCH review 09/13] ipc/mqueue: The mqueue filesystem should never contain executables Eric W. Biederman 2016-06-20 17:21 ` Eric W. Biederman 2016-06-20 17:21 ` [PATCH review 10/13] vfs: Generalize filesystem nodev handling Eric W. Biederman 2016-06-20 17:21 ` Eric W. Biederman [not found] ` <20160620172130.15712-10-ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> 2016-06-20 22:57 ` Andy Lutomirski 2016-06-20 22:57 ` Andy Lutomirski [not found] ` <CALCETrUWsnRgjyRyb+_0u0PYubx9gg=hUAso=073yjJY+m205g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-06-21 19:09 ` Eric W. Biederman 2016-06-21 19:09 ` Eric W. Biederman 2016-06-20 17:21 ` [PATCH review 11/13] mnt: Simplify mount_too_revealing Eric W. Biederman 2016-06-20 17:21 ` Eric W. Biederman [not found] ` <20160620172130.15712-11-ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> 2016-06-22 19:48 ` Seth Forshee 2016-06-22 19:48 ` Seth Forshee 2016-06-20 17:21 ` [PATCH review 12/13] userns: Remove implicit MNT_NODEV fragility Eric W. Biederman 2016-06-20 17:21 ` Eric W. Biederman 2016-06-22 19:49 ` Seth Forshee [not found] ` <20160620172130.15712-12-ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> 2016-06-20 22:58 ` Andy Lutomirski 2016-06-20 22:58 ` Andy Lutomirski 2016-06-22 19:49 ` Seth Forshee 2016-06-20 17:21 ` [PATCH review 13/13] userns: Remove the now unnecessary FS_USERNS_DEV_MOUNT flag Eric W. Biederman 2016-06-20 17:21 ` Eric W. Biederman 2016-06-22 19:50 ` Seth Forshee [not found] ` <20160620172130.15712-13-ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> 2016-06-22 19:50 ` Seth Forshee
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='CAEiveUf=fnJwVGXLoVeau1_v1gMA1n_FjdjfDAPP47O+C6RkTA@mail.gmail.com' \ --to=tixxdz-re5jqeeqqe8avxtiumwx3w@public.gmane.org \ --cc=James.Bottomley-JuX6DAaQMKPCXq6kfMZ53/egYHeGw8Jk@public.gmane.org \ --cc=containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org \ --cc=ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org \ --cc=linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org \ --cc=miklos-sUDqSbJrdHQHWmgEVkV9KA@public.gmane.org \ --cc=seth.forshee-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.