From fb6b07bf866f083972a0a92c023a0a3b1740bd0c Mon Sep 17 00:00:00 2001 From: Egor Lazarchuk Date: Wed, 5 Mar 2025 17:40:13 +0000 Subject: [PATCH 1/3] chore(jailer): clarify main function wrapping Add a comment why we wrap the actual main inplementation. Signed-off-by: Egor Lazarchuk --- src/jailer/src/main.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/jailer/src/main.rs b/src/jailer/src/main.rs index fc7ac216599..f5ebd663b02 100644 --- a/src/jailer/src/main.rs +++ b/src/jailer/src/main.rs @@ -305,6 +305,7 @@ pub fn to_cstring + Debug>(path: T) -> Result Result<(), JailerError> { let result = main_exec(); if let Err(e) = result { From 975e4f865da3240eb17fce996219e6290c526f6b Mon Sep 17 00:00:00 2001 From: Egor Lazarchuk Date: Wed, 5 Mar 2025 17:45:58 +0000 Subject: [PATCH 2/3] refactor(jailer): use C-string literals Since Rust 1.77 we can create constants with C strings without runtime conversion. Signed-off-by: Egor Lazarchuk --- src/jailer/src/chroot.rs | 33 +++++++++++++-------------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/src/jailer/src/chroot.rs b/src/jailer/src/chroot.rs index 44386729196..cad575227f4 100644 --- a/src/jailer/src/chroot.rs +++ b/src/jailer/src/chroot.rs @@ -10,9 +10,9 @@ use vmm_sys_util::syscall::SyscallReturnCode; use super::{to_cstring, JailerError}; -const OLD_ROOT_DIR_NAME_NUL_TERMINATED: &[u8] = b"old_root\0"; -const ROOT_DIR_NUL_TERMINATED: &[u8] = b"/\0"; -const CURRENT_DIR_NUL_TERMINATED: &[u8] = b".\0"; +const OLD_ROOT_DIR: &CStr = c"old_root"; +const ROOT_DIR: &CStr = c"/"; +const CURRENT_DIR: &CStr = c"."; // This uses switching to a new mount namespace + pivot_root(), together with the regular chroot, // to provide a hardened jail (at least compared to only relying on chroot). @@ -24,16 +24,13 @@ pub fn chroot(path: &Path) -> Result<(), JailerError> { .into_empty_result() .map_err(JailerError::UnshareNewNs)?; - let root_dir = CStr::from_bytes_with_nul(ROOT_DIR_NUL_TERMINATED) - .map_err(JailerError::FromBytesWithNul)?; - // Recursively change the propagation type of all the mounts in this namespace to SLAVE, so // we can call pivot_root. // SAFETY: Safe because we provide valid parameters. SyscallReturnCode(unsafe { libc::mount( null(), - root_dir.as_ptr(), + ROOT_DIR.as_ptr(), null(), libc::MS_SLAVE | libc::MS_REC, null(), @@ -64,25 +61,21 @@ pub fn chroot(path: &Path) -> Result<(), JailerError> { // Change current dir to the chroot dir, so we only need to handle relative paths from now on. env::set_current_dir(path).map_err(JailerError::SetCurrentDir)?; - // We use the CStr conversion to make sure the contents of the byte slice would be a - // valid C string (and for the as_ptr() method). - let old_root_dir = CStr::from_bytes_with_nul(OLD_ROOT_DIR_NAME_NUL_TERMINATED) - .map_err(JailerError::FromBytesWithNul)?; - // Create the old_root folder we're going to use for pivot_root, using a relative path. // SAFETY: The call is safe because we provide valid arguments. - SyscallReturnCode(unsafe { libc::mkdir(old_root_dir.as_ptr(), libc::S_IRUSR | libc::S_IWUSR) }) + SyscallReturnCode(unsafe { libc::mkdir(OLD_ROOT_DIR.as_ptr(), libc::S_IRUSR | libc::S_IWUSR) }) .into_empty_result() .map_err(JailerError::MkdirOldRoot)?; - let cwd = CStr::from_bytes_with_nul(CURRENT_DIR_NUL_TERMINATED) - .map_err(JailerError::FromBytesWithNul)?; - // We are now ready to call pivot_root. We have to use sys_call because there is no libc // wrapper for pivot_root. // SAFETY: Safe because we provide valid parameters. SyscallReturnCode(unsafe { - libc::syscall(libc::SYS_pivot_root, cwd.as_ptr(), old_root_dir.as_ptr()) + libc::syscall( + libc::SYS_pivot_root, + CURRENT_DIR.as_ptr(), + OLD_ROOT_DIR.as_ptr(), + ) }) .into_empty_result() .map_err(JailerError::PivotRoot)?; @@ -90,19 +83,19 @@ pub fn chroot(path: &Path) -> Result<(), JailerError> { // pivot_root doesn't guarantee that we will be in "/" at this point, so switch to "/" // explicitly. // SAFETY: Safe because we provide valid parameters. - SyscallReturnCode(unsafe { libc::chdir(root_dir.as_ptr()) }) + SyscallReturnCode(unsafe { libc::chdir(ROOT_DIR.as_ptr()) }) .into_empty_result() .map_err(JailerError::ChdirNewRoot)?; // Umount the old_root, thus isolating the process from everything outside the jail root folder. // SAFETY: Safe because we provide valid parameters. - SyscallReturnCode(unsafe { libc::umount2(old_root_dir.as_ptr(), libc::MNT_DETACH) }) + SyscallReturnCode(unsafe { libc::umount2(OLD_ROOT_DIR.as_ptr(), libc::MNT_DETACH) }) .into_empty_result() .map_err(JailerError::UmountOldRoot)?; // Remove the no longer necessary old_root directory. // SAFETY: Safe because we provide valid parameters. - SyscallReturnCode(unsafe { libc::rmdir(old_root_dir.as_ptr()) }) + SyscallReturnCode(unsafe { libc::rmdir(OLD_ROOT_DIR.as_ptr()) }) .into_empty_result() .map_err(JailerError::RmOldRootDir) } From 5544bd1f4ad90e407c6591b59e229e1d98cdad34 Mon Sep 17 00:00:00 2001 From: Egor Lazarchuk Date: Wed, 5 Mar 2025 18:02:12 +0000 Subject: [PATCH 3/3] refactor(jailer): use CStr for strings As a follow up of the previous commit, replace string which should be C string with CStr. Signed-off-by: Egor Lazarchuk --- src/jailer/src/env.rs | 45 +++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/src/jailer/src/env.rs b/src/jailer/src/env.rs index 61cd121b0dc..d2e9a80711a 100644 --- a/src/jailer/src/env.rs +++ b/src/jailer/src/env.rs @@ -1,7 +1,7 @@ // Copyright 2018 Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -use std::ffi::{CString, OsString}; +use std::ffi::{CStr, CString, OsString}; use std::fs::{self, canonicalize, read_to_string, File, OpenOptions, Permissions}; use std::io; use std::io::Write; @@ -30,19 +30,19 @@ const STDERR_FILENO: libc::c_int = 2; // Kernel-based virtual machine (hardware virtualization extensions) // minor/major numbers are taken from // https://www.kernel.org/doc/html/latest/admin-guide/devices.html -const DEV_KVM_WITH_NUL: &str = "/dev/kvm"; +const DEV_KVM: &CStr = c"/dev/kvm"; const DEV_KVM_MAJOR: u32 = 10; const DEV_KVM_MINOR: u32 = 232; // TUN/TAP device minor/major numbers are taken from // www.kernel.org/doc/Documentation/networking/tuntap.txt -const DEV_NET_TUN_WITH_NUL: &str = "/dev/net/tun"; +const DEV_NET_TUN: &CStr = c"/dev/net/tun"; const DEV_NET_TUN_MAJOR: u32 = 10; const DEV_NET_TUN_MINOR: u32 = 200; // Random number generator device minor/major numbers are taken from // https://www.kernel.org/doc/Documentation/admin-guide/devices.txt -const DEV_URANDOM_WITH_NUL: &str = "/dev/urandom"; +const DEV_URANDOM: &CStr = c"/dev/urandom"; const DEV_URANDOM_MAJOR: u32 = 1; const DEV_URANDOM_MINOR: u32 = 9; @@ -54,7 +54,7 @@ const DEV_URANDOM_MINOR: u32 = 9; // so we will have to find it at initialization time parsing /proc/misc. // What we do know is the major number for misc devices: // https://elixir.bootlin.com/linux/v6.1.51/source/Documentation/admin-guide/devices.txt -const DEV_UFFD_PATH: &str = "/dev/userfaultfd"; +const DEV_UFFD_PATH: &CStr = c"/dev/userfaultfd"; const DEV_UFFD_MAJOR: u32 = 10; // Relevant folders inside the jail that we create or/and for which we change ownership. @@ -425,18 +425,17 @@ impl Env { fn mknod_and_own_dev( &self, - dev_path_str: &'static str, + dev_path: &CStr, dev_major: u32, dev_minor: u32, ) -> Result<(), JailerError> { - let dev_path = CString::new(dev_path_str).unwrap(); // As per sysstat.h: // S_IFCHR -> character special device // S_IRUSR -> read permission, owner // S_IWUSR -> write permission, owner // See www.kernel.org/doc/Documentation/networking/tuntap.txt, 'Configuration' chapter for // more clarity. - // SAFETY: This is safe because dev_path is CString, and hence null-terminated. + // SAFETY: This is safe because dev_path is CStr, and hence null-terminated. SyscallReturnCode(unsafe { libc::mknod( dev_path.as_ptr(), @@ -445,7 +444,7 @@ impl Env { ) }) .into_empty_result() - .map_err(|err| JailerError::MknodDev(err, dev_path_str.to_owned()))?; + .map_err(|err| JailerError::MknodDev(err, dev_path.to_str().unwrap().to_owned()))?; // SAFETY: This is safe because dev_path is CStr, and hence null-terminated. SyscallReturnCode(unsafe { libc::chown(dev_path.as_ptr(), self.uid(), self.gid()) }) @@ -663,14 +662,14 @@ impl Env { // $: mknod $dev_net_tun_path c 10 200 // www.kernel.org/doc/Documentation/networking/tuntap.txt specifies 10 and 200 as the major // and minor for the /dev/net/tun device. - self.mknod_and_own_dev(DEV_NET_TUN_WITH_NUL, DEV_NET_TUN_MAJOR, DEV_NET_TUN_MINOR)?; + self.mknod_and_own_dev(DEV_NET_TUN, DEV_NET_TUN_MAJOR, DEV_NET_TUN_MINOR)?; // Do the same for /dev/kvm with (major, minor) = (10, 232). - self.mknod_and_own_dev(DEV_KVM_WITH_NUL, DEV_KVM_MAJOR, DEV_KVM_MINOR)?; + self.mknod_and_own_dev(DEV_KVM, DEV_KVM_MAJOR, DEV_KVM_MINOR)?; // And for /dev/urandom with (major, minor) = (1, 9). // If the device is not accessible on the host, output a warning to inform user that MMDS // version 2 will not be available to use. let _ = self - .mknod_and_own_dev(DEV_URANDOM_WITH_NUL, DEV_URANDOM_MAJOR, DEV_URANDOM_MINOR) + .mknod_and_own_dev(DEV_URANDOM, DEV_URANDOM_MAJOR, DEV_URANDOM_MINOR) .map_err(|err| { println!( "Warning! Could not create /dev/urandom device inside jailer: {}.", @@ -1116,14 +1115,14 @@ mod tests { // process management; it can't be isolated from side effects. } - fn ensure_mknod_and_own_dev(env: &Env, dev_path: &'static str, major: u32, minor: u32) { + fn ensure_mknod_and_own_dev(env: &Env, dev_path: &CStr, major: u32, minor: u32) { use std::os::unix::fs::FileTypeExt; // Create a new device node. env.mknod_and_own_dev(dev_path, major, minor).unwrap(); // Ensure device's properties. - let metadata = fs::metadata(dev_path).unwrap(); + let metadata = fs::metadata(dev_path.to_str().unwrap()).unwrap(); assert!(metadata.file_type().is_char_device()); assert_eq!(get_major(metadata.st_rdev()), major); assert_eq!(get_minor(metadata.st_rdev()), minor); @@ -1140,7 +1139,7 @@ mod tests { ), format!( "Failed to create {} via mknod inside the jail: File exists (os error 17)", - dev_path + dev_path.to_str().unwrap() ) ); } @@ -1152,25 +1151,25 @@ mod tests { let env = create_env(mock_cgroups.proc_mounts_path.as_str()); // Ensure device nodes are created with correct major/minor numbers and permissions. - let mut dev_infos: Vec<(&str, u32, u32)> = vec![ - ("/dev/net/tun-test", DEV_NET_TUN_MAJOR, DEV_NET_TUN_MINOR), - ("/dev/kvm-test", DEV_KVM_MAJOR, DEV_KVM_MINOR), + let mut dev_infos: Vec<(&CStr, u32, u32)> = vec![ + (c"/dev/net/tun-test", DEV_NET_TUN_MAJOR, DEV_NET_TUN_MINOR), + (c"/dev/kvm-test", DEV_KVM_MAJOR, DEV_KVM_MINOR), ]; if let Some(uffd_dev_minor) = env.uffd_dev_minor { - dev_infos.push(("/dev/userfaultfd-test", DEV_UFFD_MAJOR, uffd_dev_minor)); + dev_infos.push((c"/dev/userfaultfd-test", DEV_UFFD_MAJOR, uffd_dev_minor)); } for (dev, major, minor) in dev_infos { // Checking this just to be super sure there's no file at `dev_str` path (though // it shouldn't be as we deleted it at the end of the previous test run). - if Path::new(dev).exists() { - fs::remove_file(dev).unwrap(); + if Path::new(dev.to_str().unwrap()).exists() { + fs::remove_file(dev.to_str().unwrap()).unwrap(); } ensure_mknod_and_own_dev(&env, dev, major, minor); // Remove the device node. - fs::remove_file(dev).expect("Could not remove file."); + fs::remove_file(dev.to_str().unwrap()).expect("Could not remove file."); } } @@ -1180,7 +1179,7 @@ mod tests { mock_cgroups.add_v1_mounts().unwrap(); let env = create_env(mock_cgroups.proc_mounts_path.as_str()); - if !Path::new(DEV_UFFD_PATH).exists() { + if !Path::new(DEV_UFFD_PATH.to_str().unwrap()).exists() { assert_eq!(env.uffd_dev_minor, None); } else { assert!(env.uffd_dev_minor.is_some());