CVE-2023-41051: Out-of-bounds Read (CWE-125)
value: In a typical Virtual Machine Monitor (VMM) there are several components, such as boot loader, virtual device drivers, virtio backend drivers and vhost drivers, that need to access the VM physical memory. The vm-memory rust crate provides a set of traits to decouple VM memory consumers from VM memory providers. 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. The issue started in version 0.1.0 but was fixed in version 0.12.2 by inserting a check that verifies that the `VolatileSlice` returned by `get_slice` is of the correct length. Users are advised to upgrade. There are no known workarounds for this issue.

CWE description: The product reads data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 4.7
Published: 2023-09-01T19:15Z
Repository: https://github.com/rust-vmm/vm-memory
Language: Rust
Files changed: CHANGELOG.md, Cargo.toml, src/volatile_memory.rs
Commit date: 2023-08-29 13:27:23 +0000
Commit message: 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>

Diff: diff --git a/CHANGELOG.md b/CHANGELOG.md
index 77dbf8c..a70cb03 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,30 +1,25 @@
 # 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
 - [[#241]](https://github.com/rust-vmm/vm-memory/pull/241) Add Xen memory
   mapping support: Foreign and Grant. Add new API for accessing pointers to
diff --git a/Cargo.toml b/Cargo.toml
index f3aa9ee..cd63941 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,8 +1,8 @@
 [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"]
 authors = ["Liu Jiang <gerry@linux.alibaba.com>"]
 repository = "https://github.com/rust-vmm/vm-memory"
diff --git a/src/volatile_memory.rs b/src/volatile_memory.rs
index a588bf5..76e41bb 100644
--- a/src/volatile_memory.rs
+++ b/src/volatile_memory.rs
@@ -107,22 +107,36 @@ pub trait VolatileMemory {
         self.len() == 0
     }
 
     /// 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.
     fn as_volatile_slice(&self) -> VolatileSlice<BS<Self::B>> {
         self.get_slice(0, self.len()).unwrap()
     }
 
     /// 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,
                 slice.bitmap,
                 slice.mmap,
@@ -144,12 +158,22 @@ pub trait VolatileMemory {
             .ok_or(Error::TooBig {
                 nelements: n,
                 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,
                 n,
                 slice.bitmap,
@@ -169,11 +193,25 @@ pub trait VolatileMemory {
     /// If the resulting pointer is not aligned, this method will return an
     /// [`Error`](enum.Error.html).
     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
     /// using the resulting reference are not automatically accounted for by the dirty bitmap
     /// tracking functionality.
@@ -189,11 +227,25 @@ pub trait VolatileMemory {
     /// [`Error`](enum.Error.html).
     unsafe fn aligned_as_mut<T: ByteValued>(&self, offset: usize) -> Result<&mut T> {
         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
     /// using the resulting reference are not automatically accounted for by the dirty bitmap
     /// tracking functionality.
@@ -204,12 +256,22 @@ pub trait VolatileMemory {
     /// [`Error`](enum.Error.html).
     fn get_atomic_ref<T: AtomicInteger>(&self, offset: usize) -> Result<&T> {
         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)) }
     }
 
     /// Returns the sum of `base` and `offset` if the resulting address is valid.
     fn compute_end_offset(&self, base: usize, offset: usize) -> Result<usize> {

Vulnerable code: ## [Unreleased]
### Added

### Changed
## [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()`.

version = "0.12.1"
        // SAFETY: This is safe because the pointer is range-checked by get_slice, and
        // the lifetime is the same as self.
        // SAFETY: This is safe because the pointer is range-checked by get_slice, and
        // the lifetime is the same as self.
        Ok(&*(slice.addr as *const T))
        Ok(&mut *(slice.addr as *mut T))
        // SAFETY: This is safe because the pointer is range-checked by get_slice, and
        // the lifetime is the same as self.
Fixed code: ## [v0.12.2]
- [[#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.
- [[#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()`.
version = "0.12.2"
    ///
    /// 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.

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

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

        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)) }
        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)) }
        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`.