mmap: drop file-offset checking

When mmap-ing a file-like object, vm-memory was trying to validate the
the range [offset, offset+len) was valid in the file. However, our
homegrown check had lots of edge cases where it gave false-positives
(e.g. it rejected things that should really be working, such as vfio
devices, or fds that cannot be seeked like guest_memfd), and trying to
implement a check that works for all of these is in the end wasted
effort anyway, because the kernel validates all this as part of the mmap
syscall anyway. So just drop these checks in favor of failing at
mmap-time.

Drop a test case about mmap-ing a file of size 0 - this works because
the mmap will simply grow the file.

Signed-off-by: Patrick Roy <[email protected]>

Author: roypat

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\]
 

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.
 ///

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<T> = result::Result<T, Error>;
@@ -137,7 +130,6 @@ impl<B: Bitmap> MmapRegionBuilder<B> {
         }
 
         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(

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<Self> {
         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)