Fix statvfs() call on non-existent directories.
https://android-review.googlesource.com/c/platform/external/minijail/+/978609
attempted to fix the case of failing to obtain mount flags from mount
destinations that already existed, but in the process created (or
exposed) a bug. Parts of the code would assume that relative source
paths would refer to pseudo filesystems (like "proc" or "devpts"), but
other parts of the code would assume they referred to valid relative
paths and, for example, call statvfs(2) on them, which would fail
and incorrectly fail the entire function.
Fix by only trying to obtain mount flags when the source is *not*
assumed to be a pseudo fs, and reorganize the code to only have to do
this check once.
Bug: crbug.com/985467
Test: Unit tests pass.
Test: Deploy on Chrome OS, integration tests pass.
Change-Id: I328cdc8b2cefad7a773b599d29191c404ebc54f0
diff --git a/system.c b/system.c
index 2e5c232..06d9c57 100644
--- a/system.c
+++ b/system.c
@@ -340,7 +340,7 @@
rc = statvfs(path, &stvfs_buf);
if (rc) {
rc = errno;
- pwarn("failed to look up mount flags: source=%s", path);
+ pwarn("statvfs('%s') failed", path);
return -rc;
}
*mnt_flags = vfs_flags_to_mount_flags(stvfs_buf.f_flag);
@@ -356,82 +356,104 @@
{
int rc;
struct stat st_buf;
- bool domkdir;
+ bool do_mkdir = false;
+ bool is_abspath = source && source[0] == '/';
+ /* Assume relative |source| paths are pseudo filesystems. */
+ bool is_pseudofs = !is_abspath;
rc = stat(dest, &st_buf);
- if (rc == 0) /* destination exists */
- return get_mount_flags_for_directory(source, mnt_flags);
+ if (rc) {
+ /*
+ * |dest| does not exist. Try to create it.
+ * Either make a directory or touch a file depending on the
+ * source type.
+ *
+ * If |source| isn't an absolute path, assume it is a filesystem
+ * type such as "tmpfs" and create a directory to mount it on.
+ * |dest| will be something like "none" or "proc" which we
+ * shouldn't be checking.
+ */
+ if (is_abspath) {
+ /* |source| is an absolute path -- it better exist! */
+ rc = stat(source, &st_buf);
+ if (rc) {
+ rc = errno;
+ pwarn("stat('%s') failed", source);
+ return -rc;
+ }
- /*
- * Try to create the destination.
- * Either make a directory or touch a file depending on the source type.
- *
- * If the source isn't an absolute path, assume it is a filesystem type
- * such as "tmpfs" and create a directory to mount it on. The dest will
- * be something like "none" or "proc" which we shouldn't be checking.
- */
- if (source[0] == '/') {
- /* The source is an absolute path -- it better exist! */
- rc = stat(source, &st_buf);
- if (rc) {
- rc = errno;
- pwarn("stat(%s) failed", source);
- return -rc;
+ /*
+ * If bind mounting, we only create a directory if the
+ * source is a directory, else we always bind mount it
+ * as a file to support device nodes, sockets, etc...
+ *
+ * For all other mounts, we assume a block/char source
+ * is going to want a directory to mount to. If the
+ * source is something else (e.g. a fifo or socket),
+ * this probably will not do the right thing, but we'll
+ * fail later on when we try to mount(), so shouldn't be
+ * a big deal.
+ */
+ do_mkdir = S_ISDIR(st_buf.st_mode) ||
+ (!bind && (S_ISBLK(st_buf.st_mode) ||
+ S_ISCHR(st_buf.st_mode)));
+
+ } else {
+ /*
+ * |source| is a relative path -- assume it's a pseudo
+ * fs.
+ */
+
+ /* Disallow relative bind mounts. */
+ if (bind) {
+ warn("relative bind-mounts are not allowed: "
+ "source=%s",
+ source);
+ return -EINVAL;
+ }
+
+ do_mkdir = true;
}
/*
- * If bind mounting, we only create a directory if the source
- * is a directory, else we always bind mount it as a file to
- * support device nodes, sockets, etc...
- *
- * For all other mounts, we assume a block/char source is
- * going to want a directory to mount to. If the source is
- * something else (e.g. a fifo or socket), this probably will
- * not do the right thing, but we'll fail later on when we try
- * to mount(), so shouldn't be a big deal.
+ * Now that we know what we want to do, do it!
+ * We always create the intermediate dirs and the final path
+ * with 0755 perms and root/root ownership. This shouldn't be a
+ * problem because the actual mount will set those
+ * perms/ownership on the mount point which is all people should
+ * need to access it.
*/
- domkdir = S_ISDIR(st_buf.st_mode) ||
- (!bind && (S_ISBLK(st_buf.st_mode) ||
- S_ISCHR(st_buf.st_mode)));
-
- } else {
- /* The source is a relative path -- assume it's a pseudo fs. */
-
- /* Disallow relative bind mounts. */
- if (bind) {
- warn("relative bind-mounts are not allowed: source=%s",
- source);
- return -EINVAL;
+ rc = mkdir_p(dest, 0755, do_mkdir);
+ if (rc)
+ return rc;
+ if (!do_mkdir) {
+ int fd = open(dest, O_RDWR | O_CREAT | O_CLOEXEC, 0700);
+ if (fd < 0) {
+ rc = errno;
+ pwarn("open('%s') failed", dest);
+ return -rc;
+ }
+ close(fd);
}
-
- domkdir = true;
+ if (chown(dest, uid, gid)) {
+ rc = errno;
+ pwarn("chown('%s', %u, %u) failed", dest, uid, gid);
+ return -rc;
+ }
}
/*
- * Now that we know what we want to do, do it!
- * We always create the intermediate dirs and the final path with 0755
- * perms and root/root ownership. This shouldn't be a problem because
- * the actual mount will set those perms/ownership on the mount point
- * which is all people should need to access it.
+ * At this point, either because it already existed or because it was
+ * created above, |dest| exists.
*/
- rc = mkdir_p(dest, 0755, domkdir);
- if (rc)
- return rc;
- if (!domkdir) {
- int fd = open(dest, O_RDWR | O_CREAT | O_CLOEXEC, 0700);
- if (fd < 0) {
- rc = errno;
- pwarn("open(%s) failed", dest);
- return -rc;
- }
- close(fd);
+ if (is_pseudofs) {
+ /* If |source| is a pseudo fs, it will have no mount flags. */
+ if (mnt_flags)
+ *mnt_flags = 0;
+ return 0;
+ } else {
+ return get_mount_flags_for_directory(source, mnt_flags);
}
- if (chown(dest, uid, gid)) {
- rc = errno;
- pwarn("chown(%s, %u, %u) failed", dest, uid, gid);
- return -rc;
- }
- return get_mount_flags_for_directory(source, mnt_flags);
}
/*
