vmm: preserved FDs: prevent usage of same FD multiple times

Allowing the same file descriptor (FD) to be preserved more than once
can lead to severe issues. Consider the case where management software
adds two virtio-net devices at runtime, both backed by the same
externally provided FD.

When the first device is removed during runtime, the FD for the second
device becomes invalid [0].

To avoid misconfigurations, we now prevent multiple preservation
attempts of the same FD.

This however has some caveats: In the current model, external FDs are
added to the list of preserved FDs after the corresponding device has
been created successfully. Now assume the following:
- VM boots
  - VM config has no preserved FDs
  - We add a device to the VM that uses an external FD for its Tap dev
  - The device is successfully initialized
  - VM config is extended with the preserved FDs
- VM reboots
  - VM config already has preserved FDs
  - Same as above
  - CRASH

Therefore, we need to know for every preserved FD whether it is "cold",
i.e., only has been part of a former VmConfig without associated device,
or "hot", i.e., it is currently also actively in use. This allows state
transitions from cold->hot in which case we won't throw an error for a
reused FD. Otherwise, we throw errors.

This requires that on shutdown, all preserved FDs are marked as cold.

[0] cloud-hypervisor#7371

Signed-off-by: Philipp Schuster <philipp.schuster@cyberus-technology.de>
On-behalf-of: SAP philipp.schuster@sap.com

Author: phip1611

fuzz/fuzz_targets/http_api.rs

@@ -3,6 +3,7 @@
 // SPDX-License-Identifier: Apache-2.0
 
 #![no_main]
+use std::collections::BTreeSet;
 use std::os::unix::io::AsRawFd;
 use std::path::PathBuf;
 use std::sync::mpsc::{channel, Receiver};
@@ -192,7 +193,7 @@ impl RequestHandler for StubApiRequestHandler {
                 pci_segments: None,
                 platform: None,
                 tpm: None,
-                preserved_fds: None,
+                preserved_fds: BTreeSet::new(),
                 landlock_enable: false,
                 landlock_rules: None,
                 #[cfg(feature = "ivshmem")]

src/main.rs

@@ -904,6 +904,7 @@ fn main() {
 
 #[cfg(test)]
 mod unit_tests {
+    use std::collections::BTreeSet;
     use std::path::PathBuf;
 
     use vmm::config::VmParams;
@@ -1028,7 +1029,7 @@ mod unit_tests {
             pci_segments: None,
             platform: None,
             tpm: None,
-            preserved_fds: None,
+            preserved_fds: BTreeSet::new(),
             landlock_enable: false,
             landlock_rules: None,
             #[cfg(feature = "ivshmem")]

vmm/src/config.rs

@@ -6,10 +6,10 @@
 use std::collections::{BTreeSet, HashMap};
 #[cfg(feature = "ivshmem")]
 use std::fs;
-use std::os::fd::RawFd;
+use std::os::fd::{AsRawFd, RawFd};
 use std::path::PathBuf;
-use std::result;
 use std::str::FromStr;
+use std::{mem, result};
 
 use clap::ArgMatches;
 use option_parser::{
@@ -361,6 +361,14 @@ pub enum ValidationError {
     InvalidIvshmemPath,
     #[error("Payload configuration is not bootable")]
     PayloadError(#[from] PayloadConfigError),
+    /// Multiple things in a VM can be backed up by externally provided FDs,
+    /// such as memory zones, virtio-net devices, or virtio-blk devices (not
+    /// yet implemented, but likely in future).
+    ///
+    /// It is a hard configuration error to use the same FD for multiple
+    /// things.
+    #[error("Externally provided FD already in use! FD={0}")]
+    ExternalFdAlreadyInUse(RawFd),
 }
 
 type ValidationResult<T> = std::result::Result<T, ValidationError>;
@@ -3058,7 +3066,7 @@ impl VmConfig {
             pci_segments,
             platform,
             tpm,
-            preserved_fds: None,
+            preserved_fds: BTreeSet::new(),
             landlock_enable: vm_params.landlock_enable,
             landlock_rules,
             #[cfg(feature = "ivshmem")]
@@ -3131,21 +3139,78 @@ impl VmConfig {
         removed
     }
 
+    /// Adds the provided external FDs to the preserved FDs.
+    ///
+    /// This is useful to keep external FDs valid across VM reboots, where no
+    /// device instance holds open FDs.
+    ///
     /// # Safety
     /// To use this safely, the caller must guarantee that the input
-    /// fds are all valid.
-    pub unsafe fn add_preserved_fds(&mut self, mut fds: Vec<i32>) {
-        debug!("adding preserved FDs to VM list: {fds:?}");
+    /// FDs are all valid.
+    ///
+    /// # Arguments
+    /// - `fds`: Iterator over the external FDs of some component
+    /// - `hot`: Whether this happens when the VM is running, i.e., whether
+    ///   device instances also have open (dup'ed) FDs of this or just the VM
+    ///   config.
+    ///
+    /// # Panics
+    /// This panics when an external FD is used multiple times to force
+    /// programmers to do proper checking early.
+    pub fn add_preserved_fds(
+        &mut self,
+        fds: impl IntoIterator<Item = RawFd> + Clone,
+        hot: bool,
+    ) -> ValidationResult<()> {
+        self.can_add_preserved_fds(fds.clone(), hot)
+            .expect(&format!(
+                "should have detected earlier that FDs {:?} can't be added: preserved: {:?}",
+                fds.clone().into_iter().collect::<Vec<_>>().len(),
+                self.preserved_fds
+            ));
 
-        if fds.is_empty() {
-            return;
+        for fd in fds.into_iter() {
+            let pfd = self.preserved_fds.iter().find(|pfd| pfd.as_raw_fd() == fd);
+            match (pfd, hot) {
+                (None, false) => {
+                    self.preserved_fds.insert(PreservedFd::Cold(fd));
+                }
+                (None, true) => {
+                    self.preserved_fds.insert(PreservedFd::Hot(fd));
+                }
+                // We allow the transition from Cold to Hot, i.e., "in use"
+                (Some(PreservedFd::Cold(_)), true) => {
+                    self.preserved_fds.remove(&PreservedFd::Cold(fd));
+                    self.preserved_fds.insert(PreservedFd::Hot(fd));
+                }
+                // Invalid state transition, propagate error
+                (Some(_), _) => return Err(ValidationError::ExternalFdAlreadyInUse(fd)),
+            }
         }
+        Ok(())
+    }
 
-        if let Some(preserved_fds) = &self.preserved_fds {
-            fds.append(&mut preserved_fds.clone());
+    /// Checks if any of the externally provided FDs is already preserved in the
+    /// list of preserved FDs.
+    ///
+    /// This function is supposed to be called early in every code path that adds
+    /// configurations tide to such FDs.
+    pub fn can_add_preserved_fds(
+        &self,
+        external_fds: impl IntoIterator<Item = RawFd>,
+        hot: bool,
+    ) -> ValidationResult<()> {
+        for fd in external_fds.into_iter() {
+            let pfd = self.preserved_fds.iter().find(|pfd| pfd.as_raw_fd() == fd);
+            return match (pfd, hot) {
+                (None, _) => Ok(()),
+                // We allow the transition from Cold to Hot, i.e., "in use now"
+                (Some(PreservedFd::Cold(_)), true) => Ok(()),
+                // Invalid state transition, propagate error
+                (Some(_), _) => Err(ValidationError::ExternalFdAlreadyInUse(fd)),
+            };
         }
-
-        self.preserved_fds = Some(fds);
+        Ok(())
     }
 
     #[cfg(feature = "tdx")]
@@ -3188,18 +3253,10 @@ impl Clone for VmConfig {
             tpm: self.tpm.clone(),
             preserved_fds: self
                 .preserved_fds
-                .as_ref()
+                .iter()
                 // SAFETY: FFI call with valid FDs
-                .map(|fds| {
-                    fds.iter()
-                        .map(|fd| {
-                            // SAFETY: Trivially safe.
-                            let fd_duped = unsafe { libc::dup(*fd) };
-                            warn!("Cloning VM config: duping preserved FD {fd} => {fd_duped}");
-                            fd_duped
-                        })
-                        .collect()
-                }),
+                .map(|fd| fd.dup())
+                .collect(),
             landlock_rules: self.landlock_rules.clone(),
             #[cfg(feature = "ivshmem")]
             ivshmem: self.ivshmem.clone(),
@@ -3210,12 +3267,13 @@ impl Clone for VmConfig {
 
 impl Drop for VmConfig {
     fn drop(&mut self) {
-        if let Some(mut fds) = self.preserved_fds.take() {
-            debug!("Closing preserved FDs from VM: fds={fds:?}");
-            for fd in fds.drain(..) {
-                // SAFETY: FFI call with valid FDs
-                unsafe { libc::close(fd) };
-            }
+        debug!("Closing preserved FDs: fds={:?}", self.preserved_fds);
+
+        // There is no .drain() for BTreeSet yet
+        let fds = mem::take(&mut self.preserved_fds);
+        for fd in fds {
+            // SAFETY: FFI call with valid FDs
+            unsafe { libc::close(fd.as_raw_fd()) };
         }
     }
 }
@@ -3994,7 +4052,7 @@ mod tests {
             pci_segments: None,
             platform: None,
             tpm: None,
-            preserved_fds: None,
+            preserved_fds: BTreeSet::new(),
             net: Some(vec![
                 NetConfig {
                     id: Some("net0".to_owned()),
@@ -4207,7 +4265,7 @@ mod tests {
             pci_segments: None,
             platform: None,
             tpm: None,
-            preserved_fds: None,
+            preserved_fds: BTreeSet::new(),
             landlock_enable: false,
             landlock_rules: None,
             #[cfg(feature = "ivshmem")]
@@ -4829,17 +4887,58 @@ mod tests {
             config_with_invalid_host_data.validate().unwrap_err();
         }
 
-        let mut still_valid_config = valid_config;
+        let mut still_valid_config = valid_vmconfig();
         // SAFETY: Safe as the file was just opened
         let fd1 = unsafe { libc::dup(File::open("/dev/null").unwrap().as_raw_fd()) };
         // SAFETY: Safe as the file was just opened
         let fd2 = unsafe { libc::dup(File::open("/dev/null").unwrap().as_raw_fd()) };
-        // SAFETY: safe as both FDs are valid
-        unsafe {
-            still_valid_config.add_preserved_fds(vec![fd1, fd2]);
-        }
+        still_valid_config
+            .add_preserved_fds([fd1, fd2], false)
+            .unwrap();
         let _still_valid_config = still_valid_config.clone();
     }
+
+    #[test]
+    fn test_add_preserved_fds_corner_cases() {
+        let mut config = valid_vmconfig();
+
+        config.can_add_preserved_fds([3], false).unwrap();
+        config.add_preserved_fds([3], false).unwrap();
+
+        config.can_add_preserved_fds([3], false).unwrap_err();
+
+        config.can_add_preserved_fds([3], true).unwrap();
+        config.add_preserved_fds([3], true).unwrap();
+
+        config.can_add_preserved_fds([3], true).unwrap_err();
+    }
+
+    #[test]
+    #[should_panic]
+    fn test_add_preserved_fds_corner_cases_panic_cold_cold() {
+        let mut config = valid_vmconfig();
+
+        config.add_preserved_fds([3], false).unwrap();
+
+        // panic here
+        config
+            .add_preserved_fds([3], false)
+            .expect_err("should fail as the device is already hot");
+    }
+
+    #[test]
+    #[should_panic]
+    fn test_add_preserved_fds_corner_cases_panic_hot_hot() {
+        let mut config = valid_vmconfig();
+
+        config.add_preserved_fds([3], true).unwrap();
+
+        // panic here
+        config
+            .add_preserved_fds([3], true)
+            .expect_err("should fail as the device is already hot");
+    }
+
     #[test]
     fn test_landlock_parsing() -> Result<()> {
         // should not be empty

vmm/src/device_manager.rs

@@ -124,7 +124,7 @@ use crate::vm_config::{
     DeviceConfig, DiskConfig, FsConfig, NetConfig, PmemConfig, UserDeviceConfig, VdpaConfig,
     VhostMode, VmConfig, VsockConfig,
 };
-use crate::{DEVICE_MANAGER_SNAPSHOT_ID, GuestRegionMmap, PciDeviceInfo, device_node};
+use crate::{DEVICE_MANAGER_SNAPSHOT_ID, GuestRegionMmap, PciDeviceInfo, config, device_node};
 
 #[cfg(any(target_arch = "aarch64", target_arch = "riscv64"))]
 const MMIO_LEN: u64 = 0x1000;
@@ -667,6 +667,9 @@ pub enum DeviceManagerError {
     /// Error adding fw_cfg to bus.
     #[error("Error adding fw_cfg to bus")]
     ErrorAddingFwCfgToBus(#[source] vm_device::BusError),
+
+    #[error("Multiple use of the same FD is misconfiguration")]
+    ExternalFdMisconfiguration(#[source] config::ValidationError),
 }
 
 pub type DeviceManagerResult<T> = result::Result<T, DeviceManagerError>;
@@ -2955,10 +2958,11 @@ impl DeviceManager {
                 )
                 .map_err(DeviceManagerError::CreateVirtioNet)?;
 
-                // SAFETY: 'fds' are valid because TAP devices are created successfully
-                unsafe {
-                    self.config.lock().unwrap().add_preserved_fds(fds.clone());
-                }
+                self.config
+                    .lock()
+                    .unwrap()
+                    .add_preserved_fds(fds.iter().cloned(), true)
+                    .map_err(DeviceManagerError::ExternalFdMisconfiguration)?;
 
                 Arc::new(Mutex::new(net))
             } else {
@@ -4542,8 +4546,8 @@ impl DeviceManager {
 
                     debug!("Closing preserved FDs from virtio-net device: id={id}, fds={fds:?}");
                     for fd in fds {
-                        config.preserved_fds.as_mut().unwrap().retain(|x| *x != fd);
-                        // SAFETY: Trivially safe. We know the FD is not referenced any longer.
+                        config.preserved_fds.retain(|x| x.as_raw_fd() != fd);
+                        // SAFETY: We are closing the only remaining instance of this FD.
                         unsafe {
                             libc::close(fd);
                         }

vmm/src/lib.rs

@@ -2253,10 +2253,17 @@ impl RequestHandler for Vmm {
 
     fn vm_add_net(&mut self, net_cfg: NetConfig) -> result::Result<Option<Vec<u8>>, VmError> {
         self.vm_config.as_ref().ok_or(VmError::VmNotCreated)?;
-
         {
             // Validate the configuration change in a cloned configuration
             let mut config = self.vm_config.as_ref().unwrap().lock().unwrap().clone();
+
+            // Special handling for externally provided FDs
+            if let Some(fds) = &net_cfg.fds {
+                config
+                    .can_add_preserved_fds(fds.iter().cloned(), true)
+                    .map_err(|a| VmError::ConfigValidation(a))?;
+            }
+
             add_to_config(&mut config.net, net_cfg.clone());
             config.validate().map_err(VmError::ConfigValidation)?;
         }
@@ -2593,6 +2600,8 @@ const DEVICE_MANAGER_SNAPSHOT_ID: &str = "device-manager";
 
 #[cfg(test)]
 mod unit_tests {
+    use std::collections::BTreeSet;
+
     use super::*;
     #[cfg(target_arch = "x86_64")]
     use crate::vm_config::DebugConsoleConfig;
@@ -2693,7 +2702,7 @@ mod unit_tests {
             pci_segments: None,
             platform: None,
             tpm: None,
-            preserved_fds: None,
+            preserved_fds: BTreeSet::new(),
             landlock_enable: false,
             landlock_rules: None,
             #[cfg(feature = "ivshmem")]