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.

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

Allow bind mounts of nodev,nosuid,noexec filesystems #3805

Merged
merged 1 commit into from
Jul 29, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions contrib/completions/bash/runc
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,7 @@ _runc_run() {
--no-subreaper
--no-pivot
--no-new-keyring
--no-mount-fallback
"

local options_with_args="
Expand Down Expand Up @@ -567,6 +568,7 @@ _runc_create() {
--help
--no-pivot
--no-new-keyring
--no-mount-fallback
"

local options_with_args="
Expand Down Expand Up @@ -627,6 +629,7 @@ _runc_restore() {
--no-pivot
--auto-dedup
--lazy-pages
--no-mount-fallback
"

local options_with_args="
Expand Down
4 changes: 4 additions & 0 deletions create.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ command(s) that get executed on start, edit the args parameter of the spec. See
Name: "preserve-fds",
Usage: "Pass N additional file descriptors to the container (stdio + $LISTEN_FDS + N in total)",
},
cli.BoolFlag{
Name: "no-mount-fallback",
Usage: "Do not fallback when the specific configuration is not applicable (e.g., do not try to remount a bind mount again after the first attempt failed on source filesystems that have nodev, noexec, nosuid, noatime, relatime, strictatime, nodiratime set)",
},
},
Action: func(context *cli.Context) error {
if err := checkArgs(context, 1, exactArgs); err != nil {
Expand Down
4 changes: 4 additions & 0 deletions libcontainer/configs/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,10 @@ type Config struct {
// RootlessCgroups is set when unlikely to have the full access to cgroups.
// When RootlessCgroups is set, cgroups errors are ignored.
RootlessCgroups bool `json:"rootless_cgroups,omitempty"`

// Do not try to remount a bind mount again after the first attempt failed on source
// filesystems that have nodev, noexec, nosuid, noatime, relatime, strictatime, nodiratime set
NoMountFallback bool `json:"no_mount_fallback,omitempty"`
}

type (
Expand Down
23 changes: 17 additions & 6 deletions libcontainer/rootfs_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ type mountConfig struct {
cgroup2Path string
rootlessCgroups bool
cgroupns bool
noMountFallback bool
}

// mountEntry contains mount data specific to a mount point.
Expand Down Expand Up @@ -83,6 +84,7 @@ func prepareRootfs(pipe io.ReadWriter, iConfig *initConfig, mountFds mountFds) (
cgroup2Path: iConfig.Cgroup2Path,
rootlessCgroups: iConfig.RootlessCgroups,
cgroupns: config.Namespaces.Contains(configs.NEWCGROUP),
noMountFallback: config.NoMountFallback,
}
for i, m := range config.Mounts {
entry := mountEntry{Mount: m}
Expand Down Expand Up @@ -512,7 +514,7 @@ func mountToRootfs(c *mountConfig, m mountEntry) error {
// first check that we have non-default options required before attempting a remount
if m.Flags&^(unix.MS_REC|unix.MS_REMOUNT|unix.MS_BIND) != 0 {
// only remount if unique mount options are set
if err := remount(m, rootfs); err != nil {
if err := remount(m, rootfs, c.noMountFallback); err != nil {
return err
}
}
Expand Down Expand Up @@ -1101,24 +1103,33 @@ func writeSystemProperty(key, value string) error {
return os.WriteFile(path.Join("/proc/sys", keyPath), []byte(value), 0o644)
}

func remount(m mountEntry, rootfs string) error {
func remount(m mountEntry, rootfs string, noMountFallback bool) error {
return utils.WithProcfd(rootfs, m.Destination, func(dstFD string) error {
flags := uintptr(m.Flags | unix.MS_REMOUNT)
err := mountViaFDs(m.Source, m.srcFD, m.Destination, dstFD, m.Device, flags, "")
if err == nil {
return nil
}
// Check if the source has ro flag...
// Check if the source has flags set according to noMountFallback
src := m.src()
var s unix.Statfs_t
if err := unix.Statfs(src, &s); err != nil {
return &os.PathError{Op: "statfs", Path: src, Err: err}
}
if s.Flags&unix.MS_RDONLY != unix.MS_RDONLY {
var checkflags int
if noMountFallback {
// Check for ro only
checkflags = unix.MS_RDONLY
} else {
// Check for ro, nodev, noexec, nosuid, noatime, relatime, strictatime,
// nodiratime
checkflags = unix.MS_RDONLY | unix.MS_NODEV | unix.MS_NOEXEC | unix.MS_NOSUID | unix.MS_NOATIME | unix.MS_RELATIME | unix.MS_STRICTATIME | unix.MS_NODIRATIME
}
if int(s.Flags)&checkflags == 0 {
return err
}
// ... and retry the mount with ro flag set.
flags |= unix.MS_RDONLY
// ... and retry the mount with flags found above.
flags |= uintptr(int(s.Flags) & checkflags)
return mountViaFDs(m.Source, m.srcFD, m.Destination, dstFD, m.Device, flags, "")
})
}
Expand Down
2 changes: 2 additions & 0 deletions libcontainer/specconv/spec_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@ type CreateOpts struct {
Spec *specs.Spec
RootlessEUID bool
RootlessCgroups bool
NoMountFallback bool
}

// getwd is a wrapper similar to os.Getwd, except it always gets
Expand Down Expand Up @@ -358,6 +359,7 @@ func CreateLibcontainerConfig(opts *CreateOpts) (*configs.Config, error) {
NoNewKeyring: opts.NoNewKeyring,
RootlessEUID: opts.RootlessEUID,
RootlessCgroups: opts.RootlessCgroups,
NoMountFallback: opts.NoMountFallback,
}

for _, m := range spec.Mounts {
Expand Down
4 changes: 4 additions & 0 deletions restore.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,10 @@ using the runc checkpoint command.`,
Value: "",
Usage: "Specify an LSM mount context to be used during restore.",
},
cli.BoolFlag{
Name: "no-mount-fallback",
Usage: "Do not fallback when the specific configuration is not applicable (e.g., do not try to remount a bind mount again after the first attempt failed on source filesystems that have nodev, noexec, nosuid, noatime, relatime, strictatime, nodiratime set)",
},
},
Action: func(context *cli.Context) error {
if err := checkArgs(context, 1, exactArgs); err != nil {
Expand Down
4 changes: 4 additions & 0 deletions run.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,10 @@ command(s) that get executed on start, edit the args parameter of the spec. See
Name: "preserve-fds",
Usage: "Pass N additional file descriptors to the container (stdio + $LISTEN_FDS + N in total)",
},
cli.BoolFlag{
Name: "no-mount-fallback",
Usage: "Do not fallback when the specific configuration is not applicable (e.g., do not try to remount a bind mount again after the first attempt failed on source filesystems that have nodev, noexec, nosuid, noatime, relatime, strictatime, nodiratime set)",
},
},
Action: func(context *cli.Context) error {
if err := checkArgs(context, 1, exactArgs); err != nil {
Expand Down
91 changes: 80 additions & 11 deletions tests/integration/mounts_sshfs.bats
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,20 @@
load helpers

function setup() {
# Create a ro fuse-sshfs mount; skip the test if it's not working.
setup_busybox
update_config '.process.args = ["/bin/echo", "Hello World"]'
}

function teardown() {
# Some distros do not have fusermount installed
# as a dependency of fuse-sshfs, and good ol' umount works.
fusermount -u "$DIR" || umount "$DIR"

teardown_bundle
}

function setup_sshfs() {
# Create a fuse-sshfs mount; skip the test if it's not working.
local sshfs="sshfs
-o UserKnownHostsFile=/dev/null
-o StrictHostKeyChecking=no
Expand All @@ -12,30 +25,86 @@ function setup() {
DIR="$BATS_RUN_TMPDIR/fuse-sshfs"
mkdir -p "$DIR"

if ! $sshfs -o ro rootless@localhost: "$DIR"; then
if ! $sshfs -o "$1" rootless@localhost: "$DIR"; then
skip "test requires working sshfs mounts"
fi
}

setup_busybox
update_config '.process.args = ["/bin/echo", "Hello World"]'
@test "runc run [rw bind mount of a ro fuse sshfs mount]" {
setup_sshfs "ro"
update_config ' .mounts += [{
type: "bind",
source: "'"$DIR"'",
destination: "/mnt",
options: ["rw", "rprivate", "nosuid", "nodev", "rbind"]
}]'

Copy link
Member

Choose a reason for hiding this comment

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

The original test has to be kept

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved out my check to a new bats test file (b7bb49b). Furthermore I reviewed the test again and I could simplify it and make it more like the existing test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kolyshkin: Like now?

Yes, thanks!

If you can rebase one more time, CI might even become green again :)

One more rebase done. After the rebase is before the next one :-)

Also, a nit in the first commit message: you have "roset" without a space, and an extra space before period.

Fixed commit message.

runc run --no-mount-fallback test_busybox
[ "$status" -eq 0 ]
}

function teardown() {
# New distros (Fedora 35) do not have fusermount installed
# as a dependency of fuse-sshfs, and good ol' umount works.
fusermount -u "$DIR" || umount "$DIR"
@test "runc run [dev,exec,suid,atime bind mount of a nodev,nosuid,noexec,noatime fuse sshfs mount]" {
setup_sshfs "nodev,nosuid,noexec,noatime"
# The "sync" option is used to trigger a remount with the below options.
# It serves no further purpose. Otherwise only a bind mount without
# applying the below options will be done.
update_config ' .mounts += [{
type: "bind",
source: "'"$DIR"'",
destination: "/mnt",
options: ["dev", "suid", "exec", "atime", "rprivate", "rbind", "sync"]
}]'

teardown_bundle
runc run test_busybox
[ "$status" -eq 0 ]
}

@test "runc run [rw bind mount of a ro fuse sshfs mount]" {
@test "runc run [ro bind mount of a nodev,nosuid,noexec,noatime fuse sshfs mount]" {
setup_sshfs "nodev,nosuid,noexec,noatime"
update_config ' .mounts += [{
type: "bind",
source: "'"$DIR"'",
destination: "/mnt",
options: ["rw", "rprivate", "nosuid", "nodev", "rbind"]
options: ["rbind", "ro"]
}]'

runc run test_busybox
[ "$status" -eq 0 ]
}

@test "runc run [dev,exec,suid,atime bind mount of a nodev,nosuid,noexec,noatime fuse sshfs mount without fallback]" {
setup_sshfs "nodev,nosuid,noexec,noatime"
# The "sync" option is used to trigger a remount with the below options.
# It serves no further purpose. Otherwise only a bind mount without
# applying the below options will be done.
update_config ' .mounts += [{
type: "bind",
source: "'"$DIR"'",
destination: "/mnt",
options: ["dev", "suid", "exec", "atime", "rprivate", "rbind", "sync"]
}]'

runc run --no-mount-fallback test_busybox
# The above will fail as we added --no-mount-fallback which causes us not to
# try to remount a bind mount again after the first attempt failed on source
# filesystems that have nodev, noexec, nosuid, noatime set.
[ "$status" -ne 0 ]
Copy link
Member

@AkihiroSuda AkihiroSuda Jun 5, 2023

Choose a reason for hiding this comment

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

Could you add a comment line to explain how/why this fails?
I also suggest checking the stderr content.

[[ "$output" == *"runc run failed: unable to start container process: error during container init: error mounting"*"operation not permitted"* ]]
}

@test "runc run [ro bind mount of a nodev,nosuid,noexec,noatime fuse sshfs mount without fallback]" {
setup_sshfs "nodev,nosuid,noexec,noatime"
update_config ' .mounts += [{
type: "bind",
source: "'"$DIR"'",
destination: "/mnt",
options: ["rbind", "ro"]
}]'

runc run --no-mount-fallback test_busybox
# The above will fail as we added --no-mount-fallback which causes us not to
# try to remount a bind mount again after the first attempt failed on source
# filesystems that have nodev, noexec, nosuid, noatime set.
[ "$status" -ne 0 ]
[[ "$output" == *"runc run failed: unable to start container process: error during container init: error mounting"*"operation not permitted"* ]]
}
1 change: 1 addition & 0 deletions utils_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ func createContainer(context *cli.Context, id string, spec *specs.Spec) (*libcon
Spec: spec,
RootlessEUID: os.Geteuid() != 0,
RootlessCgroups: rootlessCg,
NoMountFallback: context.Bool("no-mount-fallback"),
})
if err != nil {
return nil, err
Expand Down