From 664431ff14d433008245feefc93b4456d8404e89 Mon Sep 17 00:00:00 2001 From: Samuel Ortiz Date: Thu, 12 Dec 2019 14:08:34 +0100 Subject: [PATCH] vsock: vhost_user: vfio: Fix potential host memory overflow The vsock packets that we're building are resolving guest addresses to host ones and use the latter as raw pointers. If the corresponding guest mapped buffer spans across several regions in the guest, they will do so in the host as well. Since we have no guarantees that host regions are contiguous, it may lead the VMM into trying to access memory outside of its memory space. For now we fix that by ensuring that the guest buffers do not span across several regions. If they do, we error out. Ideally, we should enhance the rust-vmm memory model to support safe acces across host regions. Fixes CVE-2019-18960 Signed-off-by: Samuel Ortiz --- Cargo.lock | 1 + vfio/src/vfio_device.rs | 14 ++--- vm-device/Cargo.toml | 4 ++ vm-device/src/lib.rs | 70 ++++++++++++++++++++++ vm-virtio/src/queue.rs | 2 +- vm-virtio/src/vhost_user/vu_common_ctrl.rs | 25 +++++--- vm-virtio/src/vsock/packet.rs | 23 +++---- 7 files changed, 110 insertions(+), 29 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f42b2bcc9..f47c9aa94 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1073,6 +1073,7 @@ dependencies = [ "serde_derive 1.0.103 (registry+https://github.com/rust-lang/crates.io-index)", "serde_json 1.0.44 (registry+https://github.com/rust-lang/crates.io-index)", "thiserror 1.0.6 (registry+https://github.com/rust-lang/crates.io-index)", + "vm-memory 0.1.0 (git+https://github.com/rust-vmm/vm-memory)", ] [[package]] diff --git a/vfio/src/vfio_device.rs b/vfio/src/vfio_device.rs index e42706144..1dc17fa17 100644 --- a/vfio/src/vfio_device.rs +++ b/vfio/src/vfio_device.rs @@ -6,6 +6,7 @@ use crate::vec_with_array_field; use byteorder::{ByteOrder, LittleEndian}; use kvm_ioctls::*; use std::collections::HashMap; +use std::convert::TryInto; use std::ffi::CString; use std::fmt; use std::fs::{File, OpenOptions}; @@ -19,7 +20,7 @@ use std::sync::{Arc, RwLock}; use std::u32; use vfio_bindings::bindings::vfio::*; use vfio_ioctls::*; -use vm_device::ExternalDmaMapping; +use vm_device::{get_host_address_range, ExternalDmaMapping}; use vm_memory::{Address, GuestAddress, GuestMemory, GuestMemoryMmap, GuestMemoryRegion}; use vmm_sys_util::eventfd::EventFd; use vmm_sys_util::ioctl::*; @@ -524,12 +525,11 @@ impl VfioDmaMapping { impl ExternalDmaMapping for VfioDmaMapping { fn map(&self, iova: u64, gpa: u64, size: u64) -> result::Result<(), io::Error> { - let user_addr = if let Some(addr) = self - .memory - .read() - .unwrap() - .get_host_address(GuestAddress(gpa)) - { + let user_addr = if let Some(addr) = get_host_address_range( + &self.memory.read().unwrap(), + GuestAddress(gpa), + size.try_into().unwrap(), + ) { addr as u64 } else { return Err(io::Error::new( diff --git a/vm-device/Cargo.toml b/vm-device/Cargo.toml index aa1725538..32a924305 100644 --- a/vm-device/Cargo.toml +++ b/vm-device/Cargo.toml @@ -10,3 +10,7 @@ thiserror = "1.0" serde = {version = ">=1.0.27", features = ["rc"] } serde_derive = ">=1.0.27" serde_json = ">=1.0.9" + +[dependencies.vm-memory] +git = "https://github.com/rust-vmm/vm-memory" +features = ["backend-mmap"] \ No newline at end of file diff --git a/vm-device/src/lib.rs b/vm-device/src/lib.rs index 124e1338d..a2ea555ef 100644 --- a/vm-device/src/lib.rs +++ b/vm-device/src/lib.rs @@ -1,5 +1,11 @@ extern crate serde; extern crate thiserror; +extern crate vm_memory; + +use vm_memory::{ + Address, GuestAddress, GuestMemory, GuestMemoryMmap, GuestMemoryRegion, GuestRegionMmap, + MemoryRegionAddress, +}; use thiserror::Error; @@ -42,3 +48,67 @@ pub trait Snapshotable {} /// eventually resumed. Thus any Migratable component must be both Pausable /// and Snapshotable. pub trait Migratable: Pausable + Snapshotable {} + +fn get_region_host_address_range( + region: &GuestRegionMmap, + addr: MemoryRegionAddress, + size: usize, +) -> Option<*mut u8> { + region.check_address(addr).and_then(|addr| { + region + .checked_offset(addr, size) + .map(|_| region.as_ptr().wrapping_offset(addr.raw_value() as isize)) + }) +} + +/// Convert an absolute address into an address space (GuestMemory) +/// to a host pointer and verify that the provided size define a valid +/// range within a single memory region. +/// Return None if it is out of bounds or if addr+size overlaps a single region. +/// +/// This is a temporary vm-memory wrapper. +pub fn get_host_address_range( + mem: &GuestMemoryMmap, + addr: GuestAddress, + size: usize, +) -> Option<*mut u8> { + mem.to_region_addr(addr) + .and_then(|(r, addr)| get_region_host_address_range(r, addr, size)) +} + +#[cfg(test)] +mod tests { + + use super::*; + use vm_memory::{GuestAddress, GuestMemoryMmap}; + + #[test] + fn test_get_host_address_range() { + let start_addr1 = GuestAddress(0x0); + let start_addr2 = GuestAddress(0x1000); + let guest_mem = + GuestMemoryMmap::new(&[(start_addr1, 0x400), (start_addr2, 0x400)]).unwrap(); + + assert!(get_host_address_range(&guest_mem, GuestAddress(0x600), 0x100).is_none()); + + // Overlapping range + assert!(get_host_address_range(&guest_mem, GuestAddress(0x1000), 0x500).is_none()); + + // Overlapping range + assert!(get_host_address_range(&guest_mem, GuestAddress(0x1200), 0x500).is_none()); + + let ptr = get_host_address_range(&guest_mem, GuestAddress(0x1000), 0x100).unwrap(); + + let ptr0 = get_host_address_range(&guest_mem, GuestAddress(0x1100), 0x100).unwrap(); + + let ptr1 = guest_mem.get_host_address(GuestAddress(0x1200)).unwrap(); + assert_eq!( + ptr, + guest_mem + .find_region(GuestAddress(0x1100)) + .unwrap() + .as_ptr() + ); + assert_eq!(unsafe { ptr0.offset(0x100) }, ptr1); + } +} diff --git a/vm-virtio/src/queue.rs b/vm-virtio/src/queue.rs index ba8d52590..2a46ce94e 100644 --- a/vm-virtio/src/queue.rs +++ b/vm-virtio/src/queue.rs @@ -63,7 +63,7 @@ impl<'a> Iterator for DescIter<'a> { /// A virtio descriptor constraints with C representive. #[repr(C)] #[derive(Default, Clone, Copy)] -struct Descriptor { +pub struct Descriptor { addr: u64, len: u32, flags: u16, diff --git a/vm-virtio/src/vhost_user/vu_common_ctrl.rs b/vm-virtio/src/vhost_user/vu_common_ctrl.rs index 935c3c0e2..122ff1454 100644 --- a/vm-virtio/src/vhost_user/vu_common_ctrl.rs +++ b/vm-virtio/src/vhost_user/vu_common_ctrl.rs @@ -3,9 +3,13 @@ use libc; use libc::EFD_NONBLOCK; +use std::convert::TryInto; use std::os::unix::io::AsRawFd; use std::vec::Vec; +use crate::queue::Descriptor; + +use vm_device::get_host_address_range; use vm_memory::{Address, Error as MmapError, GuestMemory, GuestMemoryMmap, GuestMemoryRegion}; use vmm_sys_util::eventfd::EventFd; @@ -54,6 +58,8 @@ pub fn setup_vhost_user_vring( let mut vu_interrupt_list = Vec::new(); for (queue_index, queue) in queues.into_iter().enumerate() { + let actual_size: usize = queue.actual_size().try_into().unwrap(); + vu.set_vring_num(queue_index, queue.actual_size()) .map_err(Error::VhostUserSetVringNum)?; @@ -61,14 +67,19 @@ pub fn setup_vhost_user_vring( queue_max_size: queue.get_max_size(), queue_size: queue.actual_size(), flags: 0u32, - desc_table_addr: mem - .get_host_address(queue.desc_table) - .ok_or_else(|| Error::DescriptorTableAddress)? as u64, - used_ring_addr: mem - .get_host_address(queue.used_ring) + desc_table_addr: get_host_address_range( + &mem, + queue.desc_table, + actual_size * std::mem::size_of::(), + ) + .ok_or_else(|| Error::DescriptorTableAddress)? as u64, + // The used ring is {flags: u16; idx: u16; virtq_used_elem [{id: u16, len: u16}; actual_size]}, + // i.e. 4 + (4 + 4) * actual_size. + used_ring_addr: get_host_address_range(&mem, queue.used_ring, 4 + actual_size * 8) .ok_or_else(|| Error::UsedAddress)? as u64, - avail_ring_addr: mem - .get_host_address(queue.avail_ring) + // The used ring is {flags: u16; idx: u16; elem [u16; actual_size]}, + // i.e. 4 + (2) * actual_size. + avail_ring_addr: get_host_address_range(&mem, queue.avail_ring, 4 + actual_size * 2) .ok_or_else(|| Error::AvailAddress)? as u64, log_addr: None, }; diff --git a/vm-virtio/src/vsock/packet.rs b/vm-virtio/src/vsock/packet.rs index 7d25ac269..6bb6fc202 100644 --- a/vm-virtio/src/vsock/packet.rs +++ b/vm-virtio/src/vsock/packet.rs @@ -20,6 +20,7 @@ use byteorder::{ByteOrder, LittleEndian}; use super::super::DescriptorChain; use super::defs; use super::{Result, VsockError}; +use vm_device::get_host_address_range; // The vsock packet header is defined by the C struct: // @@ -116,9 +117,7 @@ impl VsockPacket { } let mut pkt = Self { - hdr: head - .mem - .get_host_address(head.addr) + hdr: get_host_address_range(&head.mem, head.addr, VSOCK_PKT_HDR_SIZE) .ok_or_else(|| VsockError::GuestMemory)? as *mut u8, buf: None, buf_size: 0, @@ -151,9 +150,7 @@ impl VsockPacket { pkt.buf_size = buf_desc.len as usize; pkt.buf = Some( - buf_desc - .mem - .get_host_address(buf_desc.addr) + get_host_address_range(&buf_desc.mem, buf_desc.addr, pkt.buf_size) .ok_or_else(|| VsockError::GuestMemory)? as *mut u8, ); @@ -182,19 +179,16 @@ impl VsockPacket { return Err(VsockError::BufDescMissing); } let buf_desc = head.next_descriptor().ok_or(VsockError::BufDescMissing)?; + let buf_size = buf_desc.len as usize; Ok(Self { - hdr: head - .mem - .get_host_address(head.addr) + hdr: get_host_address_range(&head.mem, head.addr, VSOCK_PKT_HDR_SIZE) .ok_or_else(|| VsockError::GuestMemory)? as *mut u8, buf: Some( - buf_desc - .mem - .get_host_address(buf_desc.addr) + get_host_address_range(&buf_desc.mem, buf_desc.addr, buf_size) .ok_or_else(|| VsockError::GuestMemory)? as *mut u8, ), - buf_size: buf_desc.len as usize, + buf_size, }) } @@ -383,7 +377,8 @@ mod tests { fn set_pkt_len(len: u32, guest_desc: &GuestQDesc, mem: &GuestMemoryMmap) { let hdr_gpa = guest_desc.addr.get(); - let hdr_ptr = mem.get_host_address(GuestAddress(hdr_gpa)).unwrap() as *mut u8; + let hdr_ptr = get_host_address_range(&mem, GuestAddress(hdr_gpa), VSOCK_PKT_HDR_SIZE) + .unwrap() as *mut u8; let len_ptr = unsafe { hdr_ptr.add(HDROFF_LEN) }; LittleEndian::write_u32(unsafe { std::slice::from_raw_parts_mut(len_ptr, 4) }, len);