Add overlap handling to redirect_fds(). am: c718268549 am: b4848039ee am: 4b324e8442 am: 82c0f84acd

Original change: https://android-review.googlesource.com/c/platform/external/minijail/+/1675393

Change-Id: Icaa1ea83b3be83a7e092f5f14d6e182ca1807287

3 files changed, 122 insertions, 29 deletions

diff --git a/libminijail.c b/libminijail.c
index 582554a..e0d9d54 100644
--- a/libminijail.c
+++ b/libminijail.c
@@ -8,6 +8,7 @@
 #define _GNU_SOURCE
 
 #include <asm/unistd.h>
+#include <assert.h>
 #include <dirent.h>
 #include <errno.h>
 #include <fcntl.h>
@@ -26,6 +27,7 @@
 #include <sys/param.h>
 #include <sys/prctl.h>
 #include <sys/resource.h>
+#include <sys/select.h>
 #include <sys/stat.h>
 #include <sys/sysmacros.h>
 #include <sys/types.h>
@@ -2592,8 +2594,74 @@ static int close_open_fds(int *inheritable_fds, size_t size)
 	return 0;
 }
 
+/* Return true if the specified file descriptor is already open. */
+static int fd_is_open(int fd)
+{
+	return fcntl(fd, F_GETFD) != -1 || errno != EBADF;
+}
+
+static_assert(FD_SETSIZE >= MAX_PRESERVED_FDS * 2 - 1,
+	      "If true, ensure_no_fd_conflict will always find an unused fd.");
+
+/* If p->parent_fd will be used by a child_fd, move it to an unused fd. */
+static int ensure_no_fd_conflict(const fd_set* child_fds,
+				 struct preserved_fd* p)
+{
+	if (!FD_ISSET(p->parent_fd, child_fds)){
+		return 0;
+	}
+
+	/*
+	 * If no other parent_fd matches the child_fd then use it instead of a
+	 * temporary.
+	 */
+	int fd = p->child_fd;
+	if (fd_is_open(fd)) {
+		fd = FD_SETSIZE - 1;
+		while (FD_ISSET(fd, child_fds) || fd_is_open(fd)) {
+			--fd;
+			if (fd < 0) {
+				die("failed to find an unused fd");
+			}
+		}
+	}
+
+	int ret = dup2(p->parent_fd, fd);
+	/*
+	 * warn() opens a file descriptor so it needs to happen after dup2 to
+	 * avoid unintended side effects. This can be avoided by reordering the
+	 * mapping requests so that the source fds with overlap are mapped
+	 * first (unless there are cycles).
+	 */
+	warn("mapped fd overlap: moving %d to %d", p->parent_fd, fd);
+	if (ret == -1) {
+		return -1;
+	}
+
+	p->parent_fd = fd;
+	return 0;
+}
+
 static int redirect_fds(struct minijail *j)
 {
+	fd_set child_fds;
+	FD_ZERO(&child_fds);
+
+	/* Relocate parent_fds that would be replaced by a child_fd. */
+	for (size_t i = 0; i < j->preserved_fd_count; i++) {
+		int child_fd = j->preserved_fds[i].child_fd;
+		if (FD_ISSET(child_fd, &child_fds)) {
+			die("fd %d is mapped more than once", child_fd);
+		}
+
+		if (ensure_no_fd_conflict(&child_fds,
+					  &j->preserved_fds[i]) == -1) {
+			return -1;
+		}
+
+		FD_SET(child_fd, &child_fds);
+	}
+
 	for (size_t i = 0; i < j->preserved_fd_count; i++) {
 		if (j->preserved_fds[i].parent_fd ==
 		    j->preserved_fds[i].child_fd) {
@@ -2609,13 +2677,10 @@ static int redirect_fds(struct minijail *j)
 	 * fds that are *not* child fds.
 	 */
 	for (size_t i = 0; i < j->preserved_fd_count; i++) {
-		int closeable = true;
-		for (size_t i2 = 0; i2 < j->preserved_fd_count; i2++) {
-			closeable &= j->preserved_fds[i].parent_fd !=
-				     j->preserved_fds[i2].child_fd;
+		int parent_fd = j->preserved_fds[i].parent_fd;
+		if (!FD_ISSET(parent_fd, &child_fds)) {
+			close(parent_fd);
 		}
-		if (closeable)
-			close(j->preserved_fds[i].parent_fd);
 	}
 	return 0;
 }
diff --git a/rust/minijail/src/lib.rs b/rust/minijail/src/lib.rs
index 289668e..d4f5787 100644
--- a/rust/minijail/src/lib.rs
+++ b/rust/minijail/src/lib.rs
@@ -9,7 +9,7 @@ use std::fmt::{self, Display};
 use std::fs;
 use std::io;
 use std::os::raw::{c_char, c_ulong, c_ushort};
-use std::os::unix::io::{AsRawFd, RawFd};
+use std::os::unix::io::{AsRawFd, IntoRawFd, RawFd};
 use std::path::{Path, PathBuf};
 use std::ptr::{null, null_mut};
 
@@ -733,6 +733,10 @@ impl Minijail {
     /// could cause a lot of trouble if not handled carefully.  FDs 0, 1, and 2
     /// are overwritten with /dev/null FDs unless they are included in the
     /// inheritable_fds list.
+    ///
+    /// Also, any Rust objects that own fds may try to close them after the fork. If they belong
+    /// to a fd number that was mapped to, the mapped fd will be closed instead.
+    ///
     /// This Function may abort in the child on error because a partially
     /// entered jail isn't recoverable.
     pub unsafe fn fork(&self, inheritable_fds: Option<&[RawFd]>) -> Result<pid_t> {
@@ -787,6 +791,10 @@ impl Minijail {
         if ret < 0 {
             return Err(Error::ForkingMinijail(ret));
         }
+        if ret == 0 {
+            // Safe because dev_null was remapped.
+            dev_null.into_raw_fd();
+        }
         Ok(ret as pid_t)
     }
 
diff --git a/rust/minijail/tests/fork_remap.rs b/rust/minijail/tests/fork_remap.rs
index 8ace83b..6cf3415 100644
--- a/rust/minijail/tests/fork_remap.rs
+++ b/rust/minijail/tests/fork_remap.rs
@@ -10,40 +10,51 @@
 
 use std::fs::{read_link, File, OpenOptions};
 use std::io::{self, Read};
-use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
+use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd};
 use std::path::Path;
 
 use minijail::Minijail;
 
 const DEV_NULL: &str = "/dev/null";
 const DEV_ZERO: &str = "/dev/zero";
+const PROC_CMDLINE: &str = "/proc/self/cmdline";
 
-const DEST_FD1: RawFd = 7;
-const DEST_FD2: RawFd = 8;
-
-fn open_dev_zero() -> Result<File, io::Error> {
+fn open_path(path: &str) -> Result<File, io::Error> {
     OpenOptions::new()
         .read(true)
-        .write(true)
-        .open(Path::new(DEV_ZERO))
+        .write(false)
+        .open(Path::new(path))
 }
 
 fn main() {
-    let mut check_file1 = open_dev_zero().unwrap();
-    let mut check_file2 = open_dev_zero().unwrap();
+    let mut check_file1 = open_path(DEV_ZERO).unwrap();
+    let mut check_file2 = open_path(PROC_CMDLINE).unwrap();
     let j = Minijail::new().unwrap();
 
-    for p in &[0, 1, check_file1.as_raw_fd(), check_file2.as_raw_fd()] {
+    let mut stdio_expected = String::new();
+    let mut file2_expected = String::new();
+    for &p in &[0, 1, 2, check_file1.as_raw_fd(), check_file2.as_raw_fd()] {
         let path = format!("/proc/self/fd/{}", p);
         let target = read_link(Path::new(&path));
         eprintln!("P: {} -> {:?}", p, &target);
+        if p == 2 {
+            stdio_expected = target.unwrap().to_string_lossy().to_string();
+        } else if p == check_file2.as_raw_fd() {
+            file2_expected = target.unwrap().to_string_lossy().to_string();
+        }
     }
 
+    // Swap fd1 and fd2.
+    let dest_fd1: RawFd = check_file2.as_raw_fd();
+    let dest_fd2: RawFd = check_file1.as_raw_fd();
+
     if unsafe {
         j.fork_remap(&[
-            (2, 2),
-            (check_file1.as_raw_fd(), DEST_FD1),
-            (check_file2.as_raw_fd(), DEST_FD2),
+            // fd 0 tests stdio mapped to /dev/null.
+            (2, 1),                              // One-to-many.
+            (2, 2),                              // Identity.
+            (check_file1.as_raw_fd(), dest_fd1), // Cross-over.
+            (check_file2.as_raw_fd(), dest_fd2), // Cross-over.
         ])
     }
     .unwrap()
@@ -54,21 +65,27 @@ fn main() {
         return;
     }
 
-    // Safe because we are re-taking ownership of a remapped fd after forking.
-    check_file1 = unsafe { File::from_raw_fd(DEST_FD1) };
-    check_file2 = unsafe { File::from_raw_fd(DEST_FD2) };
+    // Safe because we are re-taking ownership of remapped fds after forking.
+    unsafe {
+        check_file1.into_raw_fd();
+        check_file1 = File::from_raw_fd(dest_fd1);
+
+        check_file2.into_raw_fd();
+        check_file2 = File::from_raw_fd(dest_fd2);
+    }
 
     for (p, expected) in &[
         (0, DEV_NULL),
-        (1, DEV_NULL),
-        (DEST_FD1, DEV_ZERO),
-        (DEST_FD2, DEV_ZERO),
+        (1, &stdio_expected),
+        (2, &stdio_expected),
+        (dest_fd1, DEV_ZERO),
+        (dest_fd2, &file2_expected),
     ] {
         let path = format!("/proc/self/fd/{}", p);
         let target = read_link(Path::new(&path));
-        eprintln!("C: {} -> {:?}", p, &target);
+        eprintln!("  C: {} -> {:?}", p, &target);
         if !matches!(&target, Ok(p) if p == Path::new(expected)) {
-            panic!("C: got {:?}; expected Ok({:?})", target, expected);
+            panic!("  C: got {:?}; expected Ok({:?})", target, expected);
         }
     }
 
@@ -77,5 +94,8 @@ fn main() {
     check_file1.read_exact(&mut buffer).unwrap();
     assert_eq!(&buffer, &[0u8; BUFFER_LEN]);
 
-    eprintln!("Child done.");
+    let mut file2_contents = Vec::<u8>::new();
+    check_file2.read_to_end(&mut file2_contents).unwrap();
+
+    eprintln!("  Child done.");
 }