From mboxrd@z Thu Jan 1 00:00:00 1970 From: Djalal Harouni Subject: Re: [PATCH review 02/13] mnt: Refactor fs_fully_visible into mount_too_revealing Date: Thu, 23 Jun 2016 23:23:12 +0200 Message-ID: References: <87fus77pns.fsf@x220.int.ebiederm.org> <20160620172130.15712-1-ebiederm@xmission.com> <20160620172130.15712-2-ebiederm@xmission.com> <874m8m4bky.fsf@x220.int.ebiederm.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <874m8m4bky.fsf-JOvCrm2gF+uungPnsOpG7nhyD016LWXt@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: "Eric W. Biederman" Cc: Miklos Szeredi , Linux Containers , Andy Lutomirski , James Bottomley , Seth Forshee , Linux FS Devel List-Id: containers.vger.kernel.org On Tue, Jun 21, 2016 at 8:54 PM, Eric W. Biederman wrote: > Andy Lutomirski writes: > >> On Mon, Jun 20, 2016 at 10:21 AM, Eric W. Biederman >> 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-f181.google.com ([209.85.220.181]:35913 "EHLO mail-qk0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751272AbcFWVXO (ORCPT ); Thu, 23 Jun 2016 17:23:14 -0400 Received: by mail-qk0-f181.google.com with SMTP id p10so123678487qke.3 for ; Thu, 23 Jun 2016 14:23:13 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <874m8m4bky.fsf@x220.int.ebiederm.org> References: <87fus77pns.fsf@x220.int.ebiederm.org> <20160620172130.15712-1-ebiederm@xmission.com> <20160620172130.15712-2-ebiederm@xmission.com> <874m8m4bky.fsf@x220.int.ebiederm.org> From: Djalal Harouni Date: Thu, 23 Jun 2016 23:23:12 +0200 Message-ID: Subject: Re: [PATCH review 02/13] mnt: Refactor fs_fully_visible into mount_too_revealing To: "Eric W. Biederman" Cc: Andy Lutomirski , Linux Containers , Linux FS Devel , Miklos Szeredi , James Bottomley , Seth Forshee , "Serge E. Hallyn" Content-Type: text/plain; charset=UTF-8 Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Jun 21, 2016 at 8:54 PM, Eric W. Biederman wrote: > Andy Lutomirski writes: > >> On Mon, Jun 20, 2016 at 10:21 AM, Eric W. Biederman >> 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