Fix RO-remount logic for bindmounts
Using pivot-roots with bindmounts causes the kernel to keep some
mountflags of the source directory (nosuid, noexec, nodev) that have to
be specified during the RO-remount, otherwise the mount will fail with EPERM.
This was already previously covered by obtaining the source mount flags in
`setup_mount_destination`. This function failed to provide those flags if the
estination folder is already existing (mounting destination '/').
This commit moves the logic to determine the mountflags of a given
mountpoint into a dedicated function and properly handles vfs->mount
flag translation.
Test: All tests pass
Bug: crbug.com/971656
Change-Id: I7468b63e26fd43f45175ac54c952f726ff93a434
diff --git a/libminijail.c b/libminijail.c
index 6608cdd..d9b7da4 100644
--- a/libminijail.c
+++ b/libminijail.c
@@ -2800,7 +2800,7 @@
if (use_preload) {
free(oldenv_copy);
}
- die("failed to fork child");
+ pdie("failed to fork child");
}
if (child_pid) {
diff --git a/libminijail_unittest.cc b/libminijail_unittest.cc
index 9a04624..115b3ae 100644
--- a/libminijail_unittest.cc
+++ b/libminijail_unittest.cc
@@ -667,6 +667,38 @@
minijail_destroy(j);
}
+TEST_F(NamespaceTest, test_bind_ro_with_pivot_root) {
+ if (!userns_supported_) {
+ SUCCEED();
+ return;
+ }
+ struct minijail *j = minijail_new();
+ minijail_namespace_cgroups(j);
+ minijail_namespace_net(j);
+ minijail_namespace_pids(j);
+ minijail_namespace_user(j);
+ minijail_namespace_uts(j);
+ minijail_namespace_vfs(j);
+
+ minijail_use_caps(j, 0);
+ minijail_no_new_privs(j);
+ minijail_namespace_user_disable_setgroups(j);
+
+ minijail_run_as_init(j);
+ minijail_enter_pivot_root(j, "/tmp");
+
+ // /dev/shm is usually mounted nosuid,nodev.
+ minijail_bind(j, "/dev/shm", "/", false);
+
+ pid_t p = minijail_fork(j);
+ if (p) {
+ EXPECT_EQ(0, minijail_wait(j));
+ minijail_destroy(j);
+ return;
+ }
+ exit(0);
+}
+
TEST_F(NamespaceTest, test_namespaces) {
constexpr char teststr[] = "test\n";
diff --git a/system.c b/system.c
index 175e088..2e5c232 100644
--- a/system.c
+++ b/system.c
@@ -14,6 +14,7 @@
#include <stdio.h>
#include <string.h>
#include <sys/ioctl.h>
+#include <sys/mount.h>
#include <sys/prctl.h>
#include <sys/socket.h>
#include <sys/stat.h>
@@ -24,6 +25,35 @@
#include "util.h"
+/* Old libc versions might not define all constants that we need. */
+#ifndef ST_RDONLY
+#define ST_RDONLY 0x0001
+#endif
+#ifndef ST_NOSUID
+#define ST_NOSUID 0x0002
+#endif
+#ifndef ST_NODEV
+#define ST_NODEV 0x0004
+#endif
+#ifndef ST_NOEXEC
+#define ST_NOEXEC 0x0008
+#endif
+#ifndef ST_SYNCHRONOUS
+#define ST_SYNCHRONOUS 0x0010
+#endif
+#ifndef ST_MANDLOCK
+#define ST_MANDLOCK 0x0040
+#endif
+#ifndef ST_NOATIME
+#define ST_NOATIME 0x0400
+#endif
+#ifndef ST_NODIRATIME
+#define ST_NODIRATIME 0x0800
+#endif
+#ifndef ST_RELATIME
+#define ST_RELATIME 0x1000
+#endif
+
/*
* SECBIT_NO_CAP_AMBIENT_RAISE was added in kernel 4.3, so fill in the
* definition if the securebits header doesn't provide it.
@@ -296,6 +326,28 @@
}
/*
+ * get_mount_flags_for_directory: Returns the mount flags for the given
+ * directory.
+ */
+int get_mount_flags_for_directory(const char *path, unsigned long *mnt_flags)
+{
+ int rc;
+ struct statvfs stvfs_buf;
+
+ if (!mnt_flags)
+ return 0;
+
+ rc = statvfs(path, &stvfs_buf);
+ if (rc) {
+ rc = errno;
+ pwarn("failed to look up mount flags: source=%s", path);
+ return -rc;
+ }
+ *mnt_flags = vfs_flags_to_mount_flags(stvfs_buf.f_flag);
+ return 0;
+}
+
+/*
* setup_mount_destination: Ensures the mount target exists.
* Creates it if needed and possible.
*/
@@ -308,7 +360,7 @@
rc = stat(dest, &st_buf);
if (rc == 0) /* destination exists */
- return 0;
+ return get_mount_flags_for_directory(source, mnt_flags);
/*
* Try to create the destination.
@@ -342,19 +394,6 @@
(!bind && (S_ISBLK(st_buf.st_mode) ||
S_ISCHR(st_buf.st_mode)));
- /* If bind mounting, also grab the mount flags of the source. */
- if (bind && mnt_flags) {
- struct statvfs stvfs_buf;
- rc = statvfs(source, &stvfs_buf);
- if (rc) {
- rc = errno;
- pwarn(
- "failed to look up mount flags: source=%s",
- source);
- return -rc;
- }
- *mnt_flags = stvfs_buf.f_flag;
- }
} else {
/* The source is a relative path -- assume it's a pseudo fs. */
@@ -392,7 +431,35 @@
pwarn("chown(%s, %u, %u) failed", dest, uid, gid);
return -rc;
}
- return 0;
+ return get_mount_flags_for_directory(source, mnt_flags);
+}
+
+/*
+ * vfs_flags_to_mount_flags: Converts the given flags returned by statvfs to
+ * flags that can be used by mount().
+ */
+unsigned long vfs_flags_to_mount_flags(unsigned long vfs_flags)
+{
+ unsigned int i;
+ unsigned long mount_flags = 0;
+
+ static struct {
+ unsigned long mount_flag;
+ unsigned long vfs_flag;
+ } const flag_translation_table[] = {
+ {MS_NOSUID, ST_NOSUID}, {MS_NODEV, ST_NODEV},
+ {MS_NOEXEC, ST_NOEXEC}, {MS_SYNCHRONOUS, ST_SYNCHRONOUS},
+ {MS_MANDLOCK, ST_MANDLOCK}, {MS_NOATIME, ST_NOATIME},
+ {MS_NODIRATIME, ST_NODIRATIME}, {MS_RELATIME, ST_RELATIME},
+ };
+
+ for (i = 0; i < ARRAY_SIZE(flag_translation_table); i++) {
+ if (vfs_flags & flag_translation_table[i].vfs_flag) {
+ mount_flags |= flag_translation_table[i].mount_flag;
+ }
+ }
+
+ return mount_flags;
}
/*
diff --git a/system.h b/system.h
index 7369846..f332952 100644
--- a/system.h
+++ b/system.h
@@ -54,9 +54,13 @@
int mkdir_p(const char *path, mode_t mode, bool isdir);
+int get_mount_flags_for_directory(const char *path, unsigned long *mnt_flags);
+
int setup_mount_destination(const char *source, const char *dest, uid_t uid,
uid_t gid, bool bind, unsigned long *mnt_flags);
+unsigned long vfs_flags_to_mount_flags(unsigned long vfs_flags);
+
int lookup_user(const char *user, uid_t *uid, gid_t *gid);
int lookup_group(const char *group, gid_t *gid);
diff --git a/system_unittest.cc b/system_unittest.cc
index 470421d..07a343c 100644
--- a/system_unittest.cc
+++ b/system_unittest.cc
@@ -273,7 +273,7 @@
std::string proc = dir.path + "/proc";
EXPECT_EQ(0, setup_mount_destination("/proc", proc.c_str(), -1, -1, true,
&mount_flags));
- EXPECT_EQ(stvfs_buf.f_flag, mount_flags);
+ EXPECT_EQ(vfs_flags_to_mount_flags(stvfs_buf.f_flag), mount_flags);
EXPECT_EQ(0, rmdir(proc.c_str()));
// Same thing holds for children of a mount.
@@ -281,7 +281,7 @@
std::string proc_self = dir.path + "/proc_self";
EXPECT_EQ(0, setup_mount_destination("/proc/self", proc_self.c_str(), -1, -1,
true, &mount_flags));
- EXPECT_EQ(stvfs_buf.f_flag, mount_flags);
+ EXPECT_EQ(vfs_flags_to_mount_flags(stvfs_buf.f_flag), mount_flags);
EXPECT_EQ(0, rmdir(proc_self.c_str()));
}
