Gao feng <gaofeng@xxxxxxxxxxxxxx> writes:
> On 2012/09/14 17:33, Eric W. Biederman wrote:
>> Zhao Hongjiang <zhaohongjiang37@xxxxxxxxx> writes:
>>
>>> On 2012-9-14 5:26, Eric W. Biederman wrote:
>>>> Zhao Hongjiang <zhaohongjiang37@xxxxxxxxx> writes:
>>>>
>>>>> From: Zhao Hongjiang <zhaohongjiang@xxxxxxxxxx>
>>>>>
>>>>> Relax the permission checks to allow unprivileged users that have
>>>>> CAP_SYS_ADMIN permissions in the user namespace referred to by the
>>>>> current mount namespace to be allowed to remount filesystems.
>>>>
>>>> Remount in general make filesystem configuration changes not mount level
>>>> changes.
>>>>
>>>> In general remount is not safe for unprivielged users.
>>>>
>>>> Do you have a use case where you need to remount a filesystem?
>>>
>>> As we can do a umount+mount，I don't see why remount operation is not allowed.
>>> Shouldn't we add checks in remount path in the specific filesystem to ensure
>>> safety instead when we enable unprivilleged mount?
>>
>> But the thing is remount != mount+umount. Remount is change lowlevel
>> filesystem options.
>>
>> The basic danger is if someone in the primary user namespace mounted a
>> filesystem, and then we cloned that filesystem.
>>
>> umounting filesystems is ok. There reference count will drop or they
>> will just unmount if the ref count goes to zero.
>>
>> However mount -o remount -r /home could very easily remount everyone's
>> home directory in all mount namespaces read-only by making the
>> filesystem itself readonly.
>>
>> That danger applies even to some extent even if the options are safe for
>> us to perform at the filesystem level.
>>
>> Now that doesn't mean remount is a hopeless operation. What it does
>> mean is that we need to be very carefully with enabling remounting
>> of a filesystem.
>>
> Hi Eric
>
> what's you idea about the patch below.
> Maybe it better to add a new fs_flags FS_USERNS_REMOUNT?
> It's not a good experience that remount is disabled in container.
I think you totally did not read and understand what I said above,
and your patch description is wrong.
Remounting proc is not safe, making the patch below is broken. Read
proc and read my comments above.
Thinking that this might be an issue that can be fixed with fs_flags is
thinking about this wrong. Thinking a remount in any way would be
isolated to a single mount namespaces is wrong. When implementing
remount you have to think about everyone who has access to that
filesystem, and is is legitimate for you to affect all of them.
What are people doing with remounting? Why do we want it? As a
practical matter, assume that it is impossible to resue the current
filesystem level implementation of remount.
As I said above I don't even think mount -o remount -r /some/filesystem
is safe. Think what would happen if you run the shell script below with
/tmp being a separate tmpfs mount point and /proc being procfs.
---
#!/bin/sh
export IFIFO=/tmp/pidns-test-$$-in
export OFIFO=/tmp/pidns-test-$$-out
rm -f $IFIFO $OFIFO
mkfifo $IFIFO
mkfifo $OFIFO
unshare --user -- /bin/bash -s <<'EOF' &
echo waiting-for-uid-and-gid-maps > $OFIFO
read LINE < $IFIFO
exec unshare --mount -- /bin/bash -s <<'EOF2'
mount -n -o remount -ro /tmp
mount -n -o remount /proc hidpide=2
EOF2
EOF
child=$!
read LINE < $OFIFO
uid=$(id --user)
gid=$(id --group)
echo "0 $uid 1" > /proc/$child/uid_map
echo "0 $gid 1" > /proc/$child/gid_map
echo uid-and-gid-maps > $IFIFO
wait $child
---
> Thanks!
>
> From 8c5a01c007d72c748018665d3bd27cd2bde52c0f Mon Sep 17 00:00:00 2001
> From: Gao feng <gaofeng@xxxxxxxxxxxxxx>
> Date: Thu, 17 Jan 2013 14:41:00 +0800
> Subject: [PATCH] userns: allow remount filesystem in un-init userns
>
> The proc and sysfs filesystem already enable userns support,
> remounting these filesystems in un-init userns do no harm
> to the host.
>
> Signed-off-by: Gao feng <gaofeng@xxxxxxxxxxxxxx>
> ---
> fs/namespace.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 55605c5..b9d83fb 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1748,7 +1748,10 @@ static int do_remount(struct path *path, int flags, int mnt_flags,
> struct super_block *sb = path->mnt->mnt_sb;
> struct mount *mnt = real_mount(path->mnt);
>
> - if (!capable(CAP_SYS_ADMIN))
> + if (sb->s_type->fs_flags & FS_USERNS_MOUNT) {
> + if (!ns_capable(mnt->mnt_ns->user_ns, CAP_SYS_ADMIN))
> + return -EPERM;
> + } else if (!capable(CAP_SYS_ADMIN))
> return -EPERM;
>
> if (!check_mnt(mnt))
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/containers
