diff --git a/CHANGELOG.md b/CHANGELOG.md index 532e96fc..4f43bd83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,9 @@ `Bytes::write_to`, `Bytes::write_all_to`, `GuestMemory::as_slice`, `GuestMemory::as_slice_mut`, `GuestMemory::with_regions`, `GuestMemory::with_regions_mut`, `GuestMemory::map_and_fold`, `VolatileSlice::as_ptr`, `VolatileRef::as_ptr`, and `VolatileArrayRef::as_ptr`. +- \[[#320](https://github.com/rust-vmm/vm-memory/pull/320)\] Drop `check_file_offset` check when using `MmapRegionBuilder`. + The `mmap(2)` syscall itself already validates that offset and length are valid, and trying to replicate this check + in userspace ended up being imperfect. ## \[v0.16.1\] diff --git a/src/mmap/mod.rs b/src/mmap/mod.rs index 9f579d74..b8dd0842 100644 --- a/src/mmap/mod.rs +++ b/src/mmap/mod.rs @@ -13,8 +13,6 @@ //! This implementation is mmap-ing the memory of the guest into the current process. use std::borrow::Borrow; -#[cfg(target_family = "unix")] -use std::io::{Seek, SeekFrom}; use std::ops::Deref; use std::result; use std::sync::atomic::Ordering; @@ -72,34 +70,6 @@ pub enum Error { UnsortedMemoryRegions, } -// TODO: use this for Windows as well after we redefine the Error type there. -#[cfg(target_family = "unix")] -/// Checks if a mapping of `size` bytes fits at the provided `file_offset`. -/// -/// For a borrowed `FileOffset` and size, this function checks whether the mapping does not -/// extend past EOF, and that adding the size to the file offset does not lead to overflow. -pub fn check_file_offset( - file_offset: &FileOffset, - size: usize, -) -> result::Result<(), MmapRegionError> { - let mut file = file_offset.file(); - let start = file_offset.start(); - - if let Some(end) = start.checked_add(size as u64) { - let filesize = file - .seek(SeekFrom::End(0)) - .map_err(MmapRegionError::SeekEnd)?; - file.rewind().map_err(MmapRegionError::SeekStart)?; - if filesize < end { - return Err(MmapRegionError::MappingPastEof); - } - } else { - return Err(MmapRegionError::InvalidOffsetLength); - } - - Ok(()) -} - /// [`GuestMemoryRegion`](trait.GuestMemoryRegion.html) implementation that mmaps the guest's /// memory region in the current process. /// diff --git a/src/mmap/unix.rs b/src/mmap/unix.rs index 752f05f1..b5ee5b04 100644 --- a/src/mmap/unix.rs +++ b/src/mmap/unix.rs @@ -17,7 +17,6 @@ use std::result; use crate::bitmap::{Bitmap, NewBitmap, BS}; use crate::guest_memory::FileOffset; -use crate::mmap::check_file_offset; use crate::volatile_memory::{self, VolatileMemory, VolatileSlice}; /// Error conditions that may arise when creating a new `MmapRegion` object. @@ -41,12 +40,6 @@ pub enum Error { /// The `mmap` call returned an error. #[error("{0}")] Mmap(io::Error), - /// Seeking the end of the file returned an error. - #[error("Error seeking the end of the file: {0}")] - SeekEnd(io::Error), - /// Seeking the start of the file returned an error. - #[error("Error seeking the start of the file: {0}")] - SeekStart(io::Error), } pub type Result = result::Result; @@ -137,7 +130,6 @@ impl MmapRegionBuilder { } let (fd, offset) = if let Some(ref f_off) = self.file_offset { - check_file_offset(f_off, self.size)?; (f_off.file().as_raw_fd(), f_off.start()) } else { (-1, 0) @@ -558,16 +550,9 @@ mod tests { prot, flags, ); - assert_eq!(format!("{:?}", r.unwrap_err()), "InvalidOffsetLength"); - - // Offset + size is greater than the size of the file (which is 0 at this point). - let r = MmapRegion::build( - Some(FileOffset::from_arc(a.clone(), offset)), - size, - prot, - flags, + assert!( + matches!(r.unwrap_err(), Error::Mmap(err) if err.raw_os_error() == Some(libc::EINVAL)) ); - assert_eq!(format!("{:?}", r.unwrap_err()), "MappingPastEof"); // MAP_FIXED was specified among the flags. let r = MmapRegion::build( diff --git a/src/mmap/xen.rs b/src/mmap/xen.rs index 7c5c0670..0d96dd47 100644 --- a/src/mmap/xen.rs +++ b/src/mmap/xen.rs @@ -26,7 +26,6 @@ use tests::ioctl_with_ref; use crate::bitmap::{Bitmap, NewBitmap, BS}; use crate::guest_memory::{FileOffset, GuestAddress}; -use crate::mmap::check_file_offset; use crate::volatile_memory::{self, VolatileMemory, VolatileSlice}; /// Error conditions that may arise when creating a new `MmapRegion` object. @@ -44,12 +43,6 @@ pub enum Error { /// The `mmap` call returned an error. #[error("{0}")] Mmap(io::Error), - /// Seeking the end of the file returned an error. - #[error("Error seeking the end of the file: {0}")] - SeekEnd(io::Error), - /// Seeking the start of the file returned an error. - #[error("Error seeking the start of the file: {0}")] - SeekStart(io::Error), /// Invalid file offset. #[error("Invalid file offset")] InvalidFileOffset, @@ -524,7 +517,6 @@ struct MmapXenUnix(MmapUnix); impl MmapXenUnix { fn new(range: &MmapRange) -> Result { let (fd, offset) = if let Some(ref f_off) = range.file_offset { - check_file_offset(f_off, range.size)?; (f_off.file().as_raw_fd(), f_off.start()) } else { (-1, 0)