fix bad syscall handling

This commit brings the following changes to our bad syscall handling:
- It whitelists fcntl(*, FCNTL_F_SETFD, FCNTL_FD_CLOEXEC), which
  appears to be invoked by the Rust File::open() method.
- We now terminate the Firecracker process whenever a bad syscall is
  intercepted (we still attempt to flush metrics and log an error
  message).
- To avoid getting some unnecessary SIGSYS signals (or whitelisting
  additional syscalls), the vmm::stop() method calls process::exit()
  immediately after flushing the metrics one last time. We no longer
  explicitly bring down the other Firecracker threads.

Signed-off-by: Alexandru Agache <[email protected]>

Author: alexandruag

CHANGELOG.md

@@ -9,8 +9,9 @@
 
 ### Changed
 - Log the app version when the `Logger` is initialized.
-
 - Pretty print panic information.
+- Firecracker terminates with exit code 148 when a non-whitelisted syscall
+is intercepted.
 
 ### Fixed
 

logger/Cargo.toml

@@ -5,7 +5,7 @@ authors = ["Amazon firecracker team <[email protected]>"]
 
 [dependencies]
 chrono = ">=0.4"
-lazy_static = ">=0.2"
+lazy_static = ">=1.2"
 libc = ">=0.2.39"
 log = { version = "0.4", features = ["std"] }
 serde = ">=1.0.27"

src/main.rs

@@ -21,6 +21,7 @@ use clap::{App, Arg};
 use std::io::ErrorKind;
 use std::panic;
 use std::path::PathBuf;
+use std::process;
 use std::sync::mpsc::channel;
 use std::sync::{Arc, RwLock};
 
@@ -38,8 +39,11 @@ fn main() {
         .preinit(Some(DEFAULT_INSTANCE_ID.to_string()))
         .expect("Failed to register logger");
 
-    // If the signal handler can't be set, it's OK to panic.
-    vmm::setup_sigsys_handler().expect("Failed to register signal handler");
+    if let Err(e) = vmm::setup_sigsys_handler() {
+        error!("Failed to register signal handler: {}", e);
+        process::exit(vmm::FC_EXIT_CODE_GENERIC_ERROR as i32);
+    }
+
     // Start firecracker by setting up a panic hook, which will be called before
     // terminating as we're building with panic = "abort".
     // It's worth noting that the abort is caused by sending a SIG_ABORT signal to the process.
@@ -83,7 +87,15 @@ fn main() {
         .expect("Missing argument: api_sock");
 
     let mut instance_id = String::from(DEFAULT_INSTANCE_ID);
+
+    // We disable seccomp filtering when testing, because when running the test_gnutests
+    // integration test from test_unittests.py, an invalid syscall is issued, and we crash
+    // otherwise.
+    #[cfg(not(test))]
     let mut seccomp_level = seccomp::SECCOMP_LEVEL_ADVANCED;
+    #[cfg(test)]
+    let mut seccomp_level = seccomp::SECCOMP_LEVEL_NONE;
+
     let mut start_time_us = None;
     let mut start_time_cpu_us = None;
     let mut is_jailed = false;

vmm/src/default_syscalls/x86_64.rs

@@ -17,6 +17,7 @@ pub const ALLOWED_SYSCALLS: &[i64] = &[
     libc::SYS_epoll_pwait,
     libc::SYS_exit,
     libc::SYS_exit_group,
+    libc::SYS_fcntl,
     libc::SYS_fstat,
     libc::SYS_futex,
     libc::SYS_ioctl,
@@ -41,10 +42,12 @@ const EPOLL_CTL_ADD: u64 = 1;
 const EPOLL_CTL_DEL: u64 = 2;
 
 // See include/uapi/asm-generic/fcntl.h in the kernel code.
+const FCNTL_FD_CLOEXEC: u64 = 1;
+const FCNTL_F_SETFD: u64 = 2;
+const O_CLOEXEC: u64 = 0x02000000;
+const O_NONBLOCK: u64 = 0x00004000;
 const O_RDONLY: u64 = 0x00000000;
 const O_RDWR: u64 = 0x00000002;
-const O_NONBLOCK: u64 = 0x00004000;
-const O_CLOEXEC: u64 = 0x02000000;
 
 // See include/uapi/linux/futex.h in the kernel code.
 const FUTEX_WAIT: u64 = 0;
@@ -163,6 +166,19 @@ pub fn default_context() -> Result<SeccompFilterContext, Error> {
                 libc::SYS_exit_group,
                 (0, vec![SeccompRule::new(vec![], SeccompAction::Allow)]),
             ),
+            (
+                libc::SYS_fcntl,
+                (
+                    0,
+                    vec![SeccompRule::new(
+                        vec![
+                            SeccompCondition::new(1, SeccompCmpOp::Eq, FCNTL_F_SETFD)?,
+                            SeccompCondition::new(2, SeccompCmpOp::Eq, FCNTL_FD_CLOEXEC)?,
+                        ],
+                        SeccompAction::Allow,
+                    )],
+                ),
+            ),
             (
                 libc::SYS_fstat,
                 (0, vec![SeccompRule::new(vec![], SeccompAction::Allow)]),

vmm/src/lib.rs

@@ -53,7 +53,7 @@ use std::fs::{metadata, File, OpenOptions};
 use std::os::unix::io::{AsRawFd, RawFd};
 use std::path::PathBuf;
 use std::result;
-use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering, ATOMIC_USIZE_INIT};
+use std::sync::atomic::{AtomicUsize, Ordering, ATOMIC_USIZE_INIT};
 use std::sync::mpsc::{channel, Receiver, Sender, TryRecvError};
 use std::sync::{Arc, Barrier, RwLock};
 use std::thread;
@@ -75,7 +75,7 @@ use logger::{AppInfo, Level, LogOption, Metric, LOGGER, METRICS};
 use memory_model::{GuestAddress, GuestMemory};
 use serde_json::Value;
 pub use sigsys_handler::setup_sigsys_handler;
-use sys_util::{register_signal_handler, EventFd, Killable, Terminal};
+use sys_util::{register_signal_handler, EventFd, Terminal};
 use vm_control::VmResponse;
 use vmm_config::boot_source::{BootSourceConfig, BootSourceConfigError};
 use vmm_config::drive::{BlockDeviceConfig, BlockDeviceConfigs, DriveError};
@@ -96,6 +96,15 @@ const WRITE_METRICS_PERIOD_SECONDS: u64 = 60;
 static START_INSTANCE_REQUEST_TS: AtomicUsize = ATOMIC_USIZE_INIT;
 static START_INSTANCE_REQUEST_CPU_TS: AtomicUsize = ATOMIC_USIZE_INIT;
 
+/// Success exit code.
+pub const FC_EXIT_CODE_OK: u8 = 0;
+/// Generic error exit code.
+pub const FC_EXIT_CODE_GENERIC_ERROR: u8 = 1;
+/// Generic exit code for an error considered not possible to occur if the program logic is sound.
+pub const FC_EXIT_CODE_UNEXPECTED_ERROR: u8 = 2;
+/// Firecracker was shut down after intercepting a restricted system call.
+pub const FC_EXIT_CODE_BAD_SYSCALL: u8 = 148;
+
 /// Errors associated with the VMM internal logic. These errors cannot be generated by direct user
 /// input, but can result from bad configuration of the host (for example if Firecracker doesn't
 /// have permissions to open the KVM fd).
@@ -340,7 +349,6 @@ impl MaybeHandler {
 }
 
 struct EpollEvent<T: AsRawFd> {
-    dispatch_index: u64,
     fd: T,
 }
 
@@ -428,23 +436,7 @@ impl EpollContext {
         .map_err(Error::EpollFd)?;
         self.dispatch_table.push(Some(token));
 
-        Ok(EpollEvent { dispatch_index, fd })
-    }
-
-    fn remove_event<T>(&mut self, epoll_event: EpollEvent<T>) -> Result<()>
-    where
-        T: AsRawFd,
-    {
-        epoll::ctl(
-            self.epoll_raw_fd,
-            epoll::ControlOptions::EPOLL_CTL_DEL,
-            epoll_event.fd.as_raw_fd(),
-            epoll::Event::new(epoll::Events::EPOLLIN, epoll_event.dispatch_index),
-        )
-        .map_err(Error::EpollFd)?;
-        self.dispatch_table[epoll_event.dispatch_index as usize] = None;
-
-        Ok(())
+        Ok(EpollEvent { fd })
     }
 
     fn allocate_tokens(&mut self, count: usize) -> (u64, Sender<Box<EpollHandler>>) {
@@ -528,7 +520,6 @@ struct Vmm {
     // Guest VM core resources.
     guest_memory: Option<GuestMemory>,
     kernel_config: Option<KernelConfig>,
-    kill_signaled: Option<Arc<AtomicBool>>,
     vcpu_handles: Option<Vec<thread::JoinHandle<()>>>,
     exit_evt: Option<EpollEvent<EventFd>>,
     vm: Vm,
@@ -591,7 +582,6 @@ impl Vmm {
             shared_info: api_shared_info,
             guest_memory: None,
             kernel_config: None,
-            kill_signaled: None,
             vcpu_handles: None,
             exit_evt: None,
             vm,
@@ -910,9 +900,6 @@ impl Vmm {
         self.vcpu_handles = Some(Vec::with_capacity(vcpu_count as usize));
         // It is safe to unwrap since it's set just above.
         let vcpu_handles = self.vcpu_handles.as_mut().unwrap();
-        self.kill_signaled = Some(Arc::new(AtomicBool::new(false)));
-        // It is safe to unwrap since it's set just above.
-        let kill_signaled = self.kill_signaled.as_mut().unwrap();
 
         let vcpu_thread_barrier = Arc::new(Barrier::new((vcpu_count + 1) as usize));
 
@@ -925,7 +912,6 @@ impl Vmm {
                 .as_ref()
                 .ok_or(StartMicrovmError::DeviceManager)?;
             let mmio_bus = device_manager.bus.clone();
-            let kill_signaled = kill_signaled.clone();
             let vcpu_thread_barrier = vcpu_thread_barrier.clone();
             // If the lock is poisoned, it's OK to panic.
             let vcpu_exit_evt = self
@@ -1050,10 +1036,6 @@ impl Vmm {
                                 },
                                 _ => (),
                             }
-
-                            if kill_signaled.load(Ordering::SeqCst) {
-                                break;
-                            }
                         }
 
                         // Nothing we need do for the success case.
@@ -1206,36 +1188,6 @@ impl Vmm {
     fn stop(&mut self, exit_code: i32) {
         info!("Vmm is stopping.");
 
-        if let Some(v) = self.kill_signaled.take() {
-            v.store(true, Ordering::SeqCst);
-        };
-
-        if let Some(handles) = self.vcpu_handles.take() {
-            for handle in handles {
-                match handle.kill(VCPU_RTSIG_OFFSET) {
-                    Ok(_) => {
-                        if let Err(e) = handle.join() {
-                            warn!("Failed to join vcpu thread: {:?}", e);
-                            METRICS.vcpu.failures.inc();
-                        }
-                    }
-                    Err(e) => {
-                        METRICS.vcpu.failures.inc();
-                        warn!("Failed to kill vcpu thread: {:?}", e)
-                    }
-                }
-            }
-        };
-
-        if let Some(evt) = self.exit_evt.take() {
-            if let Err(e) = self.epoll_context.remove_event(evt) {
-                warn!(
-                    "Cannot remove the exit event from the Epoll Context. {:?}",
-                    e
-                );
-            }
-        }
-
         if let Err(e) = self.epoll_context.disable_stdin_event() {
             warn!("Cannot disable the STDIN event. {:?}", e);
         }
@@ -1294,7 +1246,7 @@ impl Vmm {
                                 }
                                 None => warn!("leftover exit-evt in epollcontext!"),
                             }
-                            self.stop(0);
+                            self.stop(FC_EXIT_CODE_OK as i32);
                         }
                         EpollDispatch::Stdin => {
                             let mut out = [0u8; 64];
@@ -1834,11 +1786,11 @@ pub fn start_vmm_thread(
             match vmm.run_control() {
                 Ok(()) => {
                     info!("Gracefully terminated VMM control loop");
-                    vmm.stop(0)
+                    vmm.stop(FC_EXIT_CODE_OK as i32)
                 }
                 Err(e) => {
                     error!("Abruptly exited VMM control loop: {:?}", e);
-                    vmm.stop(1)
+                    vmm.stop(FC_EXIT_CODE_GENERIC_ERROR as i32);
                 }
             }
         })
@@ -2278,16 +2230,13 @@ mod tests {
     }
 
     #[test]
-    fn add_remove_event_test() {
+    fn add_event_test() {
         let mut ep = EpollContext::new().unwrap();
         let evfd = EventFd::new().unwrap();
 
         // adding new event should work
         let epev = ep.add_event(evfd, EpollDispatch::Exit);
         assert!(epev.is_ok());
-
-        // removing event should work
-        assert!(ep.remove_event(epev.unwrap()).is_ok());
     }
 
     #[test]
@@ -2323,60 +2272,6 @@ mod tests {
             *ep.dispatch_table[idx].as_ref().unwrap(),
             EpollDispatch::Exit
         );
-
-        // removing event should work
-        assert!(ep.remove_event(epev).is_ok());
-    }
-
-    #[test]
-    fn epoll_event_try_get_after_remove_test() {
-        let mut ep = EpollContext::new().unwrap();
-        let evfd = EventFd::new().unwrap();
-
-        // adding new event should work
-        let epev = ep.add_event(evfd, EpollDispatch::Exit).unwrap();
-
-        let evpoll_events_len = 10;
-
-        let mut events = vec![epoll::Event::new(epoll::Events::empty(), 0); evpoll_events_len];
-
-        // raise the event
-        assert!(epev.fd.write(1).is_ok());
-
-        // removing event should work
-        assert!(ep.remove_event(epev).is_ok());
-
-        // epoll should have no pending events
-        let epollret = epoll::wait(ep.epoll_raw_fd, 0, &mut events[..]);
-        let num_events = epollret.unwrap();
-        assert_eq!(num_events, 0);
-    }
-
-    #[test]
-    fn epoll_event_try_use_after_remove_test() {
-        let mut ep = EpollContext::new().unwrap();
-        let evfd = EventFd::new().unwrap();
-
-        // adding new event should work
-        let epev = ep.add_event(evfd, EpollDispatch::Exit).unwrap();
-
-        let evpoll_events_len = 10;
-        let mut events = vec![epoll::Event::new(epoll::Events::empty(), 0); evpoll_events_len];
-
-        // raise the event
-        assert!(epev.fd.write(1).is_ok());
-
-        // epoll should report one event
-        let epollret = epoll::wait(ep.epoll_raw_fd, 0, &mut events[..]);
-        let num_events = epollret.unwrap();
-        assert_eq!(num_events, 1);
-
-        // removing event should work
-        assert!(ep.remove_event(epev).is_ok());
-
-        // reported event should no longer be available
-        let idx = events[0].data as usize;
-        assert!(ep.dispatch_table[idx].is_none());
     }
 
     #[test]