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 <qwandor@google.com>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Chirantan Ekbote <chirantan@chromium.org>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>

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<ComponentDiskPart> = 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(),

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<QcowFile> {
-        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)?;

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(<Arch as LinuxArch>::Error),
     InputDeviceNew(virtio::InputError),
-    InputEventsOpen(std::io::Error),
-    InvalidFdPath,
+    InputEventsOpen(io::Error),
     InvalidWaylandPath,
     IoJail(minijail::Error),
     LoadKernel(Box<dyn StdError>),
@@ -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<Option<Minijail>> {
 
 type DeviceResult<T = VirtioDeviceStub> = std::result::Result<T, Error>;
 
-/// 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<F>(path: &Path, read_only: bool, error_constructor: F) -> Result<File>
-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<Ids> {
     })
 }
 
-/// 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<Option<RawDescriptor>> {
-    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::<c_int>().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<UnixStream>;
 }
 
 impl<'a> IntoUnixStream for &'a Path {
     fn into_unix_stream(self) -> Result<UnixStream> {
-        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<VmComponents> {
     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."),
     };
 

sys_util/src/descriptor.rs

@@ -161,6 +161,13 @@ impl From<File> for SafeDescriptor {
     }
 }
 
+impl From<SafeDescriptor> 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

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<P: AsRef<Path>>(path: P) -> Result<Option<SafeDescriptor>> {
+    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::<RawFd>().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<P: AsRef<Path>>(path: P, read_only: bool) -> Result<File> {
+    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
+        );
+    }
 }