Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Dec 27, 2016
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 21 additions & 12 deletions libcontainer/rootfs_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,8 @@ func finalizeRootfs(config *configs.Config) (err error) {
// remount dev as ro if specified
for _, m := range config.Mounts {
if libcontainerUtils.CleanPath(m.Destination) == "/dev" {
if m.Flags&syscall.MS_RDONLY != 0 {
if err := remountReadonly(m.Destination); err != nil {
if m.Flags&syscall.MS_RDONLY == syscall.MS_RDONLY {
if err := remountReadonly(m); err != nil {
return newSystemErrorWithCausef(err, "remounting %q as readonly", m.Destination)
}
}
Expand Down Expand Up @@ -700,17 +700,26 @@ func createIfNotExists(path string, isDir bool) error {
return nil
}

// remountReadonly will bind over the top of an existing path and ensure that it is read-only.
func remountReadonly(path string) error {
// readonlyPath will make a path read only.
func readonlyPath(path string) error {
if err := syscall.Mount(path, path, "", syscall.MS_BIND|syscall.MS_REC, ""); err != nil {
if os.IsNotExist(err) {
return nil
}
return err
}
return syscall.Mount(path, path, "", syscall.MS_BIND|syscall.MS_REMOUNT|syscall.MS_RDONLY|syscall.MS_REC, "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@justincormack justincormack Dec 16, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@cyphar cyphar Dec 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}

// remountReadonly will remount an existing mount point and ensure that it is read-only.
func remountReadonly(m *configs.Mount) error {
var (
dest = m.Destination
flags = m.Flags
)
for i := 0; i < 5; i++ {
if err := syscall.Mount("", path, "", syscall.MS_REMOUNT|syscall.MS_RDONLY, ""); err != nil && !os.IsNotExist(err) {
if err := syscall.Mount("", dest, "", uintptr(flags|syscall.MS_REMOUNT|syscall.MS_RDONLY), ""); err != nil {
switch err {
case syscall.EINVAL:
// Probably not a mountpoint, use bind-mount
if err := syscall.Mount(path, path, "", syscall.MS_BIND, ""); err != nil {
return err
}
return syscall.Mount(path, path, "", syscall.MS_BIND|syscall.MS_REMOUNT|syscall.MS_RDONLY|syscall.MS_REC|defaultMountFlags, "")
case syscall.EBUSY:
time.Sleep(100 * time.Millisecond)
continue
Expand All @@ -720,7 +729,7 @@ func remountReadonly(path string) error {
}
return nil
}
return fmt.Errorf("unable to mount %s as readonly max retries reached", path)
return fmt.Errorf("unable to mount %s as readonly max retries reached", dest)
}

// maskPath masks the top of the specified path inside a container to avoid
Expand Down
2 changes: 1 addition & 1 deletion libcontainer/standard_init_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func (l *linuxStandardInit) Init() error {
}
}
for _, path := range l.config.Config.ReadonlyPaths {
if err := remountReadonly(path); err != nil {
if err := readonlyPath(path); err != nil {
return err
}
}
Expand Down