Split the code for remounting mount points and mounting paths. #1222

justincormack · 2016-12-11T02:29:44Z

A remount of a mount point must include all the current flags or
these will be cleared:

The mountflags and data arguments should match the values used in the
original mount() call, except for those parameters that are being
deliberately changed.

The current code does not do this; the bug manifests in the specified
flags for /dev being lost on remount read only at present. As we
need to specify flags, split the code path for this from remounting
paths which are not mount points, as these can only inherit the
existing flags of the path, and these cannot be changed.

In the bind case, remove extra flags from the bind remount. A bind
mount can only be remounted read only, no other flags can be set,
all other flags are inherited from the parent. From the man page:

Since Linux 2.6.26, this flag can also be used to make an existing
bind mount read-only by specifying mountflags as:

MS_REMOUNT | MS_BIND | MS_RDONLY

Note that only the MS_RDONLY setting of the bind mount can be changed
in this manner.

MS_REC can only be set on the original bind, so move this. See note
in man page on bind mounts:

The remaining bits in the mountflags argument are also ignored, with
the exception of MS_REC.

Signed-off-by: Justin Cormack justin.cormack@docker.com

justincormack · 2016-12-16T01:29:33Z

Fixed the CI issue, sorry about that.

cyphar · 2016-12-16T02:50:25Z

Sorry for not looking into this earlier, I will take a look later today.

justincormack · 2016-12-16T08:20:48Z

@cyphar no problem, thanks.

cyphar · 2016-12-16T12:22:17Z

libcontainer/rootfs_linux.go

@@ -121,7 +121,7 @@ func finalizeRootfs(config *configs.Config) (err error) {
 	for _, m := range config.Mounts {
 		if libcontainerUtils.CleanPath(m.Destination) == "/dev" {
 			if m.Flags&syscall.MS_RDONLY != 0 {


While you're changing this can you also switch this to be:

if m.Flags&syscall.MS_RDONLY == syscall.MS_RDONLY {

cyphar · 2016-12-16T13:19:02Z

libcontainer/rootfs_linux.go

+		}
+		return err
+	}
+	return syscall.Mount(path, path, "", syscall.MS_BIND|syscall.MS_REMOUNT|syscall.MS_RDONLY|syscall.MS_REC, "")


I'm not sure we should've dropped the |defaultMountFlags from here.

I specifically mention that in the commit message, bind mounts do not accept arbitrary mount flags, you can only set RDONLY. All other mount flags are inherited from the underlying mount and cannot be changed (because it is actually the same mount and hence same kernel datastructure). Thats one reason I had to split the code as the remount case is different...

So why is it not included in the MS_BIND command (the first one)?

Yeah, good point. From my quick testing, it looks like doing it one shot should work.

@justincormack What I mean is why not make the first Mount line like this:

if err := syscall.Mount(path, path, "", syscall.MS_BIND|syscall.MS_REC|defaultMountFlags, ""); err != nil {

I accidentally commented on the wrong line.

Okay, but I just tried to add nosuid to a bind mount. You're right that it doesn't work with the initial mount (sorry for misunderstanding what you said in that regard), but it does work with the remount (so we could apply them in the second mount):

% sudo mount --bind -o nosuid /usr/bin bin % mount | grep /home/cyphar/bin /dev/mapper/system-root on /home/cyphar/bin type btrfs (rw,relatime,space_cache,subvolid=259,subvol=/@/.snapshots/1/snapshot/usr/bin) % sudo mount -o remount,nosuid bin bin % mount | grep /home/cyphar/bin /dev/mapper/system-root on /home/cyphar/bin type btrfs (rw,nosuid,relatime,space_cache,subvolid=259,subvol=/@/.snapshots/1/snapshot/usr/bin) % ./bin/sudo sudo: effective uid is not 0, is ./bin/sudo on a file system with the 'nosuid' option set or an NFS file system without root privileges?

The reason I'm harping on this is because defaultMountFlags contains security-related flags and I want to make sure that there's a good reason that we aren't going to be applying them for bindmounts anymore (though it looks like we never applied them).

Hmm. But in this case if you wanted this path nosuid you would have mounted it like that in the config? Having the read-only masked part only being nosuid but not the read-write part seems weird.

But in this case if you wanted this path nosuid you would have mounted it like that in the config?

Yeah, by my concern is with paths marked as readonlyPaths, which don't have an entry in the config. The spec just mentions that the path needs to be readonly, so this is probably fine. I was just concerned about suddenly allowing something that wasn't intended to be allowed previously.

But there will always be an entry for the mountpoint above, where the flags can be set. I don;t think anyone should rely on read only also setting other flags, and if they are set on the writeable part they are far more dangerous.

rhatdan · 2016-12-16T14:00:52Z

@rhvgoyal PTAL

A remount of a mount point must include all the current flags or these will be cleared: ``` The mountflags and data arguments should match the values used in the original mount() call, except for those parameters that are being deliberately changed. ``` The current code does not do this; the bug manifests in the specified flags for `/dev` being lost on remount read only at present. As we need to specify flags, split the code path for this from remounting paths which are not mount points, as these can only inherit the existing flags of the path, and these cannot be changed. In the bind case, remove extra flags from the bind remount. A bind mount can only be remounted read only, no other flags can be set, all other flags are inherited from the parent. From the man page: ``` Since Linux 2.6.26, this flag can also be used to make an existing bind mount read-only by specifying mountflags as: MS_REMOUNT | MS_BIND | MS_RDONLY Note that only the MS_RDONLY setting of the bind mount can be changed in this manner. ``` MS_REC can only be set on the original bind, so move this. See note in man page on bind mounts: ``` The remaining bits in the mountflags argument are also ignored, with the exception of MS_REC. ``` Signed-off-by: Justin Cormack <justin.cormack@docker.com>

cyphar · 2016-12-25T00:08:14Z

LGTM

hqhq · 2016-12-27T07:24:46Z

LGTM

justincormack mentioned this pull request Dec 11, 2016

mount flags on /dev are lost when remounting read only #1223

Closed

justincormack force-pushed the remount-fixes branch 3 times, most recently from 7133779 to 6b664ea Compare December 16, 2016 01:20

cyphar mentioned this pull request Dec 16, 2016

Wierd issue with User Namespace and mounting volumes. #1229

Open

cyphar reviewed Dec 16, 2016

View reviewed changes

justincormack force-pushed the remount-fixes branch from 6b664ea to 990e49b Compare December 16, 2016 21:37

justincormack force-pushed the remount-fixes branch from 990e49b to 50acb55 Compare December 16, 2016 22:01

hqhq merged commit f376b80 into opencontainers:master Dec 27, 2016

justincormack deleted the remount-fixes branch December 29, 2016 10:44

cyphar mentioned this pull request Jan 2, 2017

root.readOnly: true does not work on filesystems mounted with nodev or nosuid #1247

Open

cyphar mentioned this pull request Jul 17, 2017

bind mounting /etc/resolv.conf fails #1523

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split the code for remounting mount points and mounting paths. #1222

Split the code for remounting mount points and mounting paths. #1222

justincormack commented Dec 11, 2016 •

edited

Loading

justincormack commented Dec 16, 2016

cyphar commented Dec 16, 2016

justincormack commented Dec 16, 2016

cyphar Dec 16, 2016

justincormack Dec 16, 2016

cyphar Dec 16, 2016

justincormack Dec 16, 2016 •

edited

Loading

cyphar Dec 16, 2016

mrunalp Dec 19, 2016

cyphar Dec 19, 2016

cyphar Dec 20, 2016 •

edited

Loading

justincormack Dec 20, 2016

cyphar Dec 21, 2016

justincormack Dec 24, 2016

cyphar Dec 25, 2016

rhatdan commented Dec 16, 2016

cyphar commented Dec 25, 2016 •

edited by caniszczyk

Loading

hqhq commented Dec 27, 2016 •

edited by caniszczyk

Loading

Split the code for remounting mount points and mounting paths. #1222

Split the code for remounting mount points and mounting paths. #1222

Conversation

justincormack commented Dec 11, 2016 • edited Loading

justincormack commented Dec 16, 2016

cyphar commented Dec 16, 2016

justincormack commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justincormack Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyphar Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Dec 16, 2016

cyphar commented Dec 25, 2016 • edited by caniszczyk Loading

hqhq commented Dec 27, 2016 • edited by caniszczyk Loading

justincormack commented Dec 11, 2016 •

edited

Loading

justincormack Dec 16, 2016 •

edited

Loading

cyphar Dec 20, 2016 •

edited

Loading

cyphar commented Dec 25, 2016 •

edited by caniszczyk

Loading

hqhq commented Dec 27, 2016 •

edited by caniszczyk

Loading