remove instantes of using IntoRawFd in unsafe blocks

The trait IntoRawFd isn't marked unsafe, but its documentation says that
an impl must return a uniquely owned RawFd. Some code blocks depended on
that behavior to ensure safety with the unsafe File::from_raw_fd, but
this leads to a soundness hole where a nominally safe impl of IntoRawFd
can lead to unsafety in functions that had been left as safe.

This change sidesteps the issue by not using IntoRawFd, and using only
safe conversions instead.

BUG=None
TEST=cargo build --features='wl-dmabuf plugin'

Change-Id: I9b357e5592be21189fb96e343823dd63000aac30
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2185580
Reviewed-by: Zach Reizner <zachr@chromium.org>
Tested-by: Zach Reizner <zachr@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Commit-Queue: Zach Reizner <zachr@chromium.org>
Auto-Submit: Zach Reizner <zachr@chromium.org>

4 files changed, 19 insertions, 20 deletions

diff --git a/devices/src/virtio/wl.rs b/devices/src/virtio/wl.rs
index d59a801..8e94783 100644
--- a/devices/src/virtio/wl.rs
+++ b/devices/src/virtio/wl.rs
@@ -48,7 +48,7 @@ use std::thread;
 use std::time::Duration;
 
 #[cfg(feature = "wl-dmabuf")]
-use libc::{dup, EBADF, EINVAL};
+use libc::{EBADF, EINVAL};
 
 use data_model::VolatileMemoryError;
 use data_model::*;
@@ -587,15 +587,13 @@ impl WlVfd {
             })?;
         match allocate_and_register_gpu_memory_response {
             VmMemoryResponse::AllocateAndRegisterGpuMemory {
-                fd,
+                fd: MaybeOwnedFd::Owned(file),
                 pfn,
                 slot,
                 desc,
             } => {
                 let mut vfd = WlVfd::default();
-                // Duplicate FD for shared memory instance.
-                let raw_fd = unsafe { File::from_raw_fd(dup(fd.as_raw_fd())) };
-                let vfd_shm = SharedMemory::from_raw_fd(raw_fd).map_err(WlError::NewAlloc)?;
+                let vfd_shm = SharedMemory::from_file(file).map_err(WlError::NewAlloc)?;
                 vfd.guest_shared_memory = Some((vfd_shm.size(), vfd_shm.into()));
                 vfd.slot = Some((slot, pfn, vm));
                 vfd.is_dmabuf = true;
diff --git a/src/plugin/mod.rs b/src/plugin/mod.rs
index ae7e19c..470d5f0 100644
--- a/src/plugin/mod.rs
+++ b/src/plugin/mod.rs
@@ -8,7 +8,7 @@ mod vcpu;
 use std::fmt::{self, Display};
 use std::fs::File;
 use std::io;
-use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd};
+use std::os::unix::io::{AsRawFd, FromRawFd};
 use std::os::unix::net::UnixDatagram;
 use std::path::Path;
 use std::result;
@@ -181,10 +181,6 @@ impl Display for Error {
 
 type Result<T> = result::Result<T, Error>;
 
-fn downcast_file<F: IntoRawFd>(f: F) -> File {
-    unsafe { File::from_raw_fd(f.into_raw_fd()) }
-}
-
 fn new_seqpacket_pair() -> SysResult<(UnixDatagram, UnixDatagram)> {
     let mut fds = [0, 0];
     unsafe {
diff --git a/src/plugin/process.rs b/src/plugin/process.rs
index 6f65f1d..783239a 100644
--- a/src/plugin/process.rs
+++ b/src/plugin/process.rs
@@ -7,7 +7,7 @@ use std::env::set_var;
 use std::fs::File;
 use std::io::Write;
 use std::mem::transmute;
-use std::os::unix::io::{AsRawFd, RawFd};
+use std::os::unix::io::{IntoRawFd, RawFd};
 use std::os::unix::net::UnixDatagram;
 use std::path::Path;
 use std::process::Command;
@@ -346,7 +346,7 @@ impl Process {
         read_only: bool,
         dirty_log: bool,
     ) -> SysResult<()> {
-        let shm = SharedMemory::from_raw_fd(memfd)?;
+        let shm = SharedMemory::from_file(memfd)?;
         // Checking the seals ensures the plugin process won't shrink the mmapped file, causing us
         // to SIGBUS in the future.
         let seals = shm.get_seals()?;
@@ -515,7 +515,14 @@ impl Process {
         let request = protobuf::parse_from_bytes::<MainRequest>(&self.request_buffer[..msg_size])
             .map_err(Error::DecodeRequest)?;
 
-        let mut response_files = Vec::new();
+        /// Use this to make it easier to stuff various kinds of File-like objects into the
+        /// `boxed_fds` list.
+        fn box_owned_fd<F: IntoRawFd + 'static>(f: F) -> Box<dyn IntoRawFd> {
+            Box::new(f)
+        }
+
+        // This vec is used to extend ownership of certain FDs until the end of this function.
+        let mut boxed_fds = Vec::new();
         let mut response_fds = Vec::new();
         let mut response = MainResponse::new();
         let res = if request.has_create() {
@@ -557,7 +564,7 @@ impl Process {
                                 Ok(()) => {
                                     response_fds.push(evt.as_raw_fd());
                                     response_fds.push(resample_evt.as_raw_fd());
-                                    response_files.push(downcast_file(resample_evt));
+                                    boxed_fds.push(box_owned_fd(resample_evt));
                                     entry.insert(PluginObject::IrqEvent {
                                         irq_id: irq_event.irq_id,
                                         evt,
@@ -586,7 +593,7 @@ impl Process {
                 Ok((request_socket, child_socket)) => {
                     self.request_sockets.push(request_socket);
                     response_fds.push(child_socket.as_raw_fd());
-                    response_files.push(downcast_file(child_socket));
+                    boxed_fds.push(box_owned_fd(child_socket));
                     Ok(())
                 }
                 Err(e) => Err(e),
diff --git a/sys_util/src/shm.rs b/sys_util/src/shm.rs
index d158230..ee5f5a3 100644
--- a/sys_util/src/shm.rs
+++ b/sys_util/src/shm.rs
@@ -5,7 +5,7 @@
 use std::ffi::{CStr, CString};
 use std::fs::{read_link, File};
 use std::io::{self, Read, Seek, SeekFrom, Write};
-use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd};
+use std::os::unix::io::{AsRawFd, FromRawFd, RawFd};
 
 use libc::{
     self, c_char, c_int, c_long, c_uint, close, fcntl, ftruncate64, off64_t, syscall, EINVAL,
@@ -136,13 +136,11 @@ impl SharedMemory {
         Ok(SharedMemory { fd: file, size: 0 })
     }
 
-    /// Constructs a `SharedMemory` instance from a file descriptor that represents shared memory.
+    /// Constructs a `SharedMemory` instance from a `File` that represents shared memory.
     ///
     /// The size of the resulting shared memory will be determined using `File::seek`. If the given
     /// file's size can not be determined this way, this will return an error.
-    pub fn from_raw_fd<T: IntoRawFd>(fd: T) -> Result<SharedMemory> {
-        // Safe because the IntoRawFd trait indicates fd has unique ownership.
-        let mut file = unsafe { File::from_raw_fd(fd.into_raw_fd()) };
+    pub fn from_file(mut file: File) -> Result<SharedMemory> {
         let file_size = file.seek(SeekFrom::End(0))?;
         Ok(SharedMemory {
             fd: file,