diff mbox

[REVIEW,4/6] fs: Allow superblock owner to access do_remount_sb()

Message ID 20180523232538.4880-4-ebiederm@xmission.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric W. Biederman May 23, 2018, 11:25 p.m. UTC 
Superblock level remounts are currently restricted to global
CAP_SYS_ADMIN, as is the path for changing the root mount to
read only on umount. Loosen both of these permission checks to
also allow CAP_SYS_ADMIN in any namespace which is privileged
towards the userns which originally mounted the filesystem.

Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
---
 fs/namespace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Christian Brauner May 24, 2018, 3:58 p.m. UTC | #1 
On Wed, May 23, 2018 at 06:25:36PM -0500, Eric W. Biederman wrote:
> Superblock level remounts are currently restricted to global
> CAP_SYS_ADMIN, as is the path for changing the root mount to
> read only on umount. Loosen both of these permission checks to
> also allow CAP_SYS_ADMIN in any namespace which is privileged
> towards the userns which originally mounted the filesystem.

Acked-by: Christian Brauner <christian@brauner.io>

> 
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Acked-by: Serge Hallyn <serge.hallyn@canonical.com>

Note, I just talked to Serge. This should be Acked-by: Serge Hallyn <serge@hallyn.com>

> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>
> ---
>  fs/namespace.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 5f75969adff1..8ddd14806799 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1590,7 +1590,7 @@ static int do_umount(struct mount *mnt, int flags)
>  		 * Special case for "unmounting" root ...
>  		 * we just try to remount it readonly.
>  		 */
> -		if (!capable(CAP_SYS_ADMIN))
> +		if (!ns_capable(sb->s_user_ns, CAP_SYS_ADMIN))
>  			return -EPERM;
>  		down_write(&sb->s_umount);
>  		if (!sb_rdonly(sb))
> @@ -2333,7 +2333,7 @@ static int do_remount(struct path *path, int ms_flags, int sb_flags,
>  	down_write(&sb->s_umount);
>  	if (ms_flags & MS_BIND)
>  		err = change_mount_flags(path->mnt, ms_flags);
> -	else if (!capable(CAP_SYS_ADMIN))
> +	else if (!ns_capable(sb->s_user_ns, CAP_SYS_ADMIN))
>  		err = -EPERM;
>  	else
>  		err = do_remount_sb(sb, sb_flags, data, 0);
> -- 
> 2.14.1
> 
> _______________________________________________
> Containers mailing list
> Containers@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers
Eric W. Biederman May 24, 2018, 4:45 p.m. UTC | #2 
Christian Brauner <christian@brauner.io> writes:

> On Wed, May 23, 2018 at 06:25:36PM -0500, Eric W. Biederman wrote:
>> Superblock level remounts are currently restricted to global
>> CAP_SYS_ADMIN, as is the path for changing the root mount to
>> read only on umount. Loosen both of these permission checks to
>> also allow CAP_SYS_ADMIN in any namespace which is privileged
>> towards the userns which originally mounted the filesystem.
>
> Acked-by: Christian Brauner <christian@brauner.io>
>
>> 
>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
>
> Note, I just talked to Serge. This should be Acked-by: Serge Hallyn <serge@hallyn.com>

Now you know how long these patches have been sitting waiting to get
merged.

Eric
Christian Brauner May 24, 2018, 5:28 p.m. UTC | #3 
On Thu, May 24, 2018 at 11:45:06AM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian@brauner.io> writes:
> 
> > On Wed, May 23, 2018 at 06:25:36PM -0500, Eric W. Biederman wrote:
> >> Superblock level remounts are currently restricted to global
> >> CAP_SYS_ADMIN, as is the path for changing the root mount to
> >> read only on umount. Loosen both of these permission checks to
> >> also allow CAP_SYS_ADMIN in any namespace which is privileged
> >> towards the userns which originally mounted the filesystem.
> >
> > Acked-by: Christian Brauner <christian@brauner.io>
> >
> >> 
> >> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> >> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> >> Acked-by: Serge Hallyn <serge.hallyn@canonical.com>
> >
> > Note, I just talked to Serge. This should be Acked-by: Serge Hallyn <serge@hallyn.com>
> 
> Now you know how long these patches have been sitting waiting to get
> merged.

Indeed. :)

Christian
diff mbox

Patch

diff --git a/fs/namespace.c b/fs/namespace.c
index 5f75969adff1..8ddd14806799 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1590,7 +1590,7 @@  static int do_umount(struct mount *mnt, int flags)
 		 * Special case for "unmounting" root ...
 		 * we just try to remount it readonly.
 		 */
-		if (!capable(CAP_SYS_ADMIN))
+		if (!ns_capable(sb->s_user_ns, CAP_SYS_ADMIN))
 			return -EPERM;
 		down_write(&sb->s_umount);
 		if (!sb_rdonly(sb))
@@ -2333,7 +2333,7 @@  static int do_remount(struct path *path, int ms_flags, int sb_flags,
 	down_write(&sb->s_umount);
 	if (ms_flags & MS_BIND)
 		err = change_mount_flags(path->mnt, ms_flags);
-	else if (!capable(CAP_SYS_ADMIN))
+	else if (!ns_capable(sb->s_user_ns, CAP_SYS_ADMIN))
 		err = -EPERM;
 	else
 		err = do_remount_sb(sb, sb_flags, data, 0);