Remove the AsSlice trait

Taking rust-style references to guest memory of a running virtual
machine might violate the aliasing rules, as the guest can modify
its memory at will. Additionally, since the `read_from` and
`write_to` functions "leak" references to guest memory to code
outside of the vm-memory crate, it is possible to trigger undefined
behavior in single-threaded, safe code, for example by creating a
`Read` implementation the reads from a volatile slice:

struct VolatileSliceReader<'a> {
    src: VolatileSlice<'a, ()>
}

impl<'a> Read for VolatileSliceReader<'a> {
    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
        Ok(self.src.copy_to(buf))
    }
}

\#[test]  // test fails under miri
fn undefined_behavior() {
    let mut data = vec![0u8; 20];
    let vslice = unsafe {VolatileSlice::new(data.as_mut_ptr(), 20)};

    let mut reader = VolatileSliceReader {src: vslice};
    vslice.read_from(0, &mut reader, 10);
}

Since this trait was marked `pub(crate)`, this is not a breaking
change.

As a consequence, the `read_from` and `write_to` functions of
`Bytes` implementations now always first copy to a buffer
allocated within the function, before transferign data from/to
the Read/Write object provided. There is no safe way to keep
the existing abstraction, as Rust does not give us a low-level
interface to Read/Write that operates with pointers.

Signed-off-by: Patrick Roy <[email protected]>

Author: roypat

src/guest_memory.rs

@@ -250,6 +250,8 @@ pub trait GuestMemoryRegion: Bytes<MemoryRegionAddress, E = Error> {
     /// # Safety
     ///
     /// Unsafe because of possible aliasing.
+    #[deprecated = "It is impossible to use this function for accessing memory of a running virtual \
+    machine without violating aliasing rules "]
     unsafe fn as_slice(&self) -> Option<&[u8]> {
         None
     }
@@ -263,6 +265,8 @@ pub trait GuestMemoryRegion: Bytes<MemoryRegionAddress, E = Error> {
     /// Unsafe because of possible aliasing. Mutable accesses performed through the
     /// returned slice are not visible to the dirty bitmap tracking functionality of
     /// the region, and must be manually recorded using the associated bitmap object.
+    #[deprecated = "It is impossible to use this function for accessing memory of a running virtual \
+    machine without violating aliasing rules "]
     unsafe fn as_mut_slice(&self) -> Option<&mut [u8]> {
         None
     }
@@ -848,38 +852,22 @@ impl<T: GuestMemory + ?Sized> Bytes<GuestAddress> for T {
         self.try_access(count, addr, |offset, len, caddr, region| -> Result<usize> {
             // Check if something bad happened before doing unsafe things.
             assert!(offset <= count);
-            // SAFETY: Safe because we are checking the offset.
-            if let Some(dst) = unsafe { region.as_mut_slice() } {
-                // This is safe cause `start` and `len` are within the `region`, and we manually
-                // record the dirty status of the written range below.
-                let start = caddr.raw_value() as usize;
-                let end = start + len;
-                let bytes_read = loop {
-                    match src.read(&mut dst[start..end]) {
-                        Ok(n) => break n,
-                        Err(ref e) if e.kind() == std::io::ErrorKind::Interrupted => continue,
-                        Err(e) => return Err(Error::IOError(e)),
-                    }
-                };
-
-                region.bitmap().mark_dirty(start, bytes_read);
-                Ok(bytes_read)
-            } else {
-                let len = std::cmp::min(len, MAX_ACCESS_CHUNK);
-                let mut buf = vec![0u8; len].into_boxed_slice();
-                loop {
-                    match src.read(&mut buf[..]) {
-                        Ok(bytes_read) => {
-                            // We don't need to update the dirty bitmap manually here because it's
-                            // expected to be handled by the logic within the `Bytes`
-                            // implementation for the region object.
-                            let bytes_written = region.write(&buf[0..bytes_read], caddr)?;
-                            assert_eq!(bytes_written, bytes_read);
-                            break Ok(bytes_read);
-                        }
-                        Err(ref e) if e.kind() == std::io::ErrorKind::Interrupted => continue,
-                        Err(e) => break Err(Error::IOError(e)),
+
+            let len = std::cmp::min(len, MAX_ACCESS_CHUNK);
+            let mut buf = vec![0u8; len].into_boxed_slice();
+
+            loop {
+                match src.read(&mut buf[..]) {
+                    Ok(bytes_read) => {
+                        // We don't need to update the dirty bitmap manually here because it's
+                        // expected to be handled by the logic within the `Bytes`
+                        // implementation for the region object.
+                        let bytes_written = region.write(&buf[0..bytes_read], caddr)?;
+                        assert_eq!(bytes_written, bytes_read);
+                        break Ok(bytes_read);
                     }
+                    Err(ref e) if e.kind() == std::io::ErrorKind::Interrupted => continue,
+                    Err(e) => break Err(Error::IOError(e)),
                 }
             }
         })
@@ -936,32 +924,15 @@ impl<T: GuestMemory + ?Sized> Bytes<GuestAddress> for T {
         self.try_access(count, addr, |offset, len, caddr, region| -> Result<usize> {
             // Check if something bad happened before doing unsafe things.
             assert!(offset <= count);
-            // SAFETY: Safe because we are checking the offset is valid.
-            if let Some(src) = unsafe { region.as_slice() } {
-                // This is safe cause `start` and `len` are within the `region`.
-                let start = caddr.raw_value() as usize;
-                let end = start + len;
-                loop {
-                    // It is safe to read from volatile memory. Accessing the guest
-                    // memory as a slice should be OK as long as nothing assumes another
-                    // thread won't change what is loaded; however, we may want to introduce
-                    // VolatileRead and VolatileWrite traits in the future.
-                    match dst.write(&src[start..end]) {
-                        Ok(n) => break Ok(n),
-                        Err(ref e) if e.kind() == std::io::ErrorKind::Interrupted => continue,
-                        Err(e) => break Err(Error::IOError(e)),
-                    }
-                }
-            } else {
-                let len = std::cmp::min(len, MAX_ACCESS_CHUNK);
-                let mut buf = vec![0u8; len].into_boxed_slice();
-                let bytes_read = region.read(&mut buf, caddr)?;
-                assert_eq!(bytes_read, len);
-                // For a non-RAM region, reading could have side effects, so we
-                // must use write_all().
-                dst.write_all(&buf).map_err(Error::IOError)?;
-                Ok(len)
-            }
+
+            let len = std::cmp::min(len, MAX_ACCESS_CHUNK);
+            let mut buf = vec![0u8; len].into_boxed_slice();
+            let bytes_read = region.read(&mut buf, caddr)?;
+            assert_eq!(bytes_read, len);
+            // For a non-RAM region, reading could have side effects, so we
+            // must use write_all().
+            dst.write_all(&buf).map_err(Error::IOError)?;
+            Ok(len)
         })
     }
 

src/mmap.rs

@@ -50,26 +50,6 @@ impl NewBitmap for () {
     fn with_len(_len: usize) -> Self {}
 }
 
-/// Trait implemented by the underlying `MmapRegion`.
-pub(crate) trait AsSlice {
-    /// Returns a slice corresponding to the data in the underlying `MmapRegion`.
-    ///
-    /// # Safety
-    ///
-    /// This is unsafe because of possible aliasing.
-    unsafe fn as_slice(&self) -> &[u8];
-
-    /// Returns a mutable slice corresponding to the data in the underlying `MmapRegion`.
-    ///
-    /// # Safety
-    ///
-    /// This is unsafe because of possible aliasing. Accesses done through the resulting slice
-    /// are not visible to dirty bitmap tracking functionality (when present), and have to be
-    /// explicitly accounted for.
-    #[allow(clippy::mut_from_ref)]
-    unsafe fn as_mut_slice(&self) -> &mut [u8];
-}
-
 /// Errors that can occur when creating a memory map.
 #[derive(Debug)]
 pub enum Error {
@@ -486,14 +466,6 @@ impl<B: Bitmap> GuestMemoryRegion for GuestRegionMmap<B> {
         self.mapping.file_offset()
     }
 
-    unsafe fn as_slice(&self) -> Option<&[u8]> {
-        Some(self.mapping.as_slice())
-    }
-
-    unsafe fn as_mut_slice(&self) -> Option<&mut [u8]> {
-        Some(self.mapping.as_mut_slice())
-    }
-
     fn get_slice(
         &self,
         offset: MemoryRegionAddress,

src/mmap_unix.rs

@@ -19,7 +19,7 @@ use std::result;
 
 use crate::bitmap::{Bitmap, BS};
 use crate::guest_memory::FileOffset;
-use crate::mmap::{check_file_offset, AsSlice, NewBitmap};
+use crate::mmap::{check_file_offset, NewBitmap};
 use crate::volatile_memory::{self, compute_offset, VolatileMemory, VolatileSlice};
 
 /// Error conditions that may arise when creating a new `MmapRegion` object.
@@ -401,21 +401,6 @@ impl<B: Bitmap> MmapRegion<B> {
     }
 }
 
-impl<B> AsSlice for MmapRegion<B> {
-    // SAFETY: This is safe because we mapped the area at addr ourselves, so this slice will not
-    // overflow. However, it is possible to alias.
-    unsafe fn as_slice(&self) -> &[u8] {
-        std::slice::from_raw_parts(self.addr, self.size)
-    }
-
-    // SAFETY: This is safe because we mapped the area at addr ourselves, so this slice will not
-    // overflow. However, it is possible to alias.
-    #[allow(clippy::mut_from_ref)]
-    unsafe fn as_mut_slice(&self) -> &mut [u8] {
-        std::slice::from_raw_parts_mut(self.addr, self.size)
-    }
-}
-
 impl<B: Bitmap> VolatileMemory for MmapRegion<B> {
     type B = B;
 

src/mmap_windows.rs

@@ -14,7 +14,7 @@ use winapi::um::errhandlingapi::GetLastError;
 
 use crate::bitmap::{Bitmap, BS};
 use crate::guest_memory::FileOffset;
-use crate::mmap::{AsSlice, NewBitmap};
+use crate::mmap::NewBitmap;
 use crate::volatile_memory::{self, compute_offset, VolatileMemory, VolatileSlice};
 
 #[allow(non_snake_case)]
@@ -190,21 +190,6 @@ impl<B: Bitmap> MmapRegion<B> {
     }
 }
 
-impl<B> AsSlice for MmapRegion<B> {
-    unsafe fn as_slice(&self) -> &[u8] {
-        // This is safe because we mapped the area at addr ourselves, so this slice will not
-        // overflow. However, it is possible to alias.
-        std::slice::from_raw_parts(self.addr, self.size)
-    }
-
-    #[allow(clippy::mut_from_ref)]
-    unsafe fn as_mut_slice(&self) -> &mut [u8] {
-        // This is safe because we mapped the area at addr ourselves, so this slice will not
-        // overflow. However, it is possible to alias.
-        std::slice::from_raw_parts_mut(self.addr, self.size)
-    }
-}
-
 impl<B: Bitmap> VolatileMemory for MmapRegion<B> {
     type B = B;