devices: jail serial device

This change plumbs the jail throughout the arch specific device creation
process. It also adds a custom callback support for the ProxyDevice so
that the main process can interrupt the child serial process when it has
incoming bytes.

TEST=crosvm run
BUG=None

Change-Id: I6af7d2cb0acbba9bf42eaeeb294cee2bce4a1f36
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/1752589
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Reviewed-by: Daniel Verkamp <dverkamp@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Tested-by: Zach Reizner <zachr@chromium.org>
Commit-Queue: Zach Reizner <zachr@chromium.org>

aarch64/src/lib.rs

@@ -195,6 +195,7 @@ impl arch::LinuxArch for AArch64 {
         mut components: VmComponents,
         _split_irqchip: bool,
         serial_parameters: &BTreeMap<u8, SerialParameters>,
+        serial_jail: Option<Minijail>,
         create_devices: F,
     ) -> Result<RunnableLinuxVm>
     where
@@ -254,6 +255,7 @@ impl arch::LinuxArch for AArch64 {
             &com_evt_1_3,
             &com_evt_2_4,
             &serial_parameters,
+            serial_jail,
         )
         .map_err(Error::CreateSerialDevices)?;
 

arch/src/lib.rs

@@ -16,13 +16,13 @@ use std::sync::Arc;
 use devices::virtio::VirtioDevice;
 use devices::{
     Bus, BusDevice, BusError, PciDevice, PciDeviceError, PciInterruptPin, PciRoot, ProxyDevice,
-    Serial, SerialParameters, DEFAULT_SERIAL_PARAMS, SERIAL_ADDR,
+    SerialInput, SerialParameters, DEFAULT_SERIAL_PARAMS, SERIAL_ADDR,
 };
 use io_jail::Minijail;
 use kvm::{IoeventAddress, Kvm, Vcpu, Vm};
 use resources::SystemAllocator;
 use sync::Mutex;
-use sys_util::{syslog, EventFd, GuestAddress, GuestMemory, GuestMemoryError};
+use sys_util::{pipe, poll_in, syslog, warn, EventFd, GuestAddress, GuestMemory, GuestMemoryError};
 
 pub enum VmImage {
     Kernel(File),
@@ -47,7 +47,7 @@ pub struct RunnableLinuxVm {
     pub vm: Vm,
     pub kvm: Kvm,
     pub resources: SystemAllocator,
-    pub stdio_serial: Option<Arc<Mutex<Serial>>>,
+    pub stdio_serial: Option<SerialInput>,
     pub exit_evt: EventFd,
     pub vcpus: Vec<Vcpu>,
     pub vcpu_affinity: Vec<usize>,
@@ -80,6 +80,7 @@ pub trait LinuxArch {
         components: VmComponents,
         split_irqchip: bool,
         serial_parameters: &BTreeMap<u8, SerialParameters>,
+        serial_jail: Option<Minijail>,
         create_devices: F,
     ) -> Result<RunnableLinuxVm, Self::Error>
     where
@@ -103,7 +104,9 @@ pub enum DeviceRegistrationError {
     AllocateIrq,
     /// Could not create the mmio device to wrap a VirtioDevice.
     CreateMmioDevice(sys_util::Error),
-    //  Unable to create serial device from serial parameters
+    // Unable to create a pipe.
+    CreatePipe(sys_util::Error),
+    // Unable to create serial device from serial parameters
     CreateSerialDevice(devices::SerialError),
     /// Could not create an event fd.
     EventFdCreate(sys_util::Error),
@@ -134,6 +137,7 @@ impl Display for DeviceRegistrationError {
             AllocateDeviceAddrs(e) => write!(f, "Allocating device addresses: {}", e),
             AllocateIrq => write!(f, "Allocating IRQ number"),
             CreateMmioDevice(e) => write!(f, "failed to create mmio device: {}", e),
+            CreatePipe(e) => write!(f, "failed to create pipe: {}", e),
             CreateSerialDevice(e) => write!(f, "failed to create serial device: {}", e),
             Cmdline(e) => write!(f, "unable to add device to kernel command line: {}", e),
             EventFdCreate(e) => write!(f, "failed to create eventfd: {}", e),
@@ -243,7 +247,8 @@ pub fn add_serial_devices(
     com_evt_1_3: &EventFd,
     com_evt_2_4: &EventFd,
     serial_parameters: &BTreeMap<u8, SerialParameters>,
-) -> Result<(Option<u8>, Option<Arc<Mutex<Serial>>>), DeviceRegistrationError> {
+    serial_jail: Option<Minijail>,
+) -> Result<(Option<u8>, Option<SerialInput>), DeviceRegistrationError> {
     let mut stdio_serial_num = None;
     let mut stdio_serial = None;
 
@@ -260,21 +265,52 @@ pub fn add_serial_devices(
             .get(&(x + 1))
             .unwrap_or(&DEFAULT_SERIAL_PARAMS[x as usize]);
 
-        let com = Arc::new(Mutex::new(
-            param
-                .create_serial_device(&com_evt)
-                .map_err(DeviceRegistrationError::CreateSerialDevice)?,
-        ));
-        io_bus
-            .insert(com.clone(), SERIAL_ADDR[x as usize], 0x8, false)
-            .unwrap();
-
         if param.console {
             stdio_serial_num = Some(x + 1);
         }
 
-        if param.stdin {
-            stdio_serial = Some(com.clone());
+        let mut preserved_fds = Vec::new();
+        let com = param
+            .create_serial_device(&com_evt, &mut preserved_fds)
+            .map_err(DeviceRegistrationError::CreateSerialDevice)?;
+
+        match serial_jail.as_ref() {
+            Some(jail) => {
+                let (rx, tx) = pipe(true).map_err(DeviceRegistrationError::CreatePipe)?;
+                preserved_fds.push(rx.as_raw_fd());
+                let com = Arc::new(Mutex::new(
+                    ProxyDevice::new_with_user_command(com, &jail, preserved_fds, move |serial| {
+                        let mut rx_buf = [0u8; 32];
+                        // This loop may end up stealing bytes from future user callbacks, so we
+                        // check to make sure the pipe is readable so as not to block the proxy
+                        // device's loop.
+                        while poll_in(&rx) {
+                            if let Ok(count) = (&rx).read(&mut rx_buf) {
+                                if let Err(e) = serial.queue_input_bytes(&rx_buf[..count]) {
+                                    warn!("failed to queue bytes into serial device {}: {}", x, e);
+                                }
+                            }
+                        }
+                    })
+                    .map_err(DeviceRegistrationError::ProxyDeviceCreation)?,
+                ));
+                io_bus
+                    .insert(com.clone(), SERIAL_ADDR[x as usize], 0x8, false)
+                    .unwrap();
+                if param.stdin {
+                    stdio_serial = Some(SerialInput::new_remote(tx, com));
+                }
+            }
+            None => {
+                let com = Arc::new(Mutex::new(com));
+                io_bus
+                    .insert(com.clone(), SERIAL_ADDR[x as usize], 0x8, false)
+                    .unwrap();
+
+                if param.stdin {
+                    stdio_serial = Some(SerialInput::new_local(com));
+                }
+            }
         }
     }
 

devices/src/lib.rs

@@ -38,7 +38,8 @@ pub use self::proxy::Error as ProxyError;
 pub use self::proxy::ProxyDevice;
 pub use self::serial::Error as SerialError;
 pub use self::serial::{
-    get_serial_tty_string, Serial, SerialParameters, SerialType, DEFAULT_SERIAL_PARAMS, SERIAL_ADDR,
+    get_serial_tty_string, Serial, SerialInput, SerialParameters, SerialType,
+    DEFAULT_SERIAL_PARAMS, SERIAL_ADDR,
 };
 pub use self::usb::host_backend::host_backend_device_provider::HostBackendDeviceProvider;
 pub use self::usb::xhci::xhci_controller::XhciController;

devices/src/proxy.rs

@@ -56,6 +56,7 @@ enum Command {
         data: [u8; 4],
     },
     Shutdown,
+    RunUserCommand,
 }
 
 #[derive(MsgOnSocket)]
@@ -65,7 +66,11 @@ enum CommandResult {
     ReadConfigResult(u32),
 }
 
-fn child_proc(sock: UnixSeqpacket, device: &mut dyn BusDevice) {
+fn child_proc<D: BusDevice, F: FnMut(&mut D)>(
+    sock: UnixSeqpacket,
+    device: &mut D,
+    mut user_command: F,
+) {
     let mut running = true;
     let sock = MsgSocket::<CommandResult, Command>::new(sock);
 
@@ -107,6 +112,10 @@ fn child_proc(sock: UnixSeqpacket, device: &mut dyn BusDevice) {
                 running = false;
                 sock.send(&CommandResult::Ok)
             }
+            Command::RunUserCommand => {
+                user_command(device);
+                sock.send(&CommandResult::Ok)
+            }
         };
         if let Err(e) = res {
             error!("child device process failed send: {}", e);
@@ -132,11 +141,34 @@ impl ProxyDevice {
     ///
     /// # Arguments
     /// * `device` - The device to isolate to another process.
-    /// * `keep_fds` - File descriptors that will be kept open in the child
+    /// * `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>(
+        device: D,
+        jail: &Minijail,
+        keep_fds: Vec<RawFd>,
+    ) -> Result<ProxyDevice> {
+        Self::new_with_user_command(device, jail, keep_fds, |_| {})
+    }
+
+    /// Similar to `ProxyDevice::new`, but adds an additional custom command to be run in the forked
+    /// process when `run_user_command` is called.
+    ///
+    /// Note that the custom command closure is run in the main thread of the child process, which
+    /// also services `BusDevice` requests. Therefore, do not run blocking calls in the closure
+    /// without a timeout, or you will block any VCPU which touches this device, and every other
+    /// thread which needs to lock this device's mutex.
+    ///
+    /// # 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.
+    /// * `user_command` - Closure to be run in the forked process.
+    pub fn new_with_user_command<D: BusDevice, F: FnMut(&mut D)>(
         mut device: D,
         jail: &Minijail,
         mut keep_fds: Vec<RawFd>,
+        user_command: F,
     ) -> Result<ProxyDevice> {
         let debug_label = device.debug_label();
         let (child_sock, parent_sock) = UnixSeqpacket::pair().map_err(Error::Io)?;
@@ -147,7 +179,7 @@ impl ProxyDevice {
             match jail.fork(Some(&keep_fds)).map_err(Error::ForkingJail)? {
                 0 => {
                     device.on_sandboxed();
-                    child_proc(child_sock, &mut device);
+                    child_proc(child_sock, &mut device, user_command);
 
                     // 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
@@ -180,6 +212,11 @@ impl ProxyDevice {
         self.pid
     }
 
+    /// Runs the callback given in `new_with_custom_command` in the child device process.
+    pub fn run_user_command(&self) {
+        self.sync_send(Command::RunUserCommand);
+    }
+
     fn sync_send(&self, cmd: Command) -> Option<CommandResult> {
         let res = self.sock.send(&cmd);
         if let Err(e) = res {

devices/src/serial.rs

@@ -5,13 +5,17 @@
 use std::collections::VecDeque;
 use std::fmt::{self, Display};
 use std::fs::File;
-use std::io::{self, stdout};
+use std::io::{self, stdout, Write};
+use std::os::unix::io::{AsRawFd, RawFd};
 use std::path::PathBuf;
 use std::str::FromStr;
+use std::sync::Arc;
 
+use sync::Mutex;
 use sys_util::{error, syslog, EventFd, Result};
 
 use crate::BusDevice;
+use crate::ProxyDevice;
 
 const LOOP_SIZE: usize = 0x40;
 
@@ -135,28 +139,38 @@ impl SerialParameters {
     ///
     /// # Arguments
     /// * `evt_fd` - eventfd used for interrupt events
-    pub fn create_serial_device(&self, evt_fd: &EventFd) -> std::result::Result<Serial, Error> {
+    /// * `keep_fds` - Vector of FDs required by this device if it were sandboxed in a child
+    ///                process. `evt_fd` will always be added to this vector by this function.
+    pub fn create_serial_device(
+        &self,
+        evt_fd: &EventFd,
+        keep_fds: &mut Vec<RawFd>,
+    ) -> std::result::Result<Serial, Error> {
+        let evt_fd = evt_fd.try_clone().map_err(Error::CloneEventFd)?;
+        keep_fds.push(evt_fd.as_raw_fd());
         match self.type_ {
-            SerialType::Stdout => Ok(Serial::new_out(
-                evt_fd.try_clone().map_err(Error::CloneEventFd)?,
-                Box::new(stdout()),
-            )),
-            SerialType::Sink => Ok(Serial::new_sink(
-                evt_fd.try_clone().map_err(Error::CloneEventFd)?,
-            )),
-            SerialType::Syslog => Ok(Serial::new_out(
-                evt_fd.try_clone().map_err(Error::CloneEventFd)?,
-                Box::new(syslog::Syslogger::new(
-                    syslog::Priority::Info,
-                    syslog::Facility::Daemon,
-                )),
-            )),
+            SerialType::Stdout => {
+                keep_fds.push(stdout().as_raw_fd());
+                Ok(Serial::new_out(evt_fd, Box::new(stdout())))
+            }
+            SerialType::Sink => Ok(Serial::new_sink(evt_fd)),
+            SerialType::Syslog => {
+                syslog::push_fds(keep_fds);
+                Ok(Serial::new_out(
+                    evt_fd,
+                    Box::new(syslog::Syslogger::new(
+                        syslog::Priority::Info,
+                        syslog::Facility::Daemon,
+                    )),
+                ))
+            }
             SerialType::File => match &self.path {
                 None => Err(Error::PathRequired),
-                Some(path) => Ok(Serial::new_out(
-                    evt_fd.try_clone().map_err(Error::CloneEventFd)?,
-                    Box::new(File::create(path.as_path()).map_err(Error::FileError)?),
-                )),
+                Some(path) => {
+                    let file = File::create(path.as_path()).map_err(Error::FileError)?;
+                    keep_fds.push(file.as_raw_fd());
+                    Ok(Serial::new_out(evt_fd, Box::new(file)))
+                }
             },
             SerialType::UnixSocket => Err(Error::Unimplemented(SerialType::UnixSocket)),
         }
@@ -213,6 +227,50 @@ pub fn get_serial_tty_string(stdio_serial_num: u8) -> String {
     }
 }
 
+/// A type for queueing input bytes to a serial device that abstracts if the device is local or part
+/// of a sandboxed device process.
+pub enum SerialInput {
+    #[doc(hidden)]
+    Local(Arc<Mutex<Serial>>),
+    #[doc(hidden)]
+    Remote {
+        input: File,
+        proxy: Arc<Mutex<ProxyDevice>>,
+    },
+}
+
+impl SerialInput {
+    /// Creates a `SerialInput` that trivially forwards queued bytes to the device in the local
+    /// process.
+    pub fn new_local(serial: Arc<Mutex<Serial>>) -> SerialInput {
+        SerialInput::Local(serial)
+    }
+
+    /// Creates a `SerialInput` that will forward bytes via `input` and will wake up the child
+    /// device process which contains the serial device so that it may process the other end of the
+    /// `input` pipe. This will use the `ProxyDevice::run_user_command` method, so the user command
+    /// closure (registered with `ProxyDevice::new_with_user_command`) should own the other side of
+    /// `input` and read from the pipe whenever the closure is run.
+    pub fn new_remote(input: File, proxy: Arc<Mutex<ProxyDevice>>) -> SerialInput {
+        SerialInput::Remote { input, proxy }
+    }
+
+    /// Just like `Serial::queue_input_bytes`, but abstracted over local and sandboxed serial
+    /// devices. In the case that the serial device is sandboxed, this method will also trigger the
+    /// user command in the `ProxyDevice`'s child process.
+    pub fn queue_input_bytes(&self, bytes: &[u8]) -> Result<()> {
+        match self {
+            SerialInput::Local(device) => device.lock().queue_input_bytes(bytes),
+            SerialInput::Remote { input, proxy } => {
+                let mut input = input;
+                input.write_all(bytes)?;
+                proxy.lock().run_user_command();
+                Ok(())
+            }
+        }
+    }
+}
+
 /// Emulates serial COM ports commonly seen on x86 I/O ports 0x3f8/0x2f8/0x3e8/0x2e8.
 ///
 /// This can optionally write the guest's output to a Write trait object. To send input to the
@@ -268,6 +326,12 @@ impl Serial {
         Ok(())
     }
 
+    /// Gets the interrupt eventfd used to interrupt the driver when it needs to respond to this
+    /// device.
+    pub fn interrupt_eventfd(&self) -> &EventFd {
+        &self.interrupt_evt
+    }
+
     fn is_dlab_set(&self) -> bool {
         (self.line_control & 0x80) != 0
     }

seccomp/arm/serial.policy

@@ -0,0 +1,5 @@
+# Copyright 2019 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.
+
+@include /usr/share/policy/crosvm/common_device.policy

seccomp/x86_64/serial.policy

@@ -0,0 +1,5 @@
+# Copyright 2019 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.
+
+@include /usr/share/policy/crosvm/common_device.policy

src/linux.rs

@@ -1352,6 +1352,7 @@ pub fn run_config(cfg: Config) -> Result<()> {
         components,
         cfg.split_irqchip,
         &cfg.serial_parameters,
+        simple_jail(&cfg, "serial.policy")?,
         |mem, vm, sys_allocator, exit_evt| {
             create_devices(
                 &cfg,
@@ -1580,7 +1581,6 @@ fn run_control(
                         Ok(count) => {
                             if let Some(ref stdio_serial) = linux.stdio_serial {
                                 stdio_serial
-                                    .lock()
                                     .queue_input_bytes(&out[..count])
                                     .expect("failed to queue bytes into serial port");
                             }

sys_util/src/lib.rs

@@ -321,3 +321,24 @@ pub fn validate_raw_fd(raw_fd: RawFd) -> Result<RawFd> {
     }
     Ok(dup_fd as RawFd)
 }
+
+/// Utility function that returns true if the given FD is readable without blocking.
+///
+/// On an error, such as an invalid or incompatible FD, this will return false, which can not be
+/// distinguished from a non-ready to read FD.
+pub fn poll_in(fd: &AsRawFd) -> bool {
+    let mut fds = libc::pollfd {
+        fd: fd.as_raw_fd(),
+        events: libc::POLLIN,
+        revents: 0,
+    };
+    // Safe because we give a valid pointer to a list (of 1) FD and check the return value.
+    let ret = unsafe { libc::poll(&mut fds, 1, 0) };
+    // An error probably indicates an invalid FD, or an FD that can't be polled. Returning false in
+    // that case is probably correct as such an FD is unlikely to be readable, although there are
+    // probably corner cases in which that is wrong.
+    if ret == -1 {
+        return false;
+    }
+    fds.revents & libc::POLLIN != 0
+}

x86_64/src/lib.rs

@@ -304,6 +304,7 @@ impl arch::LinuxArch for X8664arch {
         mut components: VmComponents,
         split_irqchip: bool,
         serial_parameters: &BTreeMap<u8, SerialParameters>,
+        serial_jail: Option<Minijail>,
         create_devices: F,
     ) -> Result<RunnableLinuxVm>
     where
@@ -366,7 +367,7 @@ impl arch::LinuxArch for X8664arch {
         )?;
 
         let (stdio_serial_num, stdio_serial) =
-            Self::setup_serial_devices(&mut vm, &mut io_bus, &serial_parameters)?;
+            Self::setup_serial_devices(&mut vm, &mut io_bus, serial_parameters, serial_jail)?;
 
         match components.vm_image {
             VmImage::Bios(ref mut bios) => Self::load_bios(&mem, bios)?,
@@ -715,13 +716,19 @@ impl X8664arch {
         vm: &mut Vm,
         io_bus: &mut devices::Bus,
         serial_parameters: &BTreeMap<u8, SerialParameters>,
-    ) -> Result<(Option<u8>, Option<Arc<Mutex<devices::Serial>>>)> {
+        serial_jail: Option<Minijail>,
+    ) -> Result<(Option<u8>, Option<devices::SerialInput>)> {
         let com_evt_1_3 = EventFd::new().map_err(Error::CreateEventFd)?;
         let com_evt_2_4 = EventFd::new().map_err(Error::CreateEventFd)?;
 
-        let (stdio_serial_num, stdio_serial) =
-            arch::add_serial_devices(io_bus, &com_evt_1_3, &com_evt_2_4, &serial_parameters)
-                .map_err(Error::CreateSerialDevices)?;
+        let (stdio_serial_num, stdio_serial) = arch::add_serial_devices(
+            io_bus,
+            &com_evt_1_3,
+            &com_evt_2_4,
+            &serial_parameters,
+            serial_jail,
+        )
+        .map_err(Error::CreateSerialDevices)?;
 
         vm.register_irqfd(&com_evt_1_3, X86_64_SERIAL_1_3_IRQ)
             .map_err(Error::RegisterIrqfd)?;