From a8adff0ff14f66570a3aa86f6106b55081526be1 Mon Sep 17 00:00:00 2001 From: Zach Reizner Date: Tue, 13 Aug 2019 11:20:14 -0700 Subject: [PATCH] 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 Reviewed-by: Daniel Verkamp Tested-by: kokoro Tested-by: Zach Reizner Commit-Queue: Zach Reizner --- aarch64/src/lib.rs | 2 + arch/src/lib.rs | 68 +++++++++++++++++------ devices/src/lib.rs | 3 +- devices/src/proxy.rs | 43 ++++++++++++++- devices/src/serial.rs | 104 ++++++++++++++++++++++++++++------- seccomp/arm/serial.policy | 5 ++ seccomp/x86_64/serial.policy | 5 ++ src/linux.rs | 2 +- sys_util/src/lib.rs | 21 +++++++ x86_64/src/lib.rs | 17 ++++-- 10 files changed, 224 insertions(+), 46 deletions(-) create mode 100644 seccomp/arm/serial.policy create mode 100644 seccomp/x86_64/serial.policy diff --git a/aarch64/src/lib.rs b/aarch64/src/lib.rs index d21d70dde3..69ae76c731 100644 --- a/aarch64/src/lib.rs +++ b/aarch64/src/lib.rs @@ -195,6 +195,7 @@ impl arch::LinuxArch for AArch64 { mut components: VmComponents, _split_irqchip: bool, serial_parameters: &BTreeMap, + serial_jail: Option, create_devices: F, ) -> Result 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)?; diff --git a/arch/src/lib.rs b/arch/src/lib.rs index 28f1168375..45ed247c44 100644 --- a/arch/src/lib.rs +++ b/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>>, + pub stdio_serial: Option, pub exit_evt: EventFd, pub vcpus: Vec, pub vcpu_affinity: Vec, @@ -80,6 +80,7 @@ pub trait LinuxArch { components: VmComponents, split_irqchip: bool, serial_parameters: &BTreeMap, + serial_jail: Option, create_devices: F, ) -> Result 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, -) -> Result<(Option, Option>>), DeviceRegistrationError> { + serial_jail: Option, +) -> Result<(Option, Option), 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)); + } + } } } diff --git a/devices/src/lib.rs b/devices/src/lib.rs index 512d08b9e0..d96ce19ae0 100644 --- a/devices/src/lib.rs +++ b/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; diff --git a/devices/src/proxy.rs b/devices/src/proxy.rs index b97cccad43..3770779aab 100644 --- a/devices/src/proxy.rs +++ b/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( + sock: UnixSeqpacket, + device: &mut D, + mut user_command: F, +) { let mut running = true; let sock = MsgSocket::::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( + device: D, + jail: &Minijail, + keep_fds: Vec, + ) -> Result { + 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( mut device: D, jail: &Minijail, mut keep_fds: Vec, + user_command: F, ) -> Result { 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 { let res = self.sock.send(&cmd); if let Err(e) = res { diff --git a/devices/src/serial.rs b/devices/src/serial.rs index 5ecefd7d35..9485497069 100644 --- a/devices/src/serial.rs +++ b/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 { + /// * `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, + ) -> std::result::Result { + 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>), + #[doc(hidden)] + Remote { + input: File, + proxy: Arc>, + }, +} + +impl SerialInput { + /// Creates a `SerialInput` that trivially forwards queued bytes to the device in the local + /// process. + pub fn new_local(serial: Arc>) -> 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>) -> 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 } diff --git a/seccomp/arm/serial.policy b/seccomp/arm/serial.policy new file mode 100644 index 0000000000..f9e98f0e8d --- /dev/null +++ b/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 diff --git a/seccomp/x86_64/serial.policy b/seccomp/x86_64/serial.policy new file mode 100644 index 0000000000..f9e98f0e8d --- /dev/null +++ b/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 diff --git a/src/linux.rs b/src/linux.rs index f6af0128c6..bf78eb2906 100644 --- a/src/linux.rs +++ b/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"); } diff --git a/sys_util/src/lib.rs b/sys_util/src/lib.rs index 7cba7eb70a..0dab89dc28 100644 --- a/sys_util/src/lib.rs +++ b/sys_util/src/lib.rs @@ -321,3 +321,24 @@ pub fn validate_raw_fd(raw_fd: RawFd) -> Result { } 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 +} diff --git a/x86_64/src/lib.rs b/x86_64/src/lib.rs index a047b8c011..88c91f98dd 100644 --- a/x86_64/src/lib.rs +++ b/x86_64/src/lib.rs @@ -304,6 +304,7 @@ impl arch::LinuxArch for X8664arch { mut components: VmComponents, split_irqchip: bool, serial_parameters: &BTreeMap, + serial_jail: Option, create_devices: F, ) -> Result 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, - ) -> Result<(Option, Option>>)> { + serial_jail: Option, + ) -> Result<(Option, Option)> { 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)?;