disk_*: handle out of range accesses

Cargo.lock

@@ -1329,9 +1329,11 @@ dependencies = [
  "disk_file",
  "disk_vhd1",
  "futures",
+ "guestmem",
  "guid",
  "inspect",
  "mesh",
+ "pal_async",
  "scsi_buffers",
  "tempfile",
  "thiserror 2.0.0",

vm/devices/storage/disk_blockdevice/src/ioctl.rs

@@ -70,12 +70,9 @@ open_enum! {
 }
 
 pub fn discard(file: &fs::File, start: u64, len: u64) -> std::io::Result<()> {
-    let info = file.metadata()?;
-    if info.file_type().is_block_device() {
-        // SAFETY: The FD is owned by the corresponding File, and this IOCTL is legal to call on any valid FD.
-        //         More documentation on this specific ioctl can be found in fs.h.
-        unsafe { blk_discard_ioctl(file.as_raw_fd(), &[start, len])? };
-    }
+    // SAFETY: The FD is owned by the corresponding File, and this IOCTL is legal to call on any valid FD.
+    //         More documentation on this specific ioctl can be found in fs.h.
+    unsafe { blk_discard_ioctl(file.as_raw_fd(), &[start, len])? };
     Ok(())
 }
 

vm/devices/storage/disk_blockdevice/src/lib.rs

@@ -173,7 +173,9 @@ struct ResizeEpoch {
 #[derive(Debug, Copy, Clone, Inspect)]
 #[inspect(tag = "device_type")]
 enum DeviceType {
-    File,
+    File {
+        sector_count: u64,
+    },
     UnknownBlock,
     NVMe {
         ns_id: u32,
@@ -190,7 +192,7 @@ impl BlockDevice {
                 });
             }
             DeviceType::UnknownBlock => {}
-            DeviceType::File => {}
+            DeviceType::File { .. } => {}
         }
     }
 
@@ -362,12 +364,14 @@ impl BlockDevice {
     }
 
     fn map_io_error(&self, err: std::io::Error) -> DiskError {
-        if !matches!(self.device_type, DeviceType::File) && err.raw_os_error() == Some(libc::EBADE)
-        {
-            DiskError::ReservationConflict
-        } else {
-            DiskError::Io(err)
+        if !matches!(self.device_type, DeviceType::File { .. }) {
+            match err.raw_os_error() {
+                Some(libc::EBADE) => return DiskError::ReservationConflict,
+                Some(libc::ENOSPC) => return DiskError::IllegalBlock,
+                _ => {}
+            }
         }
+        DiskError::Io(err)
     }
 }
 
@@ -451,10 +455,13 @@ impl DeviceMetadata {
     }
 
     fn from_file(metadata: &fs::Metadata) -> anyhow::Result<Self> {
+        let logical_block_size = 512;
         Self {
-            device_type: DeviceType::File,
+            device_type: DeviceType::File {
+                sector_count: metadata.len() / logical_block_size as u64,
+            },
             disk_size: metadata.size(),
-            logical_block_size: 512,
+            logical_block_size,
             physical_block_size: metadata.blksize() as u32,
             discard_granularity: 0,
             supports_pr: false,
@@ -596,7 +603,7 @@ impl DiskIo for BlockDevice {
         let bytes_read = r.map_err(|err| self.map_io_error(err))?;
         tracing::trace!(bytes_read, "read_vectored");
         if bytes_read != io_size as i32 {
-            return Err(DiskError::Io(std::io::ErrorKind::UnexpectedEof.into()));
+            return Err(DiskError::IllegalBlock);
         }
 
         if let Some(mut bounce_buffer) = bounce_buffer {
@@ -616,6 +623,13 @@ impl DiskIo for BlockDevice {
         let io_size = buffers.len();
         tracing::trace!(sector, io_size, "write_vectored");
 
+        // Ensure the write doesn't extend the file.
+        if let DeviceType::File { sector_count } = self.device_type {
+            if sector + (io_size as u64 >> self.sector_shift) > sector_count {
+                return Err(DiskError::IllegalBlock);
+            }
+        }
+
         let mut bounce_buffer;
         let locked;
         let should_bounce = self.always_bounce || !buffers.is_aligned(self.sector_size() as usize);
@@ -656,7 +670,7 @@ impl DiskIo for BlockDevice {
         let bytes_written = r.map_err(|err| self.map_io_error(err))?;
         tracing::trace!(bytes_written, "write_vectored");
         if bytes_written != io_size as i32 {
-            return Err(DiskError::Io(std::io::ErrorKind::UnexpectedEof.into()));
+            return Err(DiskError::IllegalBlock);
         }
 
         Ok(())
@@ -699,9 +713,13 @@ impl Unmap for BlockDevice {
         let file_offset = sector_offset << self.sector_shift;
         let length = sector_count << self.sector_shift;
         tracing::debug!(file = ?file, file_offset, length, "unmap_async");
-        unblock(move || ioctl::discard(&file, file_offset, length))
-            .await
-            .map_err(|err| self.map_io_error(err))?;
+        match unblock(move || ioctl::discard(&file, file_offset, length)).await {
+            Ok(()) => {}
+            Err(_) if sector_offset + sector_count > self.sector_count() => {
+                return Err(DiskError::IllegalBlock)
+            }
+            Err(err) => return Err(self.map_io_error(err)),
+        }
         Ok(())
     }
 
@@ -719,7 +737,7 @@ impl PersistentReservation for BlockDevice {
             &DeviceType::NVMe { rescap, .. } => {
                 nvme_common::from_nvme_reservation_capabilities(rescap)
             }
-            DeviceType::File | DeviceType::UnknownBlock => unreachable!(),
+            DeviceType::File { .. } | DeviceType::UnknownBlock => unreachable!(),
         }
     }
 
@@ -975,4 +993,21 @@ mod tests {
     async fn test_async_disk_io_unaligned_fua() {
         run_async_disk_io_unaligned(true).await;
     }
+
+    #[async_test]
+    async fn test_illegal_lba() {
+        let disk = get_block_device_or_skip!();
+        let gm = GuestMemory::allocate(512);
+        match disk
+            .write_vectored(
+                &OwnedRequestBuffers::linear(0, 512, true).buffer(&gm),
+                i64::MAX as u64 / 512,
+                false,
+            )
+            .await
+        {
+            Err(DiskError::IllegalBlock) => {}
+            r => panic!("unexpected result: {:?}", r),
+        }
+    }
 }

vm/devices/storage/disk_file/src/lib.rs

@@ -99,7 +99,9 @@ impl FileDisk {
 
 impl FileDisk {
     pub async fn read(&self, buffers: &RequestBuffers<'_>, sector: u64) -> Result<(), DiskError> {
-        assert!(((sector << self.sector_shift) + buffers.len() as u64) <= self.metadata.disk_size);
+        if ((sector << self.sector_shift) + buffers.len() as u64) > self.metadata.disk_size {
+            return Err(DiskError::IllegalBlock);
+        }
         let mut buffer = vec![0; buffers.len()];
         let file = self.file.clone();
         let offset = sector << self.sector_shift;
@@ -119,7 +121,9 @@ impl FileDisk {
         sector: u64,
         _fua: bool,
     ) -> Result<(), DiskError> {
-        assert!(((sector << self.sector_shift) + buffers.len() as u64) <= self.metadata.disk_size);
+        if ((sector << self.sector_shift) + buffers.len() as u64) > self.metadata.disk_size {
+            return Err(DiskError::IllegalBlock);
+        }
         let mut buffer = vec![0; buffers.len()];
         let file = self.file.clone();
         buffers.reader().read(&mut buffer)?;

vm/devices/storage/disk_get_vmgs/src/lib.rs

@@ -180,6 +180,9 @@ impl DiskIo for GetVmgsDisk {
     ) -> Result<(), DiskError> {
         let mut writer = buffers.writer();
         let mut remaining_sectors = buffers.len() >> self.sector_shift;
+        if sector + remaining_sectors as u64 > self.sector_count {
+            return Err(DiskError::IllegalBlock);
+        }
         while remaining_sectors != 0 {
             let this_sector_count = remaining_sectors.min(self.max_transfer_sectors as usize);
             let data = self
@@ -203,6 +206,9 @@ impl DiskIo for GetVmgsDisk {
     ) -> Result<(), DiskError> {
         let mut reader = buffers.reader();
         let mut remaining_sector_count = buffers.len() >> self.sector_shift;
+        if sector + remaining_sector_count as u64 > self.sector_count {
+            return Err(DiskError::IllegalBlock);
+        }
         while remaining_sector_count != 0 {
             let this_sector_count = remaining_sector_count.min(self.max_transfer_sectors as usize);
             let data = reader.read_n(this_sector_count << self.sector_shift)?;

vm/devices/storage/disk_nvme/nvme_driver/src/tests.rs

@@ -58,7 +58,7 @@ async fn test_nvme_driver(driver: DefaultDriver, allow_dma: bool) {
         },
     );
     nvme.client()
-        .add_namespace(1, disk_ramdisk::ram_disk(1 << 20, false).unwrap())
+        .add_namespace(1, disk_ramdisk::ram_disk(2 << 20, false).unwrap())
         .await
         .unwrap();
 

vm/devices/storage/disk_ramdisk/src/lib.rs

@@ -155,6 +155,9 @@ impl RamLayer {
         let count = buffers.len() / SECTOR_SIZE as usize;
         tracing::trace!(sector, count, "write");
         let mut state = self.state.write();
+        if sector + count as u64 > state.sector_count {
+            return Err(DiskError::IllegalBlock);
+        }
         for i in 0..count {
             let cur = i + sector as usize;
             let buf = buffers.subrange(i * SECTOR_SIZE as usize, SECTOR_SIZE as usize);
@@ -213,6 +216,9 @@ impl LayerIo for RamLayer {
         let end = sector + count;
         tracing::trace!(sector, count, "read");
         let state = self.state.read();
+        if end > state.sector_count {
+            return Err(DiskError::IllegalBlock);
+        }
         let mut range = state.data.range(sector..end);
         let mut last = sector;
         while last < end {
@@ -280,6 +286,9 @@ impl LayerIo for RamLayer {
     ) -> Result<(), DiskError> {
         tracing::trace!(sector_offset, sector_count, "unmap");
         let mut state = self.state.write();
+        if sector_offset + sector_count > state.sector_count {
+            return Err(DiskError::IllegalBlock);
+        }
         if !next_is_zero {
             // This would create a hole of zeroes, which we cannot represent in
             // the tree. Ignore the unmap.

vm/devices/storage/disk_striped/src/lib.rs

@@ -133,14 +133,6 @@ enum IoError {
         end_sector: u64,
         disk_sector_count: u64,
     },
-    #[error(
-        "Sector out of range: start_sector-{start_sector}, end_sector-{end_sector}, self.sector_count-{disk_sector_count}"
-    )]
-    UnmapInvalidInput {
-        start_sector: u64,
-        end_sector: u64,
-        disk_sector_count: u64,
-    },
     #[error("error in lower disk {index}")]
     LowerError {
         index: usize,
@@ -404,6 +396,9 @@ impl DiskIo for StripedDisk {
     ) -> Result<(), DiskError> {
         let buf_total_size = buffers.len();
         let end_sector = start_sector + ((buf_total_size as u64) >> self.sector_shift);
+        if end_sector > self.sector_count {
+            return Err(DiskError::IllegalBlock);
+        }
         let chunk_iter = self.get_chunk_iter(start_sector, end_sector)?;
 
         let mut all_futures = Vec::new();
@@ -447,6 +442,9 @@ impl DiskIo for StripedDisk {
     ) -> Result<(), DiskError> {
         let buf_total_size = buffers.len();
         let end_sector = start_sector + ((buf_total_size as u64) >> self.sector_shift);
+        if end_sector > self.sector_count {
+            return Err(DiskError::IllegalBlock);
+        }
         let chunk_iter = self.get_chunk_iter(start_sector, end_sector)?;
 
         let mut all_futures = Vec::new();
@@ -507,12 +505,7 @@ impl Unmap for StripedDisk {
         let end_sector = start_sector + sector_count;
 
         if end_sector > self.sector_count {
-            return Err(IoError::UnmapInvalidInput {
-                start_sector,
-                end_sector,
-                disk_sector_count: self.sector_count,
-            }
-            .into());
+            return Err(DiskError::IllegalBlock);
         }
 
         let chunk_iter = match self.get_chunk_iter(start_sector, end_sector) {

vm/devices/storage/disk_vhdmp/Cargo.toml

@@ -15,12 +15,15 @@ vm_resource.workspace = true
 guid = { workspace = true, features = ["inspect"] }
 inspect.workspace = true
 mesh.workspace = true
+
 futures.workspace = true
 thiserror.workspace = true
 winapi = { workspace = true, features = [ "handleapi", "winbase", "winnt" ] }
 
 [target.'cfg(windows)'.dev-dependencies]
 disk_vhd1.workspace = true
+guestmem.workspace = true
+pal_async.workspace = true
 tempfile.workspace = true
 
 [lints]

vm/devices/storage/disk_vhdmp/src/lib.rs

@@ -485,7 +485,12 @@ impl DiskIo for VhdmpDisk {
 #[cfg(test)]
 mod tests {
     use super::VhdmpDisk;
+    use disk_backend::DiskError;
+    use disk_backend::DiskIo;
     use disk_vhd1::Vhd1Disk;
+    use guestmem::GuestMemory;
+    use pal_async::async_test;
+    use scsi_buffers::OwnedRequestBuffers;
     use std::io::Write;
     use tempfile::TempPath;
 
@@ -504,4 +509,22 @@ mod tests {
         let _vhd = VhdmpDisk::open_vhd(path.as_ref(), true).unwrap();
         let _vhd = VhdmpDisk::open_vhd(path.as_ref(), false).unwrap_err();
     }
+
+    #[async_test]
+    async fn test_invalid_lba() {
+        let path = make_test_vhd();
+        let vhd = VhdmpDisk::open_vhd(path.as_ref(), true).unwrap();
+        let disk = VhdmpDisk::new(vhd, true).unwrap();
+        let gm = GuestMemory::allocate(512);
+        match disk
+            .read_vectored(
+                &OwnedRequestBuffers::linear(0, 512, true).buffer(&gm),
+                0x10000000,
+            )
+            .await
+        {
+            Err(DiskError::IllegalBlock) => {}
+            r => panic!("unexpected result: {:?}", r),
+        }
+    }
 }

vm/devices/storage/scsidisk/src/scsidvd/mod.rs

@@ -2429,16 +2429,7 @@ mod tests {
             let end_point = offset + buffers.len();
 
             if self.storage.len() < end_point {
-                println!(
-                    "exceed storage limit: storage_len {:?} offset {:?} len {:?}",
-                    self.storage.len(),
-                    offset,
-                    buffers.len()
-                );
-                return Err(DiskError::Io(std::io::Error::new(
-                    std::io::ErrorKind::InvalidInput,
-                    "exceed storage limit",
-                )));
+                return Err(DiskError::IllegalBlock);
             }
 
             buffers.writer().write(&self.storage[offset..end_point])?;

vm/devices/storage/scsidisk/src/tests/test_helpers.rs

@@ -114,16 +114,7 @@ impl DiskIo for TestDisk {
         let end_point = offset + buffers.len();
         let mut state = self.state.lock();
         if state.storage.len() < end_point {
-            println!(
-                "exceed storage limit: storage_len {:?} offset {:?} len {:?}",
-                state.storage.len(),
-                offset,
-                buffers.len()
-            );
-            return Err(DiskError::Io(std::io::Error::new(
-                std::io::ErrorKind::InvalidInput,
-                "exceed storage limit",
-            )));
+            return Err(DiskError::IllegalBlock);
         }
         buffers.writer().write(&state.storage[offset..end_point])?;
         state.is_fua_set = false;
@@ -140,16 +131,7 @@ impl DiskIo for TestDisk {
         let end_point = offset + buffers.len();
         let mut state = self.state.lock();
         if state.storage.len() < end_point {
-            println!(
-                "exceed storage limit: storage_len {:?} offset {:?} len {:?}",
-                state.storage.len(),
-                offset,
-                buffers.len()
-            );
-            return Err(DiskError::Io(std::io::Error::new(
-                std::io::ErrorKind::InvalidInput,
-                "exceed storage limit",
-            )));
+            return Err(DiskError::IllegalBlock);
         }
         buffers
             .reader()