From aff1dd4a5259f7deba56692840f7a2d9ca34c9c8 Mon Sep 17 00:00:00 2001 From: Patrick Roy Date: Tue, 29 Aug 2023 09:25:54 +0100 Subject: [PATCH] fix: Validate return value of get_slice in VolatileMemory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 19 ++++------ Cargo.toml | 2 +- src/volatile_memory.rs | 78 +++++++++++++++++++++++++++++++++++++----- 3 files changed, 78 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77dbf8c0..a70cb036 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,13 +1,16 @@ # 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] @@ -15,14 +18,6 @@ - [[#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 diff --git a/Cargo.toml b/Cargo.toml index f3aa9ee6..cd63941b 100644 --- a/Cargo.toml +++ b/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"] diff --git a/src/volatile_memory.rs b/src/volatile_memory.rs index a588bf54..76e41bb7 100644 --- a/src/volatile_memory.rs +++ b/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>>; /// 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(&self, offset: usize) -> Result>> { let slice = self.get_slice(offset, size_of::())?; - // 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::(), + "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::(), })?; 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(&self, offset: usize) -> Result<&T> { let slice = self.get_slice(offset, size_of::())?; slice.check_alignment(align_of::())?; - Ok(&*(slice.addr as *const T)) + + assert_eq!( + slice.len(), + size_of::(), + "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::())?; slice.check_alignment(align_of::())?; - Ok(&mut *(slice.addr as *mut T)) + assert_eq!( + slice.len(), + size_of::(), + "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::())?; slice.check_alignment(align_of::())?; - // 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::(), + "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)) } }