fix: Validate return value of get_slice in VolatileMemory

An issue was discovered in the default implementations of the
VolatileMemory::{get_atomic_ref, aligned_as_ref, aligned_as_mut,
get_ref, get_array_ref} trait functions, which allows out-of-bounds
memory access if the VolatileMemory::get_slice function returns a
VolatileSlice whose length is less than the function’s count argument.
No implementations of get_slice provided in vm_memory are affected.
Users of custom VolatileMemory implementations may be impacted if the
custom implementation does not adhere to get_slice's documentation.

This commit fixes this issue by inserting a check that verifies that the
VolatileSlice returned by get_slice is of the correct length.

Signed-off-by: Patrick Roy <roypat@amazon.co.uk>

Author: roypat

CHANGELOG.md

@@ -1,28 +1,23 @@
 # Changelog
-## [Unreleased]
 
-### Added
-
-### Changed
+## [v0.12.2]
 
 ### Fixed
+- [[#251]](https://github.com/rust-vmm/vm-memory/pull/251): Inserted checks
+  that verify that the value returned by `VolatileMemory::get_slice` is of
+  the correct length.
 
 ### Deprecated
+- [[#244]](https://github.com/rust-vmm/vm-memory/pull/241) Deprecate volatile
+  memory's `as_ptr()` interfaces. The new interfaces to be used instead are:
+  `ptr_guard()` and `ptr_guard_mut()`.
 
 ## [v0.12.1]
 
 ### Fixed
 - [[#241]](https://github.com/rust-vmm/vm-memory/pull/245) mmap_xen: Don't drop
   the FileOffset while in use #245
 
-## [Unreleased]
-
-### Deprecated
-
-- [[#244]](https://github.com/rust-vmm/vm-memory/pull/241) Deprecate volatile
-  memory's `as_ptr()` interfaces. The new interfaces to be used instead are:
-  `ptr_guard()` and `ptr_guard_mut()`.
-
 ## [v0.12.0]
 
 ### Added

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "vm-memory"
-version = "0.12.1"
+version = "0.12.2"
 description = "Safe abstractions for accessing the VM physical memory"
 keywords = ["memory"]
 categories = ["memory-management"]

src/volatile_memory.rs

@@ -109,6 +109,10 @@ pub trait VolatileMemory {
 
     /// Returns a [`VolatileSlice`](struct.VolatileSlice.html) of `count` bytes starting at
     /// `offset`.
+    ///
+    /// Note that the property `get_slice(offset, count).len() == count` MUST NOT be
+    /// relied on for the correctness of unsafe code. This is a safe function inside of a
+    /// safe trait, and implementors are under no obligation to follow its documentation.
     fn get_slice(&self, offset: usize, count: usize) -> Result<VolatileSlice<BS<Self::B>>>;
 
     /// Gets a slice of memory for the entire region that supports volatile access.
@@ -119,8 +123,18 @@ pub trait VolatileMemory {
     /// Gets a `VolatileRef` at `offset`.
     fn get_ref<T: ByteValued>(&self, offset: usize) -> Result<VolatileRef<T, BS<Self::B>>> {
         let slice = self.get_slice(offset, size_of::<T>())?;
-        // SAFETY: This is safe because the pointer is range-checked by get_slice, and
-        // the lifetime is the same as self.
+
+        assert_eq!(
+            slice.len(),
+            size_of::<T>(),
+            "VolatileMemory::get_slice(offset, count) returned slice of length != count."
+        );
+
+        // SAFETY: This is safe because the invariants of the constructors of VolatileSlice ensure that
+        // slice.addr is valid memory of size slice.len(). The assert above ensures that
+        // the length of the slice is exactly enough to hold one `T`. Lastly, the lifetime of the
+        // returned VolatileRef match that of the VolatileSlice returned by get_slice and thus the
+        // lifetime one `self`.
         unsafe {
             Ok(VolatileRef::with_bitmap(
                 slice.addr,
@@ -146,8 +160,18 @@ pub trait VolatileMemory {
                 size: size_of::<T>(),
             })?;
         let slice = self.get_slice(offset, nbytes as usize)?;
-        // SAFETY: This is safe because the pointer is range-checked by get_slice, and
-        // the lifetime is the same as self.
+
+        assert_eq!(
+            slice.len(),
+            nbytes as usize,
+            "VolatileMemory::get_slice(offset, count) returned slice of length != count."
+        );
+
+        // SAFETY: This is safe because the invariants of the constructors of VolatileSlice ensure that
+        // slice.addr is valid memory of size slice.len(). The assert above ensures that
+        // the length of the slice is exactly enough to hold `n` instances of `T`. Lastly, the lifetime of the
+        // returned VolatileArrayRef match that of the VolatileSlice returned by get_slice and thus the
+        // lifetime one `self`.
         unsafe {
             Ok(VolatileArrayRef::with_bitmap(
                 slice.addr,
@@ -171,7 +195,21 @@ pub trait VolatileMemory {
     unsafe fn aligned_as_ref<T: ByteValued>(&self, offset: usize) -> Result<&T> {
         let slice = self.get_slice(offset, size_of::<T>())?;
         slice.check_alignment(align_of::<T>())?;
-        Ok(&*(slice.addr as *const T))
+
+        assert_eq!(
+            slice.len(),
+            size_of::<T>(),
+            "VolatileMemory::get_slice(offset, count) returned slice of length != count."
+        );
+
+        // SAFETY: This is safe because the invariants of the constructors of VolatileSlice ensure that
+        // slice.addr is valid memory of size slice.len(). The assert above ensures that
+        // the length of the slice is exactly enough to hold one `T`.
+        // Dereferencing the pointer is safe because we check the alignment above, and the invariants
+        // of this function ensure that no aliasing pointers exist. Lastly, the lifetime of the
+        // returned VolatileArrayRef match that of the VolatileSlice returned by get_slice and thus the
+        // lifetime one `self`.
+        unsafe { Ok(&*(slice.addr as *const T)) }
     }
 
     /// Returns a mutable reference to an instance of `T` at `offset`. Mutable accesses performed
@@ -191,7 +229,21 @@ pub trait VolatileMemory {
         let slice = self.get_slice(offset, size_of::<T>())?;
         slice.check_alignment(align_of::<T>())?;
 
-        Ok(&mut *(slice.addr as *mut T))
+        assert_eq!(
+            slice.len(),
+            size_of::<T>(),
+            "VolatileMemory::get_slice(offset, count) returned slice of length != count."
+        );
+
+        // SAFETY: This is safe because the invariants of the constructors of VolatileSlice ensure that
+        // slice.addr is valid memory of size slice.len(). The assert above ensures that
+        // the length of the slice is exactly enough to hold one `T`.
+        // Dereferencing the pointer is safe because we check the alignment above, and the invariants
+        // of this function ensure that no aliasing pointers exist. Lastly, the lifetime of the
+        // returned VolatileArrayRef match that of the VolatileSlice returned by get_slice and thus the
+        // lifetime one `self`.
+
+        unsafe { Ok(&mut *(slice.addr as *mut T)) }
     }
 
     /// Returns a reference to an instance of `T` at `offset`. Mutable accesses performed
@@ -206,8 +258,18 @@ pub trait VolatileMemory {
         let slice = self.get_slice(offset, size_of::<T>())?;
         slice.check_alignment(align_of::<T>())?;
 
-        // SAFETY: This is safe because the pointer is range-checked by get_slice, and
-        // the lifetime is the same as self.
+        assert_eq!(
+            slice.len(),
+            size_of::<T>(),
+            "VolatileMemory::get_slice(offset, count) returned slice of length != count."
+        );
+
+        // SAFETY: This is safe because the invariants of the constructors of VolatileSlice ensure that
+        // slice.addr is valid memory of size slice.len(). The assert above ensures that
+        // the length of the slice is exactly enough to hold one `T`.
+        // Dereferencing the pointer is safe because we check the alignment above. Lastly, the lifetime of the
+        // returned VolatileArrayRef match that of the VolatileSlice returned by get_slice and thus the
+        // lifetime one `self`.
         unsafe { Ok(&*(slice.addr as *const T)) }
     }