From bc55e30b89025bd5c214ce0ce17bfde837f06db6 Mon Sep 17 00:00:00 2001 From: Andrew Walbran Date: Tue, 13 Jul 2021 17:35:10 +0100 Subject: [PATCH] Handle /proc/self/fd/N specially for composite disk components and QCOW2. Follow-up to https://crrev.com/c/3000986. BUG=b:192256642 TEST=cargo test Change-Id: I08c87f9295e3c8d977c6028e5d8d7e8beeafbaec Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3024191 Auto-Submit: Andrew Walbran Tested-by: kokoro Commit-Queue: Chirantan Ekbote Reviewed-by: Chirantan Ekbote --- disk/src/composite.rs | 19 ++++----- disk/src/qcow/mod.rs | 17 ++++---- src/linux.rs | 79 ++++++++++---------------------------- sys_util/src/descriptor.rs | 7 ++++ sys_util/src/lib.rs | 75 ++++++++++++++++++++++++++++++++++-- 5 files changed, 115 insertions(+), 82 deletions(-) diff --git a/disk/src/composite.rs b/disk/src/composite.rs index 3938d1b0fc..79c635a274 100644 --- a/disk/src/composite.rs +++ b/disk/src/composite.rs @@ -6,14 +6,14 @@ use std::cmp::{max, min}; use std::collections::HashSet; use std::convert::TryInto; use std::fmt::{self, Display}; -use std::fs::{File, OpenOptions}; +use std::fs::File; use std::io::{self, ErrorKind, Read, Seek, SeekFrom, Write}; use std::ops::Range; use std::path::{Path, PathBuf}; use base::{ - AsRawDescriptors, FileAllocate, FileReadWriteAtVolatile, FileSetLen, FileSync, PunchHole, - RawDescriptor, WriteZeroesAt, + open_file, AsRawDescriptors, FileAllocate, FileReadWriteAtVolatile, FileSetLen, FileSync, + PunchHole, RawDescriptor, WriteZeroesAt, }; use crc32fast::Hasher; use data_model::VolatileSlice; @@ -189,18 +189,15 @@ impl CompositeDiskFile { if proto.get_version() != COMPOSITE_DISK_VERSION { return Err(Error::UnknownVersion(proto.get_version())); } - let mut open_options = OpenOptions::new(); - open_options.read(true); let mut disks: Vec = proto .get_component_disks() .iter() .map(|disk| { - open_options.write( - disk.get_read_write_capability() == cdisk_spec::ReadWriteCapability::READ_WRITE, - ); - let file = open_options - .open(disk.get_file_path()) - .map_err(|e| Error::OpenFile(e, disk.get_file_path().to_string()))?; + let file = open_file( + Path::new(disk.get_file_path()), + disk.get_read_write_capability() != cdisk_spec::ReadWriteCapability::READ_WRITE, + ) + .map_err(|e| Error::OpenFile(e.into(), disk.get_file_path().to_string()))?; Ok(ComponentDiskPart { file: create_disk_file(file).map_err(|e| Error::DiskError(Box::new(e)))?, offset: disk.get_offset(), diff --git a/disk/src/qcow/mod.rs b/disk/src/qcow/mod.rs index c7c56cc710..a66487c607 100644 --- a/disk/src/qcow/mod.rs +++ b/disk/src/qcow/mod.rs @@ -7,7 +7,7 @@ mod refcount; mod vec_cache; use base::{ - error, AsRawDescriptor, AsRawDescriptors, FileAllocate, FileReadWriteAtVolatile, + error, open_file, AsRawDescriptor, AsRawDescriptors, FileAllocate, FileReadWriteAtVolatile, FileReadWriteVolatile, FileSetLen, FileSync, PunchHole, RawDescriptor, SeekHole, WriteZeroesAt, }; use data_model::{VolatileMemory, VolatileSlice}; @@ -16,9 +16,10 @@ use remain::sorted; use std::cmp::{max, min}; use std::fmt::{self, Display}; -use std::fs::{File, OpenOptions}; +use std::fs::File; use std::io::{self, Read, Seek, SeekFrom, Write}; use std::mem::size_of; +use std::path::Path; use std::str; use crate::qcow::qcow_raw_file::QcowRawFile; @@ -445,10 +446,8 @@ impl QcowFile { let backing_file = if let Some(backing_file_path) = header.backing_file_path.as_ref() { let path = backing_file_path.clone(); - let backing_raw_file = OpenOptions::new() - .read(true) - .open(path) - .map_err(Error::BackingFileIo)?; + let backing_raw_file = + open_file(Path::new(&path), true).map_err(|e| Error::BackingFileIo(e.into()))?; let backing_file = create_disk_file(backing_raw_file) .map_err(|e| Error::BackingFileOpen(Box::new(e)))?; Some(backing_file) @@ -582,10 +581,8 @@ impl QcowFile { /// Creates a new QcowFile at the given path. pub fn new_from_backing(file: File, backing_file_name: &str) -> Result { - let backing_raw_file = OpenOptions::new() - .read(true) - .open(backing_file_name) - .map_err(Error::BackingFileIo)?; + let backing_raw_file = open_file(Path::new(backing_file_name), true) + .map_err(|e| Error::BackingFileIo(e.into()))?; let backing_file = create_disk_file(backing_raw_file).map_err(|e| Error::BackingFileOpen(Box::new(e)))?; let size = backing_file.get_len().map_err(Error::BackingFileIo)?; diff --git a/src/linux.rs b/src/linux.rs index a532b66c30..392cb44adc 100644 --- a/src/linux.rs +++ b/src/linux.rs @@ -16,7 +16,6 @@ use std::iter; use std::mem; use std::net::Ipv4Addr; use std::num::ParseIntError; -use std::os::unix::io::FromRawFd; use std::os::unix::net::UnixStream; use std::path::{Path, PathBuf}; use std::ptr; @@ -149,8 +148,7 @@ pub enum Error { #[cfg(all(target_arch = "x86_64", feature = "gdb"))] HandleDebugCommand(::Error), InputDeviceNew(virtio::InputError), - InputEventsOpen(std::io::Error), - InvalidFdPath, + InputEventsOpen(io::Error), InvalidWaylandPath, IoJail(minijail::Error), LoadKernel(Box), @@ -281,7 +279,6 @@ impl Display for Error { HandleDebugCommand(e) => write!(f, "failed to handle a gdb command: {}", e), InputDeviceNew(e) => write!(f, "failed to set up input device: {}", e), InputEventsOpen(e) => write!(f, "failed to open event device: {}", e), - InvalidFdPath => write!(f, "failed parsing a /proc/self/fd/*"), InvalidWaylandPath => write!(f, "wayland socket path has no parent or file name"), IoJail(e) => write!(f, "{}", e), LoadKernel(e) => write!(f, "failed to load kernel: {}", e), @@ -520,33 +517,9 @@ fn simple_jail(cfg: &Config, policy: &str) -> Result> { type DeviceResult = std::result::Result; -/// Open the file with the given path, or if it is of the form `/proc/self/fd/N` then just use the -/// file descriptor. -/// -/// Note that this will not work properly if the same `/proc/self/fd/N` path is used twice in -/// different places, as the metadata (including the offset) will be shared between both file -/// descriptors. -fn open_file(path: &Path, read_only: bool, error_constructor: F) -> Result -where - F: FnOnce(PathBuf, io::Error) -> Error, -{ - // Special case '/proc/self/fd/*' paths. The FD is already open, just use it. - Ok( - if let Some(raw_descriptor) = raw_descriptor_from_path(path)? { - // Safe because we will validate |raw_fd|. - unsafe { File::from_raw_descriptor(raw_descriptor) } - } else { - OpenOptions::new() - .read(true) - .write(!read_only) - .open(path) - .map_err(|e| error_constructor(path.to_path_buf(), e))? - }, - ) -} - fn create_block_device(cfg: &Config, disk: &DiskOption, disk_device_tube: Tube) -> DeviceResult { - let raw_image: File = open_file(&disk.path, disk.read_only, Error::Disk)?; + let raw_image: File = open_file(&disk.path, disk.read_only) + .map_err(|e| Error::Disk(disk.path.clone(), e.into()))?; // Lock the disk image to prevent other crosvm instances from using it. let lock_op = if disk.read_only { FlockOperation::LockShared @@ -1344,7 +1317,8 @@ fn create_pmem_device( index: usize, pmem_device_tube: Tube, ) -> DeviceResult { - let fd = open_file(&disk.path, disk.read_only, Error::Disk)?; + let fd = open_file(&disk.path, disk.read_only) + .map_err(|e| Error::Disk(disk.path.clone(), e.into()))?; let arena_size = { let metadata = @@ -1954,32 +1928,16 @@ fn add_crosvm_user_to_jail(jail: &mut Minijail, feature: &str) -> Result { }) } -/// If the given path is of the form /proc/self/fd/N for some N, returns `Ok(Some(N))`. Otherwise -/// returns `Ok(None`). -fn raw_descriptor_from_path(path: &Path) -> Result> { - if path.parent() == Some(Path::new("/proc/self/fd")) { - let raw_descriptor = path - .file_name() - .and_then(|fd_osstr| fd_osstr.to_str()) - .and_then(|fd_str| fd_str.parse::().ok()) - .ok_or(Error::InvalidFdPath)?; - validate_raw_descriptor(raw_descriptor) - .map_err(Error::ValidateRawDescriptor) - .map(Some) - } else { - Ok(None) - } -} - trait IntoUnixStream { fn into_unix_stream(self) -> Result; } impl<'a> IntoUnixStream for &'a Path { fn into_unix_stream(self) -> Result { - if let Some(raw_descriptor) = raw_descriptor_from_path(self)? { - // Safe because we will validate |raw_fd|. - unsafe { Ok(UnixStream::from_raw_fd(raw_descriptor)) } + if let Some(fd) = + safe_descriptor_from_path(self).map_err(|e| Error::InputEventsOpen(e.into()))? + { + Ok(fd.into()) } else { UnixStream::connect(self).map_err(Error::InputEventsOpen) } @@ -2442,18 +2400,23 @@ where fn setup_vm_components(cfg: &Config) -> Result { let initrd_image = if let Some(initrd_path) = &cfg.initrd_path { - Some(open_file(initrd_path, true, Error::OpenInitrd)?) + Some( + open_file(initrd_path, true) + .map_err(|e| Error::OpenInitrd(initrd_path.to_owned(), e.into()))?, + ) } else { None }; let vm_image = match cfg.executable_path { - Some(Executable::Kernel(ref kernel_path)) => { - VmImage::Kernel(open_file(kernel_path, true, Error::OpenKernel)?) - } - Some(Executable::Bios(ref bios_path)) => { - VmImage::Bios(open_file(bios_path, true, Error::OpenBios)?) - } + Some(Executable::Kernel(ref kernel_path)) => VmImage::Kernel( + open_file(kernel_path, true) + .map_err(|e| Error::OpenKernel(kernel_path.to_owned(), e.into()))?, + ), + Some(Executable::Bios(ref bios_path)) => VmImage::Bios( + open_file(bios_path, true) + .map_err(|e| Error::OpenBios(bios_path.to_owned(), e.into()))?, + ), _ => panic!("Did not receive a bios or kernel, should be impossible."), }; diff --git a/sys_util/src/descriptor.rs b/sys_util/src/descriptor.rs index 4bccc58b66..7ea4dfb3ab 100644 --- a/sys_util/src/descriptor.rs +++ b/sys_util/src/descriptor.rs @@ -161,6 +161,13 @@ impl From for SafeDescriptor { } } +impl From for UnixStream { + fn from(s: SafeDescriptor) -> Self { + // Safe because we own the SafeDescriptor at this point. + unsafe { Self::from_raw_fd(s.into_raw_descriptor()) } + } +} + /// For use cases where a simple wrapper around a RawDescriptor is needed. /// This is a simply a wrapper and does not manage the lifetime of the descriptor. /// Most usages should prefer SafeDescriptor or using a RawDescriptor directly diff --git a/sys_util/src/lib.rs b/sys_util/src/lib.rs index e59b9cf72e..d36fd91c5a 100644 --- a/sys_util/src/lib.rs +++ b/sys_util/src/lib.rs @@ -94,16 +94,17 @@ pub use crate::write_zeroes::{PunchHole, WriteZeroes, WriteZeroesAt}; use std::cell::Cell; use std::ffi::CStr; -use std::fs::{remove_file, File}; +use std::fs::{remove_file, File, OpenOptions}; use std::mem; use std::os::unix::io::{AsRawFd, FromRawFd, RawFd}; use std::os::unix::net::UnixDatagram; +use std::path::Path; use std::ptr; use std::time::Duration; use libc::{ - c_int, c_long, fcntl, pipe2, syscall, sysconf, waitpid, SYS_getpid, SYS_gettid, F_GETFL, - F_SETFL, O_CLOEXEC, SIGKILL, WNOHANG, _SC_IOV_MAX, _SC_PAGESIZE, + c_int, c_long, fcntl, pipe2, syscall, sysconf, waitpid, SYS_getpid, SYS_gettid, EINVAL, + F_GETFL, F_SETFL, O_CLOEXEC, SIGKILL, WNOHANG, _SC_IOV_MAX, _SC_PAGESIZE, }; /// Re-export libc types that are part of the API. @@ -465,8 +466,45 @@ pub fn max_timeout() -> Duration { Duration::new(libc::time_t::max_value() as u64, 999999999) } +/// If the given path is of the form /proc/self/fd/N for some N, returns `Ok(Some(N))`. Otherwise +/// returns `Ok(None`). +pub fn safe_descriptor_from_path>(path: P) -> Result> { + let path = path.as_ref(); + if path.parent() == Some(Path::new("/proc/self/fd")) { + let raw_descriptor = path + .file_name() + .and_then(|fd_osstr| fd_osstr.to_str()) + .and_then(|fd_str| fd_str.parse::().ok()) + .ok_or_else(|| Error::new(EINVAL))?; + let validated_fd = validate_raw_fd(raw_descriptor)?; + Ok(Some( + // Safe because nothing else has access to validated_fd after this call. + unsafe { SafeDescriptor::from_raw_descriptor(validated_fd) }, + )) + } else { + Ok(None) + } +} + +/// Open the file with the given path, or if it is of the form `/proc/self/fd/N` then just use the +/// file descriptor. +/// +/// Note that this will not work properly if the same `/proc/self/fd/N` path is used twice in +/// different places, as the metadata (including the offset) will be shared between both file +/// descriptors. +pub fn open_file>(path: P, read_only: bool) -> Result { + let path = path.as_ref(); + // Special case '/proc/self/fd/*' paths. The FD is already open, just use it. + Ok(if let Some(fd) = safe_descriptor_from_path(path)? { + fd.into() + } else { + OpenOptions::new().read(true).write(!read_only).open(path)? + }) +} + #[cfg(test)] mod tests { + use libc::EBADF; use std::io::Write; use super::*; @@ -481,4 +519,35 @@ mod tests { tx.write(&[0u8; 8]) .expect_err("Write after fill didn't fail"); } + + #[test] + fn safe_descriptor_from_path_valid() { + assert!(safe_descriptor_from_path(Path::new("/proc/self/fd/2")) + .unwrap() + .is_some()); + } + + #[test] + fn safe_descriptor_from_path_invalid_integer() { + assert_eq!( + safe_descriptor_from_path(Path::new("/proc/self/fd/blah")), + Err(Error::new(EINVAL)) + ); + } + + #[test] + fn safe_descriptor_from_path_invalid_fd() { + assert_eq!( + safe_descriptor_from_path(Path::new("/proc/self/fd/42")), + Err(Error::new(EBADF)) + ); + } + + #[test] + fn safe_descriptor_from_path_none() { + assert_eq!( + safe_descriptor_from_path(Path::new("/something/else")).unwrap(), + None + ); + } }