jailer: add flag to spawn FC in a new PID ns

luminitavoicu · acatangiu · commit 0c24d8d75161 · 2021-04-19T13:21:10.000+03:00
Signed-off-by: Luminita Voicu &lt;lumivo@amazon.com&gt;
diff --git a/src/jailer/src/env.rs b/src/jailer/src/env.rs
@@ -2,7 +2,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 use std::ffi::{CStr, OsString};
-use std::fs::{self, canonicalize, File, Permissions};
+use std::fs::{self, canonicalize, File, OpenOptions, Permissions};
 use std::os::unix::fs::PermissionsExt;
 use std::os::unix::io::IntoRawFd;
 use std::os::unix::process::CommandExt;
@@ -13,6 +13,8 @@ use crate::cgroup;
 use crate::cgroup::Cgroup;
 use crate::chroot::chroot;
 use crate::{Error, Result};
+use std::io;
+use std::io::Write;
 use utils::arg_parser::Error::MissingValue;
 use utils::syscall::SyscallReturnCode;
 use utils::{arg_parser, validators};
@@ -44,6 +46,10 @@ const DEV_NULL_WITH_NUL: &[u8] = b"/dev/null\0";
 const FOLDER_HIERARCHY: [&[u8]; 4] = [b"/\0", b"/dev\0", b"/dev/net\0", b"/run\0"];
 const FOLDER_PERMISSIONS: u32 = 0o700;
 
+// When running with `--new-pid-ns` flag, the PID of the process running the exec_file differs
+// from jailer's and it is stored inside a dedicated file, prefixed with the below extension.
+const PID_FILE_EXTENSION: &str = ".pid";
+
 // Helper function, since we'll use libc::dup2 a bunch of times for daemonization.
 fn dup2(old_fd: libc::c_int, new_fd: libc::c_int) -> Result<()> {
     // This is safe because we are using a library function with valid parameters.
@@ -60,8 +66,10 @@ pub struct Env {
     gid: u32,
     netns: Option<String>,
     daemonize: bool,
+    new_pid_ns: bool,
     start_time_us: u64,
     start_time_cpu_us: u64,
+    jailer_cpu_time_us: u64,
     extra_args: Vec<String>,
     cgroups: Vec<Cgroup>,
 }
@@ -125,6 +133,8 @@ impl Env {
 
         let daemonize = arguments.flag_present("daemonize");
 
+        let new_pid_ns = arguments.flag_present("new-pid-ns");
+
         // Optional arguments.
         let mut cgroups = Vec::new();
 
@@ -165,8 +175,10 @@ impl Env {
             gid,
             netns,
             daemonize,
+            new_pid_ns,
             start_time_us,
             start_time_cpu_us,
+            jailer_cpu_time_us: 0,
             extra_args: arguments.extra_args(),
             cgroups,
         })
@@ -184,6 +196,56 @@ impl Env {
         self.uid
     }
 
+    fn exec_into_new_pid_ns(&mut self, chroot_exec_file: PathBuf) -> Result<()> {
+        // Unshare into a new PID namespace.
+        // The current process will not be moved into the newly created namespace, but its first
+        // child will assume the role of init(1) in the new namespace.
+        // The call is safe because we're invoking a C library function with valid parameters.
+        SyscallReturnCode(unsafe { libc::unshare(libc::CLONE_NEWPID) })
+            .into_empty_result()
+            .map_err(Error::UnshareNewPID)?;
+
+        // Compute jailer's total CPU time up to the current time.
+        self.jailer_cpu_time_us =
+            utils::time::get_time_us(utils::time::ClockType::ProcessCpu) - self.start_time_cpu_us;
+
+        // Duplicate the current process. The child process will belong to the previously created
+        // PID namespace.
+        // TODO: replace the `unshare()` + `fork()` combo with `clone()` if we ever need to
+        //  squeeze every bit of start-up latency we can get
+        let pid = unsafe { libc::fork() };
+        match pid {
+            0 => {
+                // Reset process start time.
+                self.start_time_cpu_us = 0;
+
+                Err(Error::Exec(self.exec_command(chroot_exec_file)))
+            }
+            child_pid => {
+                // Save the PID of the process running the exec file provided
+                // inside <chroot_exec_file>.pid file.
+                self.save_exec_file_pid(child_pid, chroot_exec_file)?;
+                unsafe { libc::exit(0) }
+            }
+        }
+    }
+
+    fn save_exec_file_pid(&mut self, pid: i32, chroot_exec_file: PathBuf) -> Result<()> {
+        let chroot_exec_file_str = chroot_exec_file
+            .to_str()
+            .ok_or_else(|| Error::FileName(chroot_exec_file.clone()))?;
+        let pid_file_path =
+            PathBuf::from(format!("{}{}", chroot_exec_file_str, PID_FILE_EXTENSION));
+        let mut pid_file = OpenOptions::new()
+            .write(true)
+            .create_new(true)
+            .open(pid_file_path.clone())
+            .map_err(|e| Error::FileOpen(pid_file_path.clone(), e))?;
+
+        // Write PID to file.
+        write!(pid_file, "{}", pid).map_err(|e| Error::Write(pid_file_path, e))
+    }
+
     fn mknod_and_own_dev(
         &self,
         dev_path_str: &'static [u8],
@@ -278,6 +340,21 @@ impl Env {
             .map_err(Error::CloseNetNsFd)
     }
 
+    fn exec_command(&self, chroot_exec_file: PathBuf) -> io::Error {
+        Command::new(chroot_exec_file)
+            .args(&["--id", &self.id])
+            .args(&["--start-time-us", &self.start_time_us.to_string()])
+            .args(&["--start-time-cpu-us", &self.start_time_cpu_us.to_string()])
+            .args(&["--parent-cpu-time-us", &self.jailer_cpu_time_us.to_string()])
+            .stdin(Stdio::inherit())
+            .stdout(Stdio::inherit())
+            .stderr(Stdio::inherit())
+            .uid(self.uid())
+            .gid(self.gid())
+            .args(&self.extra_args)
+            .exec()
+    }
+
     #[cfg(target_arch = "aarch64")]
     fn copy_cache_info(&self) -> Result<()> {
         use crate::{readln_special, to_cstring, writeln_special};
@@ -447,19 +524,12 @@ impl Env {
                 .map_err(Error::CloseDevNullFd)?;
         }
 
-        Err(Error::Exec(
-            Command::new(chroot_exec_file)
-                .args(&["--id", &self.id])
-                .args(&["--start-time-us", &self.start_time_us.to_string()])
-                .args(&["--start-time-cpu-us", &self.start_time_cpu_us.to_string()])
-                .stdin(Stdio::inherit())
-                .stdout(Stdio::inherit())
-                .stderr(Stdio::inherit())
-                .uid(self.uid())
-                .gid(self.gid())
-                .args(self.extra_args)
-                .exec(),
-        ))
+        // If specified, exec the provided binary into a new PID namespace.
+        if self.new_pid_ns {
+            self.exec_into_new_pid_ns(chroot_exec_file)
+        } else {
+            Err(Error::Exec(self.exec_command(chroot_exec_file)))
+        }
     }
 }
 
@@ -483,6 +553,7 @@ mod tests {
         pub chroot_base: &'a str,
         pub netns: Option<&'a str>,
         pub daemonize: bool,
+        pub new_pid_ns: bool,
         pub cgroups: Vec<&'a str>,
     }
 
@@ -497,6 +568,7 @@ mod tests {
                 chroot_base: "/",
                 netns: Some("zzzns"),
                 daemonize: true,
+                new_pid_ns: true,
                 cgroups: vec!["cpu.shares=2", "cpuset.mems=0"],
             }
         }
@@ -537,6 +609,10 @@ mod tests {
             arg_vec.push("--daemonize".to_string());
         }
 
+        if arg_vals.new_pid_ns {
+            arg_vec.push("--new-pid-ns".to_string());
+        }
+
         arg_vec
     }
 
@@ -577,10 +653,12 @@ mod tests {
 
         assert_eq!(good_env.netns, good_arg_vals.netns.map(String::from));
         assert!(good_env.daemonize);
+        assert!(good_env.new_pid_ns);
 
         let another_good_arg_vals = ArgVals {
             netns: None,
             daemonize: false,
+            new_pid_ns: false,
             ..good_arg_vals
         };
 
@@ -590,6 +668,7 @@ mod tests {
         let another_good_env = Env::new(&args, 0, 0)
             .expect("This another new environment should be created successfully.");
         assert!(!another_good_env.daemonize);
+        assert!(!another_good_env.new_pid_ns);
 
         let base_invalid_arg_vals = ArgVals {
             daemonize: true,
@@ -796,6 +875,7 @@ mod tests {
             chroot_base: some_dir_path,
             netns: Some("zzzns"),
             daemonize: false,
+            new_pid_ns: false,
             cgroups: Vec::new(),
         };
         fs::write(some_file_path, "some_content").unwrap();
@@ -939,4 +1019,19 @@ mod tests {
         let entries = fs::read_dir(&index_dest_path).unwrap();
         assert_eq!(entries.enumerate().count(), 6);
     }
+
+    #[test]
+    fn test_save_exec_file_pid() {
+        let exec_file_name = "file";
+        let pid_file_name = "file.pid";
+        let pid = 1;
+
+        let mut env = create_env();
+        env.save_exec_file_pid(pid, PathBuf::from(exec_file_name))
+            .unwrap();
+
+        let stored_pid = fs::read_to_string(pid_file_name);
+        fs::remove_file(pid_file_name).unwrap();
+        assert_eq!(stored_pid.unwrap(), "1");
+    }
 }
diff --git a/src/jailer/src/main.rs b/src/jailer/src/main.rs
@@ -64,6 +64,7 @@ pub enum Error {
     UmountOldRoot(io::Error),
     UnexpectedListenerFd(i32),
     UnshareNewNs(io::Error),
+    UnshareNewPID(io::Error),
     UnsetCloexec(io::Error),
     Write(PathBuf, io::Error),
 }
@@ -200,6 +201,9 @@ impl fmt::Display for Error {
             UnshareNewNs(ref err) => {
                 write!(f, "Failed to unshare into new mount namespace: {}", err)
             }
+            UnshareNewPID(ref err) => {
+                write!(f, "Failed to unshare into new PID namespace: {}", err)
+            }
             UnsetCloexec(ref err) => write!(
                 f,
                 "Failed to unset the O_CLOEXEC flag on the socket fd: {}",
@@ -265,6 +269,11 @@ pub fn build_arg_parser() -> ArgParser<'static> {
             "Daemonize the jailer before exec, by invoking setsid(), and redirecting \
              the standard I/O file descriptors to /dev/null.",
         ))
+        .arg(
+            Argument::new("new-pid-ns")
+                .takes_value(false)
+                .help("Exec into a new PID namespace."),
+        )
         .arg(Argument::new("cgroup").allow_multiple(true).help(
             "Cgroup and value to be set by the jailer. It must follow this format: \
              <cgroup_file>=<value> (e.g cpu.shares=10). This argument can be used \
@@ -665,6 +674,10 @@ mod tests {
             format!("{}", Error::UnshareNewNs(io::Error::from_raw_os_error(42))),
             "Failed to unshare into new mount namespace: No message of desired type (os error 42)",
         );
+        assert_eq!(
+            format!("{}", Error::UnshareNewPID(io::Error::from_raw_os_error(42))),
+            "Failed to unshare into new PID namespace: No message of desired type (os error 42)",
+        );
         assert_eq!(
             format!("{}", Error::UnsetCloexec(io::Error::from_raw_os_error(42))),
             "Failed to unset the O_CLOEXEC flag on the socket fd: No message of desired type (os \
diff --git a/tests/framework/jailer.py b/tests/framework/jailer.py
@@ -32,6 +32,7 @@ class JailerContext:
     chroot_base = None
     netns = None
     daemonize = None
+    new_pid_ns = None
     extra_args = None
     api_socket_name = None
     cgroups = None
@@ -46,6 +47,7 @@ def __init__(
             chroot_base=DEFAULT_CHROOT_PATH,
             netns=None,
             daemonize=True,
+            new_pid_ns=False,
             cgroups=None,
             **extra_args
     ):
@@ -63,6 +65,7 @@ def __init__(
         self.chroot_base = chroot_base
         self.netns = netns if netns is not None else jailer_id
         self.daemonize = daemonize
+        self.new_pid_ns = new_pid_ns
         self.extra_args = extra_args
         self.api_socket_name = DEFAULT_USOCKET_NAME
         self.cgroups = cgroups
@@ -103,6 +106,8 @@ def construct_param_list(self):
             jailer_param_list.extend(['--netns', str(self.netns_file_path())])
         if self.daemonize:
             jailer_param_list.append('--daemonize')
+        if self.new_pid_ns:
+            jailer_param_list.append('--new-pid-ns')
         if self.cgroups is not None:
             for cgroup in self.cgroups:
                 jailer_param_list.extend(['--cgroup', str(cgroup)])
diff --git a/tests/integration_tests/performance/test_process_startup_time.py b/tests/integration_tests/performance/test_process_startup_time.py
@@ -14,9 +14,21 @@
 # TODO: Keep a `current` startup time in S3 and validate we don't regress
 
 
-def test_startup_time(test_microvm_with_api):
-    """Check the startup time for jailer and Firecracker up to socket bind."""
+def test_startup_time_new_pid_ns(test_microvm_with_api):
+    """Check startup time when jailer is spawned in a new PID namespace."""
     microvm = test_microvm_with_api
+    microvm.bin_cloner_path = None
+    microvm.jailer.new_pid_ns = True
+    _test_startup_time(microvm)
+
+
+def test_startup_time_daemonize(test_microvm_with_api):
+    """Check startup time when jailer spawns Firecracker in a new PID ns."""
+    microvm = test_microvm_with_api
+    _test_startup_time(microvm)
+
+
+def _test_startup_time(microvm):
     microvm.spawn()
 
     microvm.basic_config(vcpu_count=2, mem_size_mib=1024)
diff --git a/tests/integration_tests/security/test_jail.py b/tests/integration_tests/security/test_jail.py
