diff --git a/CHANGELOG.md b/CHANGELOG.md index a70cb036..de581a00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,21 @@ # Changelog +## Unreleased + +### Added +- [[#247]](https://github.com/rust-vmm/vm-memory/pull/247) Add `ReadVolatile` and + `WriteVolatile` traits which are equivalents of `Read`/`Write` with volatile + access semantics. + +### Changed + +- [[#247]](https://github.com/rust-vmm/vm-memory/pull/247) Deprecate + `Bytes::{read_from, read_exact_from, write_to, write_all_to}`. Instead use + `ReadVolatile`/`WriteVolatile`, which do not incur the performance penalty + of copying to hypervisor memory due to `Read`/`Write` being incompatible + with volatile semantics (see also #217). + + ## [v0.12.2] ### Fixed diff --git a/benches/mmap/mod.rs b/benches/mmap/mod.rs index ed15e18f..bbf3ab31 100644 --- a/benches/mmap/mod.rs +++ b/benches/mmap/mod.rs @@ -8,7 +8,6 @@ extern crate criterion; extern crate vm_memory; use std::fs::{File, OpenOptions}; -use std::io::Cursor; use std::mem::size_of; use std::path::Path; @@ -105,7 +104,7 @@ pub fn benchmark_for_mmap(c: &mut Criterion) { c.bench_function(format!("read_from_{:#0X}", offset).as_str(), |b| { b.iter(|| { black_box(&memory) - .read_from(address, &mut Cursor::new(&image), ACCESS_SIZE) + .read_volatile_from(address, &mut image.as_slice(), ACCESS_SIZE) .unwrap() }) }); @@ -113,7 +112,7 @@ pub fn benchmark_for_mmap(c: &mut Criterion) { c.bench_function(format!("read_from_file_{:#0X}", offset).as_str(), |b| { b.iter(|| { black_box(&memory) - .read_from(address, &mut file, ACCESS_SIZE) + .read_volatile_from(address, &mut file, ACCESS_SIZE) .unwrap() }) }); @@ -121,7 +120,7 @@ pub fn benchmark_for_mmap(c: &mut Criterion) { c.bench_function(format!("read_exact_from_{:#0X}", offset).as_str(), |b| { b.iter(|| { black_box(&memory) - .read_exact_from(address, &mut Cursor::new(&mut image), ACCESS_SIZE) + .read_exact_volatile_from(address, &mut image.as_slice(), ACCESS_SIZE) .unwrap() }) }); @@ -154,7 +153,7 @@ pub fn benchmark_for_mmap(c: &mut Criterion) { c.bench_function(format!("write_to_{:#0X}", offset).as_str(), |b| { b.iter(|| { black_box(&memory) - .write_to(address, &mut Cursor::new(&mut image), ACCESS_SIZE) + .write_volatile_to(address, &mut image.as_mut_slice(), ACCESS_SIZE) .unwrap() }) }); @@ -162,7 +161,7 @@ pub fn benchmark_for_mmap(c: &mut Criterion) { c.bench_function(format!("write_to_file_{:#0X}", offset).as_str(), |b| { b.iter(|| { black_box(&memory) - .write_to(address, &mut file_to_write, ACCESS_SIZE) + .write_volatile_to(address, &mut file_to_write, ACCESS_SIZE) .unwrap() }) }); @@ -170,7 +169,7 @@ pub fn benchmark_for_mmap(c: &mut Criterion) { c.bench_function(format!("write_exact_to_{:#0X}", offset).as_str(), |b| { b.iter(|| { black_box(&memory) - .write_all_to(address, &mut Cursor::new(&mut image), ACCESS_SIZE) + .write_all_volatile_to(address, &mut image.as_mut_slice(), ACCESS_SIZE) .unwrap() }) }); diff --git a/coverage_config_x86_64.json b/coverage_config_x86_64.json index a6a09c43..e0da42ce 100644 --- a/coverage_config_x86_64.json +++ b/coverage_config_x86_64.json @@ -1,5 +1,5 @@ { - "coverage_score": 92.2, + "coverage_score": 91.2, "exclude_path": "mmap_windows.rs", "crate_features": "backend-mmap,backend-atomic,backend-bitmap" } diff --git a/src/bitmap/mod.rs b/src/bitmap/mod.rs index e8c09879..2f03ceb6 100644 --- a/src/bitmap/mod.rs +++ b/src/bitmap/mod.rs @@ -267,6 +267,7 @@ pub(crate) mod tests { dirty_offset += step; // Test `read_from`. + #[allow(deprecated)] // test of deprecated functions h.test_access(bytes, dirty_offset, BUF_SIZE, |m, addr| { assert_eq!( m.read_from(addr, &mut Cursor::new(&buf), BUF_SIZE).unwrap(), @@ -277,6 +278,7 @@ pub(crate) mod tests { dirty_offset += step; // Test `read_exact_from`. + #[allow(deprecated)] // test of deprecated functions h.test_access(bytes, dirty_offset, BUF_SIZE, |m, addr| { m.read_exact_from(addr, &mut Cursor::new(&buf), BUF_SIZE) .unwrap() diff --git a/src/bytes.rs b/src/bytes.rs index 24307084..58bfe3b1 100644 --- a/src/bytes.rs +++ b/src/bytes.rs @@ -280,6 +280,9 @@ pub trait Bytes { /// * `addr` - Begin writing at this address. /// * `src` - Copy from `src` into the container. /// * `count` - Copy `count` bytes from `src` into the container. + #[deprecated( + note = "Use `.read_volatile_from` or the functions of the `ReadVolatile` trait instead" + )] fn read_from(&self, addr: A, src: &mut F, count: usize) -> Result where F: Read; @@ -295,6 +298,9 @@ pub trait Bytes { /// * `addr` - Begin writing at this address. /// * `src` - Copy from `src` into the container. /// * `count` - Copy exactly `count` bytes from `src` into the container. + #[deprecated( + note = "Use `.read_exact_volatile_from` or the functions of the `ReadVolatile` trait instead" + )] fn read_exact_from(&self, addr: A, src: &mut F, count: usize) -> Result<(), Self::E> where F: Read; @@ -307,6 +313,9 @@ pub trait Bytes { /// * `addr` - Begin reading from this address. /// * `dst` - Copy from the container to `dst`. /// * `count` - Copy `count` bytes from the container to `dst`. + #[deprecated( + note = "Use `.write_volatile_to` or the functions of the `WriteVolatile` trait instead" + )] fn write_to(&self, addr: A, dst: &mut F, count: usize) -> Result where F: Write; @@ -322,6 +331,9 @@ pub trait Bytes { /// * `addr` - Begin reading from this address. /// * `dst` - Copy from the container to `dst`. /// * `count` - Copy exactly `count` bytes from the container to `dst`. + #[deprecated( + note = "Use `.write_all_volatile_to` or the functions of the `WriteVolatile` trait instead" + )] fn write_all_to(&self, addr: A, dst: &mut F, count: usize) -> Result<(), Self::E> where F: Write; diff --git a/src/guest_memory.rs b/src/guest_memory.rs index ba615efd..730d0322 100644 --- a/src/guest_memory.rs +++ b/src/guest_memory.rs @@ -52,7 +52,9 @@ use std::sync::Arc; use crate::address::{Address, AddressValue}; use crate::bitmap::{Bitmap, BS, MS}; use crate::bytes::{AtomicAccess, Bytes}; +use crate::io::{ReadVolatile, WriteVolatile}; use crate::volatile_memory::{self, VolatileSlice}; +use crate::GuestMemoryError; static MAX_ACCESS_CHUNK: usize = 4096; @@ -665,6 +667,146 @@ pub trait GuestMemory { } } + /// Reads up to `count` bytes from an object and writes them into guest memory at `addr`. + /// + /// Returns the number of bytes written into guest memory. + /// + /// # Arguments + /// * `addr` - Begin writing at this address. + /// * `src` - Copy from `src` into the container. + /// * `count` - Copy `count` bytes from `src` into the container. + /// + /// # Examples + /// + /// * Read bytes from /dev/urandom (uses the `backend-mmap` feature) + /// + /// ``` + /// # #[cfg(feature = "backend-mmap")] + /// # { + /// # use vm_memory::{Address, GuestMemory, Bytes, GuestAddress, GuestMemoryMmap}; + /// # use std::fs::File; + /// # use std::path::Path; + /// # + /// # let start_addr = GuestAddress(0x1000); + /// # let gm = GuestMemoryMmap::<()>::from_ranges(&vec![(start_addr, 0x400)]) + /// # .expect("Could not create guest memory"); + /// # let addr = GuestAddress(0x1010); + /// # let mut file = if cfg!(unix) { + /// let mut file = File::open(Path::new("/dev/urandom")).expect("Could not open /dev/urandom"); + /// # file + /// # } else { + /// # File::open(Path::new("c:\\Windows\\system32\\ntoskrnl.exe")) + /// # .expect("Could not open c:\\Windows\\system32\\ntoskrnl.exe") + /// # }; + /// + /// gm.read_volatile_from(addr, &mut file, 128) + /// .expect("Could not read from /dev/urandom into guest memory"); + /// + /// let read_addr = addr.checked_add(8).expect("Could not compute read address"); + /// let rand_val: u32 = gm + /// .read_obj(read_addr) + /// .expect("Could not read u32 val from /dev/urandom"); + /// # } + /// ``` + fn read_volatile_from(&self, addr: GuestAddress, src: &mut F, count: usize) -> Result + where + F: ReadVolatile, + { + self.try_access(count, addr, |offset, len, caddr, region| -> Result { + // Check if something bad happened before doing unsafe things. + assert!(offset <= count); + + let len = std::cmp::min(len, MAX_ACCESS_CHUNK); + + let mut vslice = region.get_slice(caddr, len)?; + + src.read_volatile(&mut vslice) + .map_err(GuestMemoryError::from) + }) + } + + /// Reads up to `count` bytes from the container at `addr` and writes them it into guest memory. + /// + /// Returns the number of bytes written into guest memory. + /// + /// # Arguments + /// * `addr` - Begin reading from this address. + /// * `dst` - Copy from guest memory to `dst`. + /// * `count` - Copy `count` bytes from guest memory to `dst`. + fn write_volatile_to(&self, addr: GuestAddress, dst: &mut F, count: usize) -> Result + where + F: WriteVolatile, + { + self.try_access(count, addr, |offset, len, caddr, region| -> Result { + // Check if something bad happened before doing unsafe things. + assert!(offset <= count); + + let len = std::cmp::min(len, MAX_ACCESS_CHUNK); + let vslice = region.get_slice(caddr, len)?; + + // For a non-RAM region, reading could have side effects, so we + // must use write_all(). + dst.write_all_volatile(&vslice)?; + + Ok(len) + }) + } + + /// Reads exactly `count` bytes from an object and writes them into guest memory at `addr`. + /// + /// # Errors + /// + /// Returns an error if `count` bytes couldn't have been copied from `src` to guest memory. + /// Part of the data may have been copied nevertheless. + /// + /// # Arguments + /// * `addr` - Begin writing at this address. + /// * `src` - Copy from `src` into guest memory. + /// * `count` - Copy exactly `count` bytes from `src` into guest memory. + fn read_exact_volatile_from( + &self, + addr: GuestAddress, + src: &mut F, + count: usize, + ) -> Result<()> + where + F: ReadVolatile, + { + let res = self.read_volatile_from(addr, src, count)?; + if res != count { + return Err(Error::PartialBuffer { + expected: count, + completed: res, + }); + } + Ok(()) + } + + /// Reads exactly `count` bytes from the container at `addr` and writes them into guest memory. + /// + /// # Errors + /// + /// Returns an error if `count` bytes couldn't have been copied from guest memory to `dst`. + /// Part of the data may have been copied nevertheless. + /// + /// # Arguments + /// * `addr` - Begin reading from this address. + /// * `dst` - Copy from guest memory to `dst`. + /// * `count` - Copy exactly `count` bytes from guest memory to `dst`. + fn write_all_volatile_to(&self, addr: GuestAddress, dst: &mut F, count: usize) -> Result<()> + where + F: WriteVolatile, + { + let res = self.write_volatile_to(addr, dst, count)?; + if res != count { + return Err(Error::PartialBuffer { + expected: count, + completed: res, + }); + } + Ok(()) + } + /// Get the host virtual address corresponding to the guest address. /// /// Some [`GuestMemory`](trait.GuestMemory.html) implementations, like `GuestMemoryMmap`, @@ -856,6 +998,7 @@ impl Bytes for T { where F: Read, { + #[allow(deprecated)] // this function itself is deprecated let res = self.read_from(addr, src, count)?; if res != count { return Err(Error::PartialBuffer { @@ -949,6 +1092,7 @@ impl Bytes for T { where F: Write, { + #[allow(deprecated)] // this function itself is deprecated let res = self.write_to(addr, dst, count)?; if res != count { return Err(Error::PartialBuffer { @@ -983,8 +1127,6 @@ mod tests { #[cfg(feature = "backend-mmap")] use crate::GuestAddress; #[cfg(feature = "backend-mmap")] - use std::io::Cursor; - #[cfg(feature = "backend-mmap")] use std::time::{Duration, Instant}; use vmm_sys_util::tempfile::TempFile; @@ -1024,7 +1166,7 @@ mod tests { let count: usize = 0x20; assert_eq!( 0x20_usize, - mem.read_from(offset, &mut Cursor::new(&image), count) + mem.read_volatile_from(offset, &mut image.as_slice(), count) .unwrap() ); } @@ -1179,19 +1321,24 @@ mod tests { assert!(mem.write_obj(obj, addr).is_ok()); assert!(mem.read_obj::(addr).is_ok()); - assert_eq!(mem.read_from(addr, &mut Cursor::new(&image), 0).unwrap(), 0); + assert_eq!( + mem.read_volatile_from(addr, &mut image.as_slice(), 0) + .unwrap(), + 0 + ); assert!(mem - .read_exact_from(addr, &mut Cursor::new(&image), 0) + .read_exact_volatile_from(addr, &mut image.as_slice(), 0) .is_ok()); assert_eq!( - mem.write_to(addr, &mut Cursor::new(&mut image), 0).unwrap(), + mem.write_volatile_to(addr, &mut image.as_mut_slice(), 0) + .unwrap(), 0 ); assert!(mem - .write_all_to(addr, &mut Cursor::new(&mut image), 0) + .write_all_volatile_to(addr, &mut image.as_mut_slice(), 0) .is_ok()); } diff --git a/src/io.rs b/src/io.rs new file mode 100644 index 00000000..482c923a --- /dev/null +++ b/src/io.rs @@ -0,0 +1,444 @@ +// Copyright 2023 Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +//! Module containing versions of the standard library's [`Read`] and [`Write`] traits compatible +//! with volatile memory accesses. + +use crate::bitmap::BitmapSlice; +use crate::volatile_memory::copy_slice_impl::{copy_from_volatile_slice, copy_to_volatile_slice}; +use crate::{VolatileMemoryError, VolatileSlice}; +use std::io::ErrorKind; +use std::os::fd::AsRawFd; + +/// A version of the standard library's [`Read`] trait that operates on volatile memory instead of +/// slices +/// +/// This trait is needed as rust slices (`&[u8]` and `&mut [u8]`) cannot be used when operating on +/// guest memory [1]. +/// +/// [1]: https://github.com/rust-vmm/vm-memory/pull/217 +pub trait ReadVolatile { + /// Tries to read some bytes into the given [`VolatileSlice`] buffer, returning how many bytes + /// were read. + /// + /// The behavior of implementations should be identical to [`Read::read`] + fn read_volatile( + &mut self, + buf: &mut VolatileSlice, + ) -> Result; + + /// Tries to fill the given [`VolatileSlice`] buffer by reading from `self` returning an error + /// if insufficient bytes could be read. + /// + /// The default implementation is identical to that of [`Read::read_exact`] + fn read_exact_volatile( + &mut self, + buf: &mut VolatileSlice, + ) -> Result<(), VolatileMemoryError> { + // Implementation based on https://github.com/rust-lang/rust/blob/7e7483d26e3cec7a44ef00cf7ae6c9c8c918bec6/library/std/src/io/mod.rs#L465 + + let mut partial_buf = buf.offset(0)?; + + while !partial_buf.is_empty() { + match self.read_volatile(&mut partial_buf) { + Err(VolatileMemoryError::IOError(err)) if err.kind() == ErrorKind::Interrupted => { + continue + } + Ok(0) => { + return Err(VolatileMemoryError::IOError(std::io::Error::new( + ErrorKind::UnexpectedEof, + "failed to fill whole buffer", + ))) + } + Ok(bytes_read) => partial_buf = partial_buf.offset(bytes_read)?, + Err(err) => return Err(err), + } + } + + Ok(()) + } +} + +/// A version of the standard library's [`Write`] trait that operates on volatile memory instead of +/// slices +/// +/// This trait is needed as rust slices (`&[u8]` and `&mut [u8]`) cannot be used when operating on +/// guest memory [1]. +/// +/// [1]: https://github.com/rust-vmm/vm-memory/pull/217 +pub trait WriteVolatile { + /// Tries to write some bytes from the given [`VolatileSlice`] buffer, returning how many bytes + /// were written. + /// + /// The behavior of implementations should be identical to [`Write::write`] + fn write_volatile( + &mut self, + buf: &VolatileSlice, + ) -> Result; + + /// Tries write the entire content of the given [`VolatileSlice`] buffer to `self` returning an + /// error if not all bytes could be written. + /// + /// The default implementation is identical to that of [`Write::write_all`] + fn write_all_volatile( + &mut self, + buf: &VolatileSlice, + ) -> Result<(), VolatileMemoryError> { + // Based on https://github.com/rust-lang/rust/blob/7e7483d26e3cec7a44ef00cf7ae6c9c8c918bec6/library/std/src/io/mod.rs#L1570 + + let mut partial_buf = buf.offset(0)?; + + while !partial_buf.is_empty() { + match self.write_volatile(&partial_buf) { + Err(VolatileMemoryError::IOError(err)) if err.kind() == ErrorKind::Interrupted => { + continue + } + Ok(0) => { + return Err(VolatileMemoryError::IOError(std::io::Error::new( + ErrorKind::WriteZero, + "failed to write whole buffer", + ))) + } + Ok(bytes_written) => partial_buf = partial_buf.offset(bytes_written)?, + Err(err) => return Err(err), + } + } + + Ok(()) + } +} + +// We explicitly implement our traits for [`std::fs::File`] and [`std::os::unix::net::UnixStream`] +// instead of providing blanket implementation for [`AsRawFd`] due to trait coherence limitations: A +// blanket implementation would prevent us from providing implementations for `&mut [u8]` below, as +// "an upstream crate could implement AsRawFd for &mut [u8]`. + +macro_rules! impl_read_write_volatile_for_raw_fd { + ($raw_fd_ty:ty) => { + impl ReadVolatile for $raw_fd_ty { + fn read_volatile( + &mut self, + buf: &mut VolatileSlice, + ) -> Result { + read_volatile_raw_fd(self, buf) + } + } + + impl WriteVolatile for $raw_fd_ty { + fn write_volatile( + &mut self, + buf: &VolatileSlice, + ) -> Result { + write_volatile_raw_fd(self, buf) + } + } + }; +} + +impl_read_write_volatile_for_raw_fd!(std::fs::File); +impl_read_write_volatile_for_raw_fd!(std::os::unix::net::UnixStream); +impl_read_write_volatile_for_raw_fd!(std::os::fd::OwnedFd); +impl_read_write_volatile_for_raw_fd!(std::os::fd::BorrowedFd<'_>); + +/// Tries to do a single `read` syscall on the provided file descriptor, storing the data raed in +/// the given [`VolatileSlice`]. +/// +/// Returns the numbers of bytes read. +fn read_volatile_raw_fd( + raw_fd: &mut Fd, + buf: &mut VolatileSlice, +) -> Result { + let fd = raw_fd.as_raw_fd(); + let guard = buf.ptr_guard_mut(); + + let dst = guard.as_ptr().cast::(); + + // SAFETY: We got a valid file descriptor from `AsRawFd`. The memory pointed to by `dst` is + // valid for writes of length `buf.len() by the invariants upheld by the constructor + // of `VolatileSlice`. + let bytes_read = unsafe { libc::read(fd, dst, buf.len()) }; + + if bytes_read < 0 { + // We don't know if a partial read might have happened, so mark everything as dirty + buf.bitmap().mark_dirty(0, buf.len()); + + Err(VolatileMemoryError::IOError(std::io::Error::last_os_error())) + } else { + let bytes_read = bytes_read.try_into().unwrap(); + buf.bitmap().mark_dirty(0, bytes_read); + Ok(bytes_read) + } +} + +/// Tries to do a single `write` syscall on the provided file descriptor, attempting to write the +/// data stored in the given [`VolatileSlice`]. +/// +/// Returns the numbers of bytes written. +fn write_volatile_raw_fd( + raw_fd: &mut Fd, + buf: &VolatileSlice, +) -> Result { + let fd = raw_fd.as_raw_fd(); + let guard = buf.ptr_guard(); + + let src = guard.as_ptr().cast::(); + + // SAFETY: We got a valid file descriptor from `AsRawFd`. The memory pointed to by `src` is + // valid for reads of length `buf.len() by the invariants upheld by the constructor + // of `VolatileSlice`. + let bytes_written = unsafe { libc::write(fd, src, buf.len()) }; + + if bytes_written < 0 { + Err(VolatileMemoryError::IOError(std::io::Error::last_os_error())) + } else { + Ok(bytes_written.try_into().unwrap()) + } +} + +impl WriteVolatile for &mut [u8] { + fn write_volatile( + &mut self, + buf: &VolatileSlice, + ) -> Result { + let total = buf.len().min(self.len()); + let src = buf.subslice(0, total)?; + + // SAFETY: + // We check above that `src` is contiguously allocated memory of length `total <= self.len())`. + // Furthermore, both src and dst of the call to + // copy_from_volatile_slice are valid for reads and writes respectively of length `total` + // since total is the minimum of lengths of the memory areas pointed to. The areas do not + // overlap, since `dst` is inside guest memory, and buf is a slice (no slices to guest + // memory are possible without violating rust's aliasing rules). + let written = unsafe { copy_from_volatile_slice(self.as_mut_ptr(), &src, total) }; + + // Advance the slice, just like the stdlib: https://doc.rust-lang.org/src/std/io/impls.rs.html#335 + *self = std::mem::take(self).split_at_mut(written).1; + + Ok(written) + } + + fn write_all_volatile( + &mut self, + buf: &VolatileSlice, + ) -> Result<(), VolatileMemoryError> { + // Based on https://github.com/rust-lang/rust/blob/f7b831ac8a897273f78b9f47165cf8e54066ce4b/library/std/src/io/impls.rs#L376-L382 + if self.write_volatile(buf)? == buf.len() { + Ok(()) + } else { + Err(VolatileMemoryError::IOError(std::io::Error::new( + ErrorKind::WriteZero, + "failed to write whole buffer", + ))) + } + } +} + +impl ReadVolatile for &[u8] { + fn read_volatile( + &mut self, + buf: &mut VolatileSlice, + ) -> Result { + let total = buf.len().min(self.len()); + let dst = buf.subslice(0, total)?; + + // SAFETY: + // We check above that `dst` is contiguously allocated memory of length `total <= self.len())`. + // Furthermore, both src and dst of the call to copy_to_volatile_slice are valid for reads + // and writes respectively of length `total` since total is the minimum of lengths of the + // memory areas pointed to. The areas do not overlap, since `dst` is inside guest memory, + // and buf is a slice (no slices to guest memory are possible without violating rust's aliasing rules). + let read = unsafe { copy_to_volatile_slice(&dst, self.as_ptr(), total) }; + + // Advance the slice, just like the stdlib: https://doc.rust-lang.org/src/std/io/impls.rs.html#232-310 + *self = self.split_at(read).1; + + Ok(read) + } + + fn read_exact_volatile( + &mut self, + buf: &mut VolatileSlice, + ) -> Result<(), VolatileMemoryError> { + // Based on https://github.com/rust-lang/rust/blob/f7b831ac8a897273f78b9f47165cf8e54066ce4b/library/std/src/io/impls.rs#L282-L302 + if buf.len() > self.len() { + return Err(VolatileMemoryError::IOError(std::io::Error::new( + ErrorKind::UnexpectedEof, + "failed to fill whole buffer", + ))); + } + + self.read_volatile(buf).map(|_| ()) + } +} + +#[cfg(test)] +mod tests { + use crate::io::{ReadVolatile, WriteVolatile}; + use crate::{VolatileMemoryError, VolatileSlice}; + use std::io::{ErrorKind, Read, Seek, Write}; + use vmm_sys_util::tempfile::TempFile; + + // ---- Test ReadVolatile for &[u8] ---- + fn read_4_bytes_to_5_byte_memory(source: Vec, expected_output: [u8; 5]) { + // Test read_volatile for &[u8] works + let mut memory = vec![0u8; 5]; + + assert_eq!( + (&source[..]) + .read_volatile(&mut VolatileSlice::from(&mut memory[..4])) + .unwrap(), + source.len().min(4) + ); + assert_eq!(&memory, &expected_output); + + // Test read_exact_volatile for &[u8] works + let mut memory = vec![0u8; 5]; + let result = (&source[..]).read_exact_volatile(&mut VolatileSlice::from(&mut memory[..4])); + + // read_exact fails if there are not enough bytes in input to completely fill + // memory[..4] + if source.len() < 4 { + match result.unwrap_err() { + VolatileMemoryError::IOError(ioe) => { + assert_eq!(ioe.kind(), ErrorKind::UnexpectedEof) + } + err => panic!("{:?}", err), + } + assert_eq!(memory, vec![0u8; 5]); + } else { + result.unwrap(); + assert_eq!(&memory, &expected_output); + } + } + + // ---- Test ReadVolatile for File ---- + fn read_4_bytes_from_file(source: Vec, expected_output: [u8; 5]) { + let mut temp_file = TempFile::new().unwrap().into_file(); + temp_file.write_all(source.as_ref()).unwrap(); + temp_file.rewind().unwrap(); + + // Test read_volatile for File works + let mut memory = vec![0u8; 5]; + + assert_eq!( + temp_file + .read_volatile(&mut VolatileSlice::from(&mut memory[..4])) + .unwrap(), + source.len().min(4) + ); + assert_eq!(&memory, &expected_output); + + temp_file.rewind().unwrap(); + + // Test read_exact_volatile for File works + let mut memory = vec![0u8; 5]; + + let read_exact_result = + temp_file.read_exact_volatile(&mut VolatileSlice::from(&mut memory[..4])); + + if source.len() < 4 { + read_exact_result.unwrap_err(); + } else { + read_exact_result.unwrap(); + } + assert_eq!(&memory, &expected_output); + } + + #[test] + fn test_read_volatile() { + let test_cases = [ + (vec![1u8, 2], [1u8, 2, 0, 0, 0]), + (vec![1, 2, 3, 4], [1, 2, 3, 4, 0]), + // ensure we don't have a buffer overrun + (vec![5, 6, 7, 8, 9], [5, 6, 7, 8, 0]), + ]; + + for (input, output) in test_cases { + read_4_bytes_to_5_byte_memory(input.clone(), output); + read_4_bytes_from_file(input, output); + } + } + + // ---- Test WriteVolatile for &mut [u8] ---- + fn write_4_bytes_to_5_byte_vec(mut source: Vec, expected_result: [u8; 5]) { + let mut memory = vec![0u8; 5]; + + // Test write_volatile for &mut [u8] works + assert_eq!( + (&mut memory[..4]) + .write_volatile(&VolatileSlice::from(source.as_mut_slice())) + .unwrap(), + source.len().min(4) + ); + assert_eq!(&memory, &expected_result); + + // Test write_all_volatile for &mut [u8] works + let mut memory = vec![0u8; 5]; + + let result = + (&mut memory[..4]).write_all_volatile(&VolatileSlice::from(source.as_mut_slice())); + + if source.len() > 4 { + match result.unwrap_err() { + VolatileMemoryError::IOError(ioe) => { + assert_eq!(ioe.kind(), ErrorKind::WriteZero) + } + err => panic!("{:?}", err), + } + // This quirky behavior of writing to the slice even in the case of failure is also + // exhibited by the stdlib + assert_eq!(&memory, &expected_result); + } else { + result.unwrap(); + assert_eq!(&memory, &expected_result); + } + } + + // ---- Test ẂriteVolatile for File works ---- + fn write_5_bytes_to_file(mut source: Vec) { + // Test write_volatile for File works + let mut temp_file = TempFile::new().unwrap().into_file(); + + temp_file + .write_volatile(&VolatileSlice::from(source.as_mut_slice())) + .unwrap(); + temp_file.rewind().unwrap(); + + let mut written = vec![0u8; source.len()]; + temp_file.read_exact(written.as_mut_slice()).unwrap(); + + assert_eq!(source, written); + // check no excess bytes were written to the file + assert_eq!(temp_file.read(&mut [0u8]).unwrap(), 0); + + // Test write_all_volatile for File works + let mut temp_file = TempFile::new().unwrap().into_file(); + + temp_file + .write_all_volatile(&VolatileSlice::from(source.as_mut_slice())) + .unwrap(); + temp_file.rewind().unwrap(); + + let mut written = vec![0u8; source.len()]; + temp_file.read_exact(written.as_mut_slice()).unwrap(); + + assert_eq!(source, written); + // check no excess bytes were written to the file + assert_eq!(temp_file.read(&mut [0u8]).unwrap(), 0); + } + + #[test] + fn test_write_volatile() { + let test_cases = [ + (vec![1u8, 2], [1u8, 2, 0, 0, 0]), + (vec![1, 2, 3, 4], [1, 2, 3, 4, 0]), + // ensure we don't have a buffer overrun + (vec![5, 6, 7, 8, 9], [5, 6, 7, 8, 0]), + ]; + + for (input, output) in test_cases { + write_4_bytes_to_5_byte_vec(input.clone(), output); + write_5_bytes_to_file(input); + } + } +} diff --git a/src/lib.rs b/src/lib.rs index b574dfa9..ca1870f8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -45,6 +45,9 @@ pub use guest_memory::{ GuestMemoryRegion, GuestUsize, MemoryRegionAddress, Result as GuestMemoryResult, }; +pub mod io; +pub use io::{ReadVolatile, WriteVolatile}; + #[cfg(all(feature = "backend-mmap", not(feature = "xen"), unix))] mod mmap_unix; diff --git a/src/mmap.rs b/src/mmap.rs index 0a442e69..e0f7acfa 100644 --- a/src/mmap.rs +++ b/src/mmap.rs @@ -273,6 +273,7 @@ impl Bytes for GuestRegionMmap { F: Read, { let maddr = addr.raw_value() as usize; + #[allow(deprecated)] // function itself is deprecated self.as_volatile_slice() .unwrap() .read_from::(maddr, src, count) @@ -318,6 +319,7 @@ impl Bytes for GuestRegionMmap { F: Read, { let maddr = addr.raw_value() as usize; + #[allow(deprecated)] // function itself is deprecated self.as_volatile_slice() .unwrap() .read_exact_from::(maddr, src, count) @@ -363,6 +365,7 @@ impl Bytes for GuestRegionMmap { F: Write, { let maddr = addr.raw_value() as usize; + #[allow(deprecated)] // function itself is deprecated self.as_volatile_slice() .unwrap() .write_to::(maddr, dst, count) @@ -408,6 +411,7 @@ impl Bytes for GuestRegionMmap { F: Write, { let maddr = addr.raw_value() as usize; + #[allow(deprecated)] // function itself is deprecated self.as_volatile_slice() .unwrap() .write_all_to::(maddr, dst, count) @@ -1177,7 +1181,7 @@ mod tests { File::open(Path::new("c:\\Windows\\system32\\ntoskrnl.exe")).unwrap() }; gm.write_obj(!0u32, addr).unwrap(); - gm.read_exact_from(addr, &mut file, mem::size_of::()) + gm.read_exact_volatile_from(addr, &mut file, mem::size_of::()) .unwrap(); let value: u32 = gm.read_obj(addr).unwrap(); if cfg!(unix) { @@ -1186,8 +1190,8 @@ mod tests { assert_eq!(value, 0x0090_5a4d); } - let mut sink = Vec::new(); - gm.write_all_to(addr, &mut sink, mem::size_of::()) + let mut sink = vec![0; mem::size_of::()]; + gm.write_all_volatile_to(addr, &mut sink.as_mut_slice(), mem::size_of::()) .unwrap(); if cfg!(unix) { assert_eq!(sink, vec![0; mem::size_of::()]); diff --git a/src/volatile_memory.rs b/src/volatile_memory.rs index 76e41bb7..5ad0ab27 100644 --- a/src/volatile_memory.rs +++ b/src/volatile_memory.rs @@ -43,6 +43,7 @@ use crate::mmap_xen::{MmapXen as MmapInfo, MmapXenSlice}; #[cfg(not(feature = "xen"))] type MmapInfo = std::marker::PhantomData<()>; +use crate::io::{ReadVolatile, WriteVolatile}; use copy_slice_impl::{copy_from_volatile_slice, copy_to_volatile_slice}; /// `VolatileMemory` related errors. @@ -682,7 +683,7 @@ impl Bytes for VolatileSlice<'_, B> { /// assert!(res.is_ok()); /// assert_eq!(res.unwrap(), 4); /// ``` - fn write(&self, buf: &[u8], addr: usize) -> Result { + fn write(&self, mut buf: &[u8], addr: usize) -> Result { if buf.is_empty() { return Ok(0); } @@ -691,18 +692,10 @@ impl Bytes for VolatileSlice<'_, B> { return Err(Error::OutOfBounds { addr }); } - let total = buf.len().min(self.len() - addr); - let dst = self.subslice(addr, total)?; - - // SAFETY: - // We check above that `addr` is a valid offset within this volatile slice, and by - // the invariants of `VolatileSlice::new`, this volatile slice points to contiguous - // memory of length self.len(). Furthermore, both src and dst of the call to - // copy_to_volatile_slice are valid for reads and writes respectively of length `total` - // since total is the minimum of lengths of the memory areas pointed to. The areas do not - // overlap, since `dst` is inside guest memory, and buf is a slice (no slices to guest - // memory are possible without violating rust's aliasing rules). - Ok(unsafe { copy_to_volatile_slice(&dst, buf.as_ptr(), total) }) + // NOTE: the duality of read <-> write here is correct. This is because we translate a call + // "volatile_slice.write(buf)" (e.g. "write to volatile_slice from buf") into + // "buf.read_volatile(volatile_slice)" (e.g. read from buf into volatile_slice) + buf.read_volatile(&mut self.offset(addr)?) } /// # Examples @@ -719,7 +712,7 @@ impl Bytes for VolatileSlice<'_, B> { /// assert!(res.is_ok()); /// assert_eq!(res.unwrap(), 14); /// ``` - fn read(&self, buf: &mut [u8], addr: usize) -> Result { + fn read(&self, mut buf: &mut [u8], addr: usize) -> Result { if buf.is_empty() { return Ok(0); } @@ -728,18 +721,11 @@ impl Bytes for VolatileSlice<'_, B> { return Err(Error::OutOfBounds { addr }); } - let total = buf.len().min(self.len() - addr); - let src = self.subslice(addr, total)?; - - // SAFETY: - // We check above that `addr` is a valid offset within this volatile slice, and by - // the invariants of `VolatileSlice::new`, this volatile slice points to contiguous - // memory of length self.len(). Furthermore, both src and dst of the call to - // copy_from_volatile_slice are valid for reads and writes respectively of length `total` - // since total is the minimum of lengths of the memory areas pointed to. The areas do not - // overlap, since `dst` is inside guest memory, and buf is a slice (no slices to guest - // memory are possible without violating rust's aliasing rules). - unsafe { Ok(copy_from_volatile_slice(buf.as_mut_ptr(), &src, total)) } + // NOTE: The duality of read <-> write here is correct. This is because we translate a call + // volatile_slice.read(buf) (e.g. read from volatile_slice into buf) into + // "buf.write_volatile(volatile_slice)" (e.g. write into buf from volatile_slice) + // Both express data transfer from volatile_slice to buf. + buf.write_volatile(&self.offset(addr)?) } /// # Examples @@ -1512,7 +1498,7 @@ fn alignment(addr: usize) -> usize { addr & (!addr + 1) } -mod copy_slice_impl { +pub(crate) mod copy_slice_impl { use super::*; // SAFETY: Has the same safety requirements as `read_volatile` + `write_volatile`, namely: @@ -1610,7 +1596,7 @@ mod copy_slice_impl { /// /// SAFETY: `slice` and `dst` must be point to a contiguously allocated memory region of at /// least length `total`. The regions must not overlap. - pub(super) unsafe fn copy_from_volatile_slice( + pub(crate) unsafe fn copy_from_volatile_slice( dst: *mut u8, slice: &VolatileSlice<'_, B>, total: usize, @@ -1625,7 +1611,7 @@ mod copy_slice_impl { /// /// SAFETY: `slice` and `src` must be point to a contiguously allocated memory region of at /// least length `total`. The regions must not overlap. - pub(super) unsafe fn copy_to_volatile_slice( + pub(crate) unsafe fn copy_to_volatile_slice( slice: &VolatileSlice<'_, B>, src: *const u8, total: usize, @@ -1647,7 +1633,6 @@ mod tests { use std::alloc::Layout; use std::fs::File; - use std::io::Cursor; use std::mem::size_of_val; use std::path::Path; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -2132,16 +2117,15 @@ mod tests { } else { File::open(Path::new("c:\\Windows\\system32\\ntoskrnl.exe")).unwrap() }; - assert!(s.read_exact_from(2, &mut file, size_of::()).is_err()); - assert!(s - .read_exact_from(core::usize::MAX, &mut file, size_of::()) - .is_err()); - assert!(s.read_exact_from(1, &mut file, size_of::()).is_ok()); + assert!(file + .read_exact_volatile(&mut s.get_slice(1, size_of::()).unwrap()) + .is_ok()); let mut f = TempFile::new().unwrap().into_file(); - assert!(s.read_exact_from(1, &mut f, size_of::()).is_err()); - format!("{:?}", s.read_exact_from(1, &mut f, size_of::())); + assert!(f + .read_exact_volatile(&mut s.get_slice(1, size_of::()).unwrap()) + .is_err()); let value = s.read_obj::(1).unwrap(); if cfg!(unix) { @@ -2150,13 +2134,12 @@ mod tests { assert_eq!(value, 0x0090_5a4d); } - let mut sink = Vec::new(); - assert!(s.write_all_to(1, &mut sink, size_of::()).is_ok()); - assert!(s.write_all_to(2, &mut sink, size_of::()).is_err()); - assert!(s - .write_all_to(core::usize::MAX, &mut sink, size_of::()) - .is_err()); - format!("{:?}", s.write_all_to(2, &mut sink, size_of::())); + let mut sink = vec![0; size_of::()]; + assert!(sink + .as_mut_slice() + .write_all_volatile(&s.get_slice(1, size_of::()).unwrap()) + .is_ok()); + if cfg!(unix) { assert_eq!(sink, vec![0; size_of::()]); } else { @@ -2190,16 +2173,15 @@ mod tests { } unsafe impl ByteValued for BytesToRead {} let cursor_size = 20; - let mut image = Cursor::new(vec![1u8; cursor_size]); + let image = vec![1u8; cursor_size]; - // Trying to read more bytes than we have available in the cursor should - // make the read_from function return maximum cursor size (i.e. 20). + // Trying to read more bytes than we have space for in image + // make the read_from function return maximum vec size (i.e. 20). let mut bytes_to_read = BytesToRead::default(); - let size_of_bytes = size_of_val(&bytes_to_read); assert_eq!( - bytes_to_read - .as_bytes() - .read_from(0, &mut image, size_of_bytes) + image + .as_slice() + .read_volatile(&mut bytes_to_read.as_bytes()) .unwrap(), cursor_size );