Fix -K check.
The -K check is not currently very useful, and was incomplete:
|skip_remount| was being set to 1 even in the cases where the remount
was happening with different flags (and therefore not being skipped.)
Fix this by renaming the |skip_remount| variable to |change_remount|
so that it better reflects what we want to test, and adding a clearer
error message (with a suggestion to explicitly add '-v'.)
Also fix some long lines and a comment.
Bug: chromium:1008895
Test: Unit tests pass.
Test: minijail0 -p -K -- /bin/id prints error.
Test: minijail0 -p -Kslave -- /bin/id prints error.
Test: minijail0 -pv -K -- /bin/id runs successfully.
Test: minijail0 -pv -Kslave -- /bin/id runs successfully.
Change-Id: I19b4bf4f495bd5ec40338f2502854d4054be4c5c
diff --git a/libminijail.c b/libminijail.c
index 29f6ef4..cd6ffe7 100644
--- a/libminijail.c
+++ b/libminijail.c
@@ -1794,14 +1794,17 @@
if (j->uidmap && write_proc_file(j->initpid, j->uidmap, "uid_map") != 0)
kill_child_and_die(j, "failed to write uid_map");
if (j->gidmap && j->flags.disable_setgroups) {
- /* Older kernels might not have the /proc/<pid>/setgroups files. */
+ /*
+ * Older kernels might not have the /proc/<pid>/setgroups files.
+ */
int ret = write_proc_file(j->initpid, "deny", "setgroups");
if (ret != 0) {
if (ret == -ENOENT) {
/* See http://man7.org/linux/man-pages/man7/user_namespaces.7.html. */
warn("could not disable setgroups(2)");
} else
- kill_child_and_die(j, "failed to disable setgroups(2)");
+ kill_child_and_die(
+ j, "failed to disable setgroups(2)");
}
}
if (j->gidmap && write_proc_file(j->initpid, j->gidmap, "gid_map") != 0)
@@ -2161,12 +2164,15 @@
pdie("unshare(CLONE_NEWNS) failed");
/*
* By default, remount all filesystems as private, unless
- * - Passed a specific remount mode, in which case remount with that,
- * - Asked not to remount at all, in which case skip the mount(2) call.
+ * - Passed a specific remount mode, in which case remount with
+ * that,
+ * - Asked not to remount at all, in which case skip the
+ * mount(2) call.
* https://www.kernel.org/doc/Documentation/filesystems/sharedsubtree.txt
*/
if (j->remount_mode) {
- if (mount(NULL, "/", NULL, MS_REC | j->remount_mode, NULL))
+ if (mount(NULL, "/", NULL, MS_REC | j->remount_mode,
+ NULL))
pdie("mount(NULL, /, NULL, MS_REC | MS_PRIVATE,"
" NULL) failed");
}
diff --git a/minijail0_cli.c b/minijail0_cli.c
index c1d053e..277c222 100644
--- a/minijail0_cli.c
+++ b/minijail0_cli.c
@@ -582,7 +582,7 @@
int forward = 1;
int binding = 0;
int chroot = 0, pivot_root = 0;
- int mount_ns = 0, skip_remount = 0;
+ int mount_ns = 0, change_remount = 0;
int inherit_suppl_gids = 0, keep_suppl_gids = 0;
int caps = 0, ambient_caps = 0;
int seccomp = -1;
@@ -681,11 +681,12 @@
add_mount(j, optarg);
break;
case 'K':
- if (optarg)
+ if (optarg) {
set_remount_mode(j, optarg);
- else
+ } else {
minijail_skip_remount_private(j);
- skip_remount = 1;
+ }
+ change_remount = 1;
break;
case 'P':
use_pivot_root(j, optarg, &pivot_root, chroot);
@@ -909,12 +910,14 @@
}
/*
- * Remounting / as MS_PRIVATE only happens when entering a new mount
- * namespace, so skipping it only applies in that case.
+ * / is only remounted when entering a new mount namespace, so unless
+ * that's set there is no need for the -K/-K<mode> flags.
*/
- if (skip_remount && !mount_ns) {
- fprintf(stderr, "Can't skip marking mounts as MS_PRIVATE"
- " without mount namespaces.\n");
+ if (change_remount && !mount_ns) {
+ fprintf(stderr, "No need to use -K (skip remounting '/') or "
+ "-K<mode> (remount '/' as <mode>)\n"
+ "without -v (new mount namespace).\n"
+ "Do you need to add '-v' explicitly?\n");
exit(1);
}
diff --git a/system_unittest.cc b/system_unittest.cc
index 470421d..4156e6e 100644
--- a/system_unittest.cc
+++ b/system_unittest.cc
@@ -260,7 +260,7 @@
EXPECT_EQ(0, setup_mount_destination(nullptr, "/dev", 0, 0, false, nullptr));
}
-// Mount flags should be obtained for bind-mounts
+// Mount flags should be obtained for bind-mounts.
TEST(setup_mount_destination, mount_flags) {
struct statvfs stvfs_buf;
ASSERT_EQ(0, statvfs("/proc", &stvfs_buf));
