devices: don't jail in ProxyDevice constructor

Jail functionality has been moved into a new JailedDevice struct,
which wraps ProxyDevice.

Doing this allows ProxyDevice to be much more generic and reusable.

5 files changed, 190 insertions, 148 deletions

diff --git a/arch/src/lib.rs b/arch/src/lib.rs
index bab679a..2e875a5 100644
--- a/arch/src/lib.rs
+++ b/arch/src/lib.rs
@@ -20,8 +20,8 @@ use acpi_tables::sdt::SDT;
 use devices::split_irqchip_common::GsiRelay;
 use devices::virtio::VirtioDevice;
 use devices::{
-    Bus, BusDevice, BusError, PciAddress, PciDevice, PciDeviceError, PciInterruptPin, PciRoot,
-    ProxyDevice,
+    Bus, BusDevice, BusError, JailedDevice, PciAddress, PciDevice, PciDeviceError, PciInterruptPin,
+    PciRoot,
 };
 use io_jail::Minijail;
 use kvm::{IoeventAddress, Kvm, Vcpu, Vm};
@@ -256,7 +256,7 @@ pub fn generate_pci_root(
             keep_fds.push(event.as_raw_fd());
         }
         let arced_dev: Arc<Mutex<dyn BusDevice>> = if let Some(jail) = jail {
-            let proxy = ProxyDevice::new(device, &jail, keep_fds)
+            let proxy = JailedDevice::new(device, &jail, keep_fds)
                 .map_err(DeviceRegistrationError::ProxyDeviceCreation)?;
             pid_labels.insert(proxy.pid() as u32, proxy.debug_label());
             Arc::new(Mutex::new(proxy))
diff --git a/arch/src/serial.rs b/arch/src/serial.rs
index b817cfd..14ed70a 100644
--- a/arch/src/serial.rs
+++ b/arch/src/serial.rs
@@ -11,7 +11,7 @@ use std::path::PathBuf;
 use std::str::FromStr;
 use std::sync::Arc;
 
-use devices::{Bus, ProxyDevice, Serial, SerialDevice};
+use devices::{Bus, JailedDevice, Serial, SerialDevice};
 use io_jail::Minijail;
 use sync::Mutex;
 use sys_util::{read_raw_stdin, syslog, EventFd};
@@ -276,7 +276,7 @@ pub fn add_serial_devices(
         match serial_jail.as_ref() {
             Some(jail) => {
                 let com = Arc::new(Mutex::new(
-                    ProxyDevice::new(com, &jail, preserved_fds)
+                    JailedDevice::new(com, &jail, preserved_fds)
                         .map_err(DeviceRegistrationError::ProxyDeviceCreation)?,
                 ));
                 io_bus
diff --git a/devices/src/jailed.rs b/devices/src/jailed.rs
new file mode 100644
index 0000000..69eb1fe
--- /dev/null
+++ b/devices/src/jailed.rs
@@ -0,0 +1,177 @@
+// Copyright 2017 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+//! Runs hardware devices in child processes.
+
+use std::fmt::{self, Display};
+use std::io;
+use std::os::unix::io::{AsRawFd, RawFd};
+use std::time::Duration;
+
+use io_jail::Minijail;
+use libc::pid_t;
+use msg_socket::{MsgReceiver, MsgSender, MsgSocket};
+use sys_util::{error, net::UnixSeqpacket};
+
+use crate::proxy::*;
+use crate::BusDevice;
+
+/// Errors for proxy devices.
+#[derive(Debug)]
+pub enum Error {
+    ForkingJail(io_jail::Error),
+    Io(io::Error),
+}
+pub type Result<T> = std::result::Result<T, Error>;
+
+impl Display for Error {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        use self::Error::*;
+
+        match self {
+            ForkingJail(e) => write!(f, "Failed to fork jail process: {}", e),
+            Io(e) => write!(f, "IO error configuring proxy device {}.", e),
+        }
+    }
+}
+
+const SOCKET_TIMEOUT_MS: u64 = 2000;
+
+fn child_proc<D: BusDevice>(sock: UnixSeqpacket, device: &mut D) {
+    let mut running = true;
+    let sock = MsgSocket::<CommandResult, Command>::new(sock);
+
+    while running {
+        let cmd = match sock.recv() {
+            Ok(cmd) => cmd,
+            Err(err) => {
+                error!("child device process failed recv: {}", err);
+                break;
+            }
+        };
+
+        let res = match cmd {
+            Command::Read { len, offset } => {
+                let mut buffer = [0u8; 8];
+                device.read(offset, &mut buffer[0..len as usize]);
+                sock.send(&CommandResult::ReadResult(buffer))
+            }
+            Command::Write { len, offset, data } => {
+                let len = len as usize;
+                device.write(offset, &data[0..len]);
+                // Command::Write does not have a result.
+                Ok(())
+            }
+            Command::ReadConfig(idx) => {
+                let val = device.config_register_read(idx as usize);
+                sock.send(&CommandResult::ReadConfigResult(val))
+            }
+            Command::WriteConfig {
+                reg_idx,
+                offset,
+                len,
+                data,
+            } => {
+                let len = len as usize;
+                device.config_register_write(reg_idx as usize, offset as u64, &data[0..len]);
+                // Command::WriteConfig does not have a result.
+                Ok(())
+            }
+            Command::Shutdown => {
+                running = false;
+                sock.send(&CommandResult::Ok)
+            }
+        };
+        if let Err(e) = res {
+            error!("child device process failed send: {}", e);
+        }
+    }
+}
+
+pub struct JailedDevice {
+    proxy: ProxyDevice,
+    pid: pid_t,
+}
+
+impl JailedDevice {
+    /// Takes the given device and isolates it into another process via fork before returning.
+    ///
+    /// The forked process will automatically be terminated when this is dropped, so be sure to keep
+    /// a reference.
+    ///
+    /// # Arguments
+    /// * `device` - The device to isolate to another process.
+    /// * `jail` - The jail to use for isolating the given device.
+    /// * `keep_fds` - File descriptors that will be kept open in the child.
+    pub fn new<D: BusDevice>(
+        mut device: D,
+        jail: &Minijail,
+        mut keep_fds: Vec<RawFd>,
+    ) -> Result<JailedDevice> {
+        let debug_label = device.debug_label();
+        let (child_sock, parent_sock) = UnixSeqpacket::pair().map_err(Error::Io)?;
+
+        keep_fds.push(child_sock.as_raw_fd());
+        // Forking here is safe as long as the program is still single threaded.
+        let pid = unsafe {
+            match jail.fork(Some(&keep_fds)).map_err(Error::ForkingJail)? {
+                0 => {
+                    device.on_sandboxed();
+                    child_proc(child_sock, &mut device);
+
+                    // We're explicitly not using std::process::exit here to avoid the cleanup of
+                    // stdout/stderr globals. This can cause cascading panics and SIGILL if a worker
+                    // thread attempts to log to stderr after at_exit handlers have been run.
+                    // TODO(crbug.com/992494): Remove this once device shutdown ordering is clearly
+                    // defined.
+                    //
+                    // exit() is trivially safe.
+                    // ! Never returns
+                    libc::exit(0);
+                }
+                p => p,
+            }
+        };
+
+        parent_sock
+            .set_write_timeout(Some(Duration::from_millis(SOCKET_TIMEOUT_MS)))
+            .map_err(Error::Io)?;
+        parent_sock
+            .set_read_timeout(Some(Duration::from_millis(SOCKET_TIMEOUT_MS)))
+            .map_err(Error::Io)?;
+        Ok(JailedDevice {
+            proxy: ProxyDevice::new(
+                MsgSocket::<Command, CommandResult>::new(parent_sock),
+                debug_label,
+            ),
+            pid,
+        })
+    }
+
+    pub fn pid(&self) -> pid_t {
+        self.pid
+    }
+}
+
+impl BusDevice for JailedDevice {
+    fn debug_label(&self) -> String {
+        self.proxy.debug_label()
+    }
+
+    fn config_register_write(&mut self, reg_idx: usize, offset: u64, data: &[u8]) {
+        self.proxy.config_register_write(reg_idx, offset, data)
+    }
+
+    fn config_register_read(&self, reg_idx: usize) -> u32 {
+        self.proxy.config_register_read(reg_idx)
+    }
+
+    fn read(&mut self, offset: u64, data: &mut [u8]) {
+        self.proxy.read(offset, data)
+    }
+
+    fn write(&mut self, offset: u64, data: &[u8]) {
+        self.proxy.write(offset, data)
+    }
+}
diff --git a/devices/src/lib.rs b/devices/src/lib.rs
index c945e00..294a8cb 100644
--- a/devices/src/lib.rs
+++ b/devices/src/lib.rs
@@ -9,6 +9,7 @@ mod cmos;
 mod i8042;
 mod ioapic;
 mod irqchip;
+mod jailed;
 mod pci;
 mod pic;
 mod pit;
@@ -32,6 +33,8 @@ pub use self::cmos::Cmos;
 pub use self::i8042::I8042Device;
 pub use self::ioapic::{Ioapic, IOAPIC_BASE_ADDRESS, IOAPIC_MEM_LENGTH_BYTES};
 pub use self::irqchip::*;
+pub use self::jailed::Error as ProxyError;
+pub use self::jailed::JailedDevice;
 pub use self::pci::{
     Ac97Backend, Ac97Dev, Ac97Parameters, PciAddress, PciConfigIo, PciConfigMmio, PciDevice,
     PciDeviceError, PciInterruptPin, PciRoot, VfioPciDevice,
@@ -39,8 +42,6 @@ pub use self::pci::{
 pub use self::pic::Pic;
 pub use self::pit::{Pit, PitError};
 pub use self::pl030::Pl030;
-pub use self::proxy::Error as ProxyError;
-pub use self::proxy::ProxyDevice;
 pub use self::serial::Serial;
 pub use self::serial_device::SerialDevice;
 pub use self::usb::host_backend::host_backend_device_provider::HostBackendDeviceProvider;
diff --git a/devices/src/proxy.rs b/devices/src/proxy.rs
index cb7ebd6..cc31550 100644
--- a/devices/src/proxy.rs
+++ b/devices/src/proxy.rs
@@ -4,41 +4,13 @@
 
 //! Runs hardware devices in child processes.
 
-use std::fmt::{self, Display};
-use std::os::unix::io::{AsRawFd, RawFd};
-use std::time::Duration;
-use std::{self, io};
-
-use io_jail::{self, Minijail};
-use libc::{self, pid_t};
 use msg_socket::{MsgOnSocket, MsgReceiver, MsgSender, MsgSocket};
-use sys_util::{error, net::UnixSeqpacket};
+use sys_util::error;
 
 use crate::BusDevice;
 
-/// Errors for proxy devices.
-#[derive(Debug)]
-pub enum Error {
-    ForkingJail(io_jail::Error),
-    Io(io::Error),
-}
-pub type Result<T> = std::result::Result<T, Error>;
-
-impl Display for Error {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        use self::Error::*;
-
-        match self {
-            ForkingJail(e) => write!(f, "Failed to fork jail process: {}", e),
-            Io(e) => write!(f, "IO error configuring proxy device {}.", e),
-        }
-    }
-}
-
-const SOCKET_TIMEOUT_MS: u64 = 2000;
-
 #[derive(Debug, MsgOnSocket)]
-enum Command {
+pub enum Command {
     Read {
         len: u32,
         offset: u64,
@@ -59,128 +31,20 @@ enum Command {
 }
 
 #[derive(MsgOnSocket)]
-enum CommandResult {
+pub enum CommandResult {
     Ok,
     ReadResult([u8; 8]),
     ReadConfigResult(u32),
 }
 
-fn child_proc<D: BusDevice>(sock: UnixSeqpacket, device: &mut D) {
-    let mut running = true;
-    let sock = MsgSocket::<CommandResult, Command>::new(sock);
-
-    while running {
-        let cmd = match sock.recv() {
-            Ok(cmd) => cmd,
-            Err(err) => {
-                error!("child device process failed recv: {}", err);
-                break;
-            }
-        };
-
-        let res = match cmd {
-            Command::Read { len, offset } => {
-                let mut buffer = [0u8; 8];
-                device.read(offset, &mut buffer[0..len as usize]);
-                sock.send(&CommandResult::ReadResult(buffer))
-            }
-            Command::Write { len, offset, data } => {
-                let len = len as usize;
-                device.write(offset, &data[0..len]);
-                // Command::Write does not have a result.
-                Ok(())
-            }
-            Command::ReadConfig(idx) => {
-                let val = device.config_register_read(idx as usize);
-                sock.send(&CommandResult::ReadConfigResult(val))
-            }
-            Command::WriteConfig {
-                reg_idx,
-                offset,
-                len,
-                data,
-            } => {
-                let len = len as usize;
-                device.config_register_write(reg_idx as usize, offset as u64, &data[0..len]);
-                // Command::WriteConfig does not have a result.
-                Ok(())
-            }
-            Command::Shutdown => {
-                running = false;
-                sock.send(&CommandResult::Ok)
-            }
-        };
-        if let Err(e) = res {
-            error!("child device process failed send: {}", e);
-        }
-    }
-}
-
-/// Wraps an inner `BusDevice` that is run inside a child process via fork.
-///
-/// Because forks are very unfriendly to destructors and all memory mappings and file descriptors
-/// are inherited, this should be used as early as possible in the main process.
 pub struct ProxyDevice {
     sock: MsgSocket<Command, CommandResult>,
-    pid: pid_t,
     debug_label: String,
 }
 
 impl ProxyDevice {
-    /// Takes the given device and isolates it into another process via fork before returning.
-    ///
-    /// The forked process will automatically be terminated when this is dropped, so be sure to keep
-    /// a reference.
-    ///
-    /// # Arguments
-    /// * `device` - The device to isolate to another process.
-    /// * `jail` - The jail to use for isolating the given device.
-    /// * `keep_fds` - File descriptors that will be kept open in the child.
-    pub fn new<D: BusDevice>(
-        mut device: D,
-        jail: &Minijail,
-        mut keep_fds: Vec<RawFd>,
-    ) -> Result<ProxyDevice> {
-        let debug_label = device.debug_label();
-        let (child_sock, parent_sock) = UnixSeqpacket::pair().map_err(Error::Io)?;
-
-        keep_fds.push(child_sock.as_raw_fd());
-        // Forking here is safe as long as the program is still single threaded.
-        let pid = unsafe {
-            match jail.fork(Some(&keep_fds)).map_err(Error::ForkingJail)? {
-                0 => {
-                    device.on_sandboxed();
-                    child_proc(child_sock, &mut device);
-
-                    // We're explicitly not using std::process::exit here to avoid the cleanup of
-                    // stdout/stderr globals. This can cause cascading panics and SIGILL if a worker
-                    // thread attempts to log to stderr after at_exit handlers have been run.
-                    // TODO(crbug.com/992494): Remove this once device shutdown ordering is clearly
-                    // defined.
-                    //
-                    // exit() is trivially safe.
-                    // ! Never returns
-                    libc::exit(0);
-                }
-                p => p,
-            }
-        };
-
-        parent_sock
-            .set_write_timeout(Some(Duration::from_millis(SOCKET_TIMEOUT_MS)))
-            .map_err(Error::Io)?;
-        parent_sock
-            .set_read_timeout(Some(Duration::from_millis(SOCKET_TIMEOUT_MS)))
-            .map_err(Error::Io)?;
-        Ok(ProxyDevice {
-            sock: MsgSocket::<Command, CommandResult>::new(parent_sock),
-            pid,
-            debug_label,
-        })
-    }
-
-    pub fn pid(&self) -> pid_t {
-        self.pid
+    pub fn new(sock: MsgSocket<Command, CommandResult>, debug_label: String) -> Self {
+        Self { sock, debug_label }
     }
 
     /// Send a command that does not expect a response from the child device process.