From 5329be3634547fe383aff7981854484088e6a622 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Thu, 31 Jan 2019 10:20:30 -0800 Subject: [PATCH] devices: block: add bounds checks As reported by the Firecracker team, the block device model doesn't check if an I/O request starts before the end of the disk but extends beyond it. For writes to disks backed by raw files, this could end up unintentionally extending the size of the disk. Add bounds checks to the request execution path to catch these out-of-bounds I/Os and fail them. While we're here, fix a few other minor issues: only seek for read and write requests (the 'sector' field of the request should be ignored for flush, write zeroes, and discard), and check for overflow when performing the shifts to convert from sectors to bytes. BUG=chromium:927393 TEST=cargo test -p devices block Change-Id: I0dd19299d03a4f0716093091f173a5c507529963 Signed-off-by: Daniel Verkamp Reviewed-on: https://chromium-review.googlesource.com/1448852 Tested-by: kokoro Reviewed-by: Dylan Reid --- devices/src/virtio/block.rs | 155 ++++++++++++++++++++++++++++++++---- 1 file changed, 141 insertions(+), 14 deletions(-) diff --git a/devices/src/virtio/block.rs b/devices/src/virtio/block.rs index b022fa900a..223fe1fcba 100644 --- a/devices/src/virtio/block.rs +++ b/devices/src/virtio/block.rs @@ -210,6 +210,7 @@ enum ExecuteError { ReadOnly { request_type: RequestType, }, + OutOfRange, Unsupported(u32), } @@ -223,6 +224,7 @@ impl ExecuteError { ExecuteError::Write { .. } => VIRTIO_BLK_S_IOERR, ExecuteError::DiscardWriteZeroes { .. } => VIRTIO_BLK_S_IOERR, ExecuteError::ReadOnly { .. } => VIRTIO_BLK_S_IOERR, + ExecuteError::OutOfRange { .. } => VIRTIO_BLK_S_IOERR, ExecuteError::Unsupported(_) => VIRTIO_BLK_S_UNSUPP, } } @@ -373,6 +375,7 @@ impl Request { &self, read_only: bool, disk: &mut T, + disk_size: u64, flush_timer: &mut TimerFd, flush_timer_armed: &mut bool, mem: &GuestMemory, @@ -386,13 +389,35 @@ impl Request { }); } - disk.seek(SeekFrom::Start(self.sector << SECTOR_SHIFT)) - .map_err(|e| ExecuteError::Seek { - ioerr: e, - sector: self.sector, - })?; + /// Check that a request accesses only data within the disk's current size. + /// All parameters are in units of bytes. + fn check_range( + io_start: u64, + io_length: u64, + disk_size: u64, + ) -> result::Result<(), ExecuteError> { + let io_end = io_start + .checked_add(io_length) + .ok_or(ExecuteError::OutOfRange)?; + if io_end > disk_size { + Err(ExecuteError::OutOfRange) + } else { + Ok(()) + } + } + match self.request_type { RequestType::In => { + let offset = self + .sector + .checked_shl(u32::from(SECTOR_SHIFT)) + .ok_or(ExecuteError::OutOfRange)?; + check_range(offset, u64::from(self.data_len), disk_size)?; + disk.seek(SeekFrom::Start(offset)) + .map_err(|e| ExecuteError::Seek { + ioerr: e, + sector: self.sector, + })?; mem.read_to_memory(self.data_addr, disk, self.data_len as usize) .map_err(|e| ExecuteError::Read { addr: self.data_addr, @@ -403,6 +428,16 @@ impl Request { return Ok(self.data_len); } RequestType::Out => { + let offset = self + .sector + .checked_shl(u32::from(SECTOR_SHIFT)) + .ok_or(ExecuteError::OutOfRange)?; + check_range(offset, u64::from(self.data_len), disk_size)?; + disk.seek(SeekFrom::Start(offset)) + .map_err(|e| ExecuteError::Seek { + ioerr: e, + sector: self.sector, + })?; mem.write_from_memory(self.data_addr, disk, self.data_len as usize) .map_err(|e| ExecuteError::Write { addr: self.data_addr, @@ -438,23 +473,29 @@ impl Request { }); } + let offset = sector + .checked_shl(u32::from(SECTOR_SHIFT)) + .ok_or(ExecuteError::OutOfRange)?; + let length = u64::from(num_sectors) + .checked_shl(u32::from(SECTOR_SHIFT)) + .ok_or(ExecuteError::OutOfRange)?; + check_range(offset, length, disk_size)?; + if self.request_type == RequestType::Discard { // Since Discard is just a hint and some filesystems may not implement // FALLOC_FL_PUNCH_HOLE, ignore punch_hole errors. - let _ = disk.punch_hole( - sector << SECTOR_SHIFT, - (num_sectors as u64) << SECTOR_SHIFT, - ); + let _ = disk.punch_hole(offset, length); } else { - disk.seek(SeekFrom::Start(sector << SECTOR_SHIFT)) + disk.seek(SeekFrom::Start(offset)) .map_err(|e| ExecuteError::Seek { ioerr: e, sector })?; - disk.write_zeroes((num_sectors as usize) << SECTOR_SHIFT) - .map_err(|e| ExecuteError::DiscardWriteZeroes { + disk.write_zeroes(length as usize).map_err(|e| { + ExecuteError::DiscardWriteZeroes { ioerr: Some(e), sector, num_sectors, flags, - })?; + } + })?; } } } @@ -489,6 +530,8 @@ impl Worker { ) -> bool { let queue = &mut self.queues[queue_index]; + let disk_size = self.disk_size.lock(); + let mut used_desc_heads = [(0, 0); QUEUE_SIZE as usize]; let mut used_count = 0; for avail_desc in queue.iter(&self.mem) { @@ -498,6 +541,7 @@ impl Worker { let status = match request.execute( self.read_only, &mut self.disk_image, + *disk_size, flush_timer, flush_timer_armed, &self.mem, @@ -854,7 +898,7 @@ impl VirtioDevice for Block { #[cfg(test)] mod tests { - use std::fs::File; + use std::fs::{File, OpenOptions}; use std::path::PathBuf; use sys_util::TempDir; @@ -903,4 +947,87 @@ mod tests { assert_eq!(0x100000220, b.features()); } } + + #[test] + fn read_last_sector() { + let tempdir = TempDir::new("/tmp/block_read_test").unwrap(); + let mut path = PathBuf::from(tempdir.as_path().unwrap()); + path.push("disk_image"); + let mut f = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .open(&path) + .unwrap(); + let disk_size = 0x1000; + f.set_len(disk_size).unwrap(); + + let mem = GuestMemory::new(&[(GuestAddress(0u64), 4 * 1024 * 1024)]) + .expect("Creating guest memory failed."); + + let req = Request { + request_type: RequestType::In, + sector: 7, // Disk is 8 sectors long, so this is the last valid sector. + data_addr: GuestAddress(0x1000), + data_len: 512, // Read 1 sector of data. + status_addr: GuestAddress(0), + discard_write_zeroes_seg: None, + }; + + let mut flush_timer = TimerFd::new().expect("failed to create flush_timer"); + let mut flush_timer_armed = false; + + assert_eq!( + 512, + req.execute( + false, + &mut f, + disk_size, + &mut flush_timer, + &mut flush_timer_armed, + &mem + ) + .expect("execute failed"), + ); + } + + #[test] + fn read_beyond_last_sector() { + let tempdir = TempDir::new("/tmp/block_read_test").unwrap(); + let mut path = PathBuf::from(tempdir.as_path().unwrap()); + path.push("disk_image"); + let mut f = OpenOptions::new() + .read(true) + .write(true) + .create(true) + .open(&path) + .unwrap(); + let disk_size = 0x1000; + f.set_len(disk_size).unwrap(); + + let mem = GuestMemory::new(&[(GuestAddress(0u64), 4 * 1024 * 1024)]) + .expect("Creating guest memory failed."); + + let req = Request { + request_type: RequestType::In, + sector: 7, // Disk is 8 sectors long, so this is the last valid sector. + data_addr: GuestAddress(0x1000), + data_len: 512 * 2, // Read 2 sectors of data (overlap the end of the disk). + status_addr: GuestAddress(0), + discard_write_zeroes_seg: None, + }; + + let mut flush_timer = TimerFd::new().expect("failed to create flush_timer"); + let mut flush_timer_armed = false; + + req.execute( + false, + &mut f, + disk_size, + &mut flush_timer, + &mut flush_timer_armed, + &mem, + ) + .expect_err("execute was supposed to fail"); + } }