From e7518f778609d86c3b35febfeae566c9bbc6eb9c Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Fri, 21 Mar 2025 10:18:33 +0000 Subject: [PATCH] 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 --- CHANGELOG.md | 3 +++ src/mmap/mod.rs | 30 ------------------------------ src/mmap/unix.rs | 19 ++----------------- src/mmap/xen.rs | 8 -------- 4 files changed, 5 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1881e7bb..6dbf4b97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,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 9f9f1939..9e738d76 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)