From 1dfe4eda5c6a356862b6c5d5a48ad40ff9e32d8a Mon Sep 17 00:00:00 2001 From: Rob Bradford Date: Wed, 4 May 2022 10:36:26 +0100 Subject: [PATCH] vmm: Prevent "internal" identifiers being used by user For devices that cannot be named by the user use the "__" prefix to identify them as internal devices. Check that any identifiers provided in the config do not clash with those internal names. This prevents the user from creating a disk such as "__serial" which would then cause a failure in unpredictable manner. Signed-off-by: Rob Bradford --- vmm/src/config.rs | 78 ++++++++++++++++----------------------- vmm/src/device_manager.rs | 41 +++++++++++--------- 2 files changed, 55 insertions(+), 64 deletions(-) diff --git a/vmm/src/config.rs b/vmm/src/config.rs index 19b2b12c0..3a601728b 100644 --- a/vmm/src/config.rs +++ b/vmm/src/config.rs @@ -172,6 +172,8 @@ pub enum ValidationError { IommuNotSupported(u16), // Identifier is not unique IdentifierNotUnique(String), + /// Invalid identifier + InvalidIdentifier(String), } type ValidationResult = std::result::Result; @@ -261,6 +263,9 @@ impl fmt::Display for ValidationError { IdentifierNotUnique(s) => { write!(f, "Identifier {} is not unique", s) } + InvalidIdentifier(s) => { + write!(f, "Identifier {} is invalid", s) + } } } } @@ -2328,6 +2333,23 @@ pub struct VmConfig { } impl VmConfig { + fn validate_identifier( + id_list: &mut BTreeSet, + id: &Option, + ) -> ValidationResult<()> { + if let Some(id) = id.as_ref() { + if id.starts_with("__") { + return Err(ValidationError::InvalidIdentifier(id.clone())); + } + + if !id_list.insert(id.clone()) { + return Err(ValidationError::IdentifierNotUnique(id.clone())); + } + } + + Ok(()) + } + // Also enables virtio-iommu if the config needs it // Returns the list of unique identifiers provided through the // configuration. @@ -2379,11 +2401,7 @@ impl VmConfig { disk.validate(self)?; self.iommu |= disk.iommu; - if let Some(id) = disk.id.clone() { - if !id_list.insert(id.clone()) { - return Err(ValidationError::IdentifierNotUnique(id)); - } - } + Self::validate_identifier(&mut id_list, &disk.id)?; } } @@ -2395,11 +2413,7 @@ impl VmConfig { net.validate(self)?; self.iommu |= net.iommu; - if let Some(id) = net.id.clone() { - if !id_list.insert(id.clone()) { - return Err(ValidationError::IdentifierNotUnique(id)); - } - } + Self::validate_identifier(&mut id_list, &net.id)?; } } @@ -2410,11 +2424,7 @@ impl VmConfig { for fs in fses { fs.validate(self)?; - if let Some(id) = fs.id.clone() { - if !id_list.insert(id.clone()) { - return Err(ValidationError::IdentifierNotUnique(id)); - } - } + Self::validate_identifier(&mut id_list, &fs.id)?; } } @@ -2423,11 +2433,7 @@ impl VmConfig { pmem.validate(self)?; self.iommu |= pmem.iommu; - if let Some(id) = pmem.id.clone() { - if !id_list.insert(id.clone()) { - return Err(ValidationError::IdentifierNotUnique(id)); - } - } + Self::validate_identifier(&mut id_list, &pmem.id)?; } } @@ -2474,11 +2480,7 @@ impl VmConfig { for user_device in user_devices { user_device.validate(self)?; - if let Some(id) = user_device.id.clone() { - if !id_list.insert(id.clone()) { - return Err(ValidationError::IdentifierNotUnique(id)); - } - } + Self::validate_identifier(&mut id_list, &user_device.id)?; } } @@ -2487,11 +2489,7 @@ impl VmConfig { vdpa_device.validate(self)?; self.iommu |= vdpa_device.iommu; - if let Some(id) = vdpa_device.id.clone() { - if !id_list.insert(id.clone()) { - return Err(ValidationError::IdentifierNotUnique(id)); - } - } + Self::validate_identifier(&mut id_list, &vdpa_device.id)?; } } @@ -2517,11 +2515,7 @@ impl VmConfig { device.validate(self)?; self.iommu |= device.iommu; - if let Some(id) = device.id.clone() { - if !id_list.insert(id.clone()) { - return Err(ValidationError::IdentifierNotUnique(id)); - } - } + Self::validate_identifier(&mut id_list, &device.id)?; } } @@ -2529,11 +2523,7 @@ impl VmConfig { vsock.validate(self)?; self.iommu |= vsock.iommu; - if let Some(id) = vsock.id.clone() { - if !id_list.insert(id.clone()) { - return Err(ValidationError::IdentifierNotUnique(id)); - } - } + Self::validate_identifier(&mut id_list, &vsock.id)?; } if let Some(numa) = &self.numa { @@ -2557,9 +2547,7 @@ impl VmConfig { if let Some(zones) = &self.memory.zones { for zone in zones.iter() { let id = zone.id.clone(); - if !id_list.insert(id.clone()) { - return Err(ValidationError::IdentifierNotUnique(id)); - } + Self::validate_identifier(&mut id_list, &Some(id))?; } } @@ -2567,9 +2555,7 @@ impl VmConfig { if let Some(sgx_epcs) = &self.sgx_epc { for sgx_epc in sgx_epcs.iter() { let id = sgx_epc.id.clone(); - if !id_list.insert(id.clone()) { - return Err(ValidationError::IdentifierNotUnique(id)); - } + Self::validate_identifier(&mut id_list, &Some(id))?; } } diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 3c0292715..82c246665 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -115,30 +115,28 @@ use vmm_sys_util::eventfd::EventFd; #[cfg(target_arch = "aarch64")] const MMIO_LEN: u64 = 0x1000; -const VFIO_DEVICE_NAME_PREFIX: &str = "_vfio"; - -const VFIO_USER_DEVICE_NAME_PREFIX: &str = "_vfio_user"; - +// Singleton devices / devices the user cannot name #[cfg(target_arch = "x86_64")] -const IOAPIC_DEVICE_NAME: &str = "_ioapic"; - -const SERIAL_DEVICE_NAME_PREFIX: &str = "_serial"; +const IOAPIC_DEVICE_NAME: &str = "__ioapic"; +const SERIAL_DEVICE_NAME: &str = "__serial"; #[cfg(target_arch = "aarch64")] -const GPIO_DEVICE_NAME_PREFIX: &str = "_gpio"; +const GPIO_DEVICE_NAME: &str = "__gpio"; +const RNG_DEVICE_NAME: &str = "__rng"; +const IOMMU_DEVICE_NAME: &str = "__iommu"; +const BALLOON_DEVICE_NAME: &str = "__balloon"; +const CONSOLE_DEVICE_NAME: &str = "__console"; -const CONSOLE_DEVICE_NAME: &str = "_console"; +// Devices that the user may name and for which we generate +// identifiers if the user doesn't give one const DISK_DEVICE_NAME_PREFIX: &str = "_disk"; const FS_DEVICE_NAME_PREFIX: &str = "_fs"; -const BALLOON_DEVICE_NAME: &str = "_balloon"; const NET_DEVICE_NAME_PREFIX: &str = "_net"; const PMEM_DEVICE_NAME_PREFIX: &str = "_pmem"; -const RNG_DEVICE_NAME: &str = "_rng"; const VDPA_DEVICE_NAME_PREFIX: &str = "_vdpa"; const VSOCK_DEVICE_NAME_PREFIX: &str = "_vsock"; -const WATCHDOG_DEVICE_NAME: &str = "_watchdog"; - -const IOMMU_DEVICE_NAME: &str = "_iommu"; - +const WATCHDOG_DEVICE_NAME: &str = "__watchdog"; +const VFIO_DEVICE_NAME_PREFIX: &str = "_vfio"; +const VFIO_USER_DEVICE_NAME_PREFIX: &str = "_vfio_user"; const VIRTIO_PCI_DEVICE_NAME_PREFIX: &str = "_virtio-pci"; /// Errors associated with device manager @@ -480,6 +478,9 @@ pub enum DeviceManagerError { /// Invalid identifier as it is not unique. IdentifierNotUnique(String), + + /// Invalid identifier + InvalidIdentifier(String), } pub type DeviceManagerResult = result::Result; @@ -1565,7 +1566,7 @@ impl DeviceManager { ); // Add a GPIO device - let id = String::from(GPIO_DEVICE_NAME_PREFIX); + let id = String::from(GPIO_DEVICE_NAME); let gpio_irq = self .address_manager .allocator @@ -1655,7 +1656,7 @@ impl DeviceManager { // Serial is tied to IRQ #4 let serial_irq = 4; - let id = String::from(SERIAL_DEVICE_NAME_PREFIX); + let id = String::from(SERIAL_DEVICE_NAME); let interrupt_group = interrupt_manager .create_group(LegacyIrqGroupConfig { @@ -1701,7 +1702,7 @@ impl DeviceManager { interrupt_manager: &Arc>, serial_writer: Option>, ) -> DeviceManagerResult>> { - let id = String::from(SERIAL_DEVICE_NAME_PREFIX); + let id = String::from(SERIAL_DEVICE_NAME); let serial_irq = self .address_manager @@ -4225,6 +4226,10 @@ impl DeviceManager { fn validate_identifier(&self, id: &Option) -> DeviceManagerResult<()> { if let Some(id) = id { + if id.starts_with("__") { + return Err(DeviceManagerError::InvalidIdentifier(id.clone())); + } + if self.device_tree.lock().unwrap().contains_key(id) { return Err(DeviceManagerError::IdentifierNotUnique(id.clone())); }