clippy: Resolve cast_ptr_alignment

This CL fixes four cases of what I believe are undefined behavior:

  - In vhost where the original code allocates a Vec<u8> with 1-byte
    alignment and casts the Vec's data pointer to a &mut vhost_memory
    which is required to be 8-byte aligned. Underaligned references of
    type &T or &mut T are always undefined behavior in Rust.

  - Same pattern in x86_64.

  - Same pattern in plugin::vcpu.

  - Code in crosvm_plugin that dereferences a potentially underaligned
    pointer. This is always undefined behavior in Rust.

TEST=bin/clippy
TEST=cargo test sys_util

Change-Id: I926f17b1fe022a798f69d738f9990d548f40c59b
Reviewed-on: https://chromium-review.googlesource.com/1566736
Commit-Ready: David Tolnay <dtolnay@chromium.org>
Tested-by: David Tolnay <dtolnay@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>
Reviewed-by: David Tolnay <dtolnay@chromium.org>

Cargo.lock

@@ -83,6 +83,7 @@ version = "0.1.0"
 dependencies = [
  "aarch64 0.1.0",
  "arch 0.1.0",
+ "assertions 0.1.0",
  "audio_streams 0.1.0",
  "bit_field 0.1.0",
  "byteorder 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -521,6 +522,7 @@ dependencies = [
 name = "vhost"
 version = "0.1.0"
 dependencies = [
+ "assertions 0.1.0",
  "libc 0.2.44 (registry+https://github.com/rust-lang/crates.io-index)",
  "net_util 0.1.0",
  "sys_util 0.1.0",
@@ -561,6 +563,7 @@ name = "x86_64"
 version = "0.1.0"
 dependencies = [
  "arch 0.1.0",
+ "assertions 0.1.0",
  "byteorder 1.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "cc 1.0.25 (registry+https://github.com/rust-lang/crates.io-index)",
  "data_model 0.1.0",

Cargo.toml

@@ -31,6 +31,7 @@ wl-dmabuf = ["devices/wl-dmabuf", "gpu_buffer", "resources/wl-dmabuf"]
 
 [dependencies]
 arch = { path = "arch" }
+assertions = { path = "assertions" }
 audio_streams = "*"
 bit_field = { path = "bit_field" }
 byteorder = "=1.1.0"

bin/clippy

@@ -18,9 +18,6 @@ SUPPRESS=(
     range_plus_one
     unit_arg
 
-    # To be resolved or suppressed locally.
-    cast_ptr_alignment
-
     # We don't care about these lints. Okay to remain suppressed globally.
     blacklisted_name
     cast_lossless

crosvm_plugin/src/lib.rs

@@ -21,7 +21,7 @@ use std::mem::{size_of, swap};
 use std::os::raw::{c_int, c_void};
 use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd};
 use std::os::unix::net::UnixDatagram;
-use std::ptr::null_mut;
+use std::ptr::{self, null_mut};
 use std::result;
 use std::slice;
 use std::slice::{from_raw_parts, from_raw_parts_mut};
@@ -682,6 +682,13 @@ pub struct crosvm_io_event {
 }
 
 impl crosvm_io_event {
+    // Clippy: we use ptr::read_unaligned to read from pointers that may be
+    // underaligned. Dereferencing such a pointer is always undefined behavior
+    // in Rust.
+    //
+    // Lint can be unsuppressed once Clippy recognizes this pattern as correct.
+    // https://github.com/rust-lang/rust-clippy/issues/2881
+    #[allow(clippy::cast_ptr_alignment)]
     unsafe fn create(
         crosvm: &mut crosvm,
         space: u32,
@@ -691,10 +698,10 @@ impl crosvm_io_event {
     ) -> result::Result<crosvm_io_event, c_int> {
         let datamatch = match length {
             0 => 0,
-            1 => *(datamatch as *const u8) as u64,
-            2 => *(datamatch as *const u16) as u64,
-            4 => *(datamatch as *const u32) as u64,
-            8 => *(datamatch as *const u64) as u64,
+            1 => ptr::read_unaligned(datamatch as *const u8) as u64,
+            2 => ptr::read_unaligned(datamatch as *const u16) as u64,
+            4 => ptr::read_unaligned(datamatch as *const u32) as u64,
+            8 => ptr::read_unaligned(datamatch as *const u64),
             _ => return Err(EINVAL),
         };
         Self::safe_create(crosvm, space, addr, length, datamatch)

src/plugin/vcpu.rs

@@ -2,11 +2,12 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+use std::alloc::Layout;
 use std::cell::{Cell, RefCell};
 use std::cmp::min;
 use std::cmp::{self, Ord, PartialEq, PartialOrd};
 use std::collections::btree_set::BTreeSet;
-use std::mem::size_of;
+use std::mem;
 use std::os::unix::net::UnixDatagram;
 use std::sync::{Arc, RwLock};
 
@@ -15,6 +16,7 @@ use libc::{EINVAL, ENOENT, ENOTTY, EPERM, EPIPE, EPROTO};
 use protobuf;
 use protobuf::Message;
 
+use assertions::const_assert;
 use data_model::DataInit;
 use kvm::{CpuId, Vcpu};
 use kvm_sys::{
@@ -23,7 +25,7 @@ use kvm_sys::{
 };
 use protos::plugin::*;
 use sync::Mutex;
-use sys_util::error;
+use sys_util::{error, LayoutAllocation};
 
 use super::*;
 
@@ -451,16 +453,23 @@ impl PluginVcpu {
                 Err(e) => Err(e),
             }
         } else if request.has_set_msrs() {
+            const SIZE_OF_MSRS: usize = mem::size_of::<kvm_msrs>();
+            const SIZE_OF_ENTRY: usize = mem::size_of::<kvm_msr_entry>();
+            const ALIGN_OF_MSRS: usize = mem::align_of::<kvm_msrs>();
+            const ALIGN_OF_ENTRY: usize = mem::align_of::<kvm_msr_entry>();
+            const_assert!(ALIGN_OF_MSRS >= ALIGN_OF_ENTRY);
+
             response.mut_set_msrs();
             let request_entries = &request.get_set_msrs().entries;
-            let vec_size_bytes =
-                size_of::<kvm_msrs>() + (request_entries.len() * size_of::<kvm_msr_entry>());
-            let vec: Vec<u8> = vec![0; vec_size_bytes];
-            let kvm_msrs: &mut kvm_msrs = unsafe {
-                // Converting the vector's memory to a struct is unsafe.  Carefully using the read-
-                // only vector to size and set the members ensures no out-of-bounds erros below.
-                &mut *(vec.as_ptr() as *mut kvm_msrs)
-            };
+            let size = SIZE_OF_MSRS + request_entries.len() * SIZE_OF_ENTRY;
+            let layout = Layout::from_size_align(size, ALIGN_OF_MSRS).expect("impossible layout");
+            let mut allocation = LayoutAllocation::zeroed(layout);
+
+            // Safe to obtain an exclusive reference because there are no other
+            // references to the allocation yet and all-zero is a valid bit
+            // pattern.
+            let kvm_msrs = unsafe { allocation.as_mut::<kvm_msrs>() };
+
             unsafe {
                 // Mapping the unsized array to a slice is unsafe becase the length isn't known.
                 // Providing the length used to create the struct guarantees the entire slice is

sys_util/src/alloc.rs

@@ -0,0 +1,119 @@
+use std::alloc::{alloc, alloc_zeroed, dealloc, Layout};
+
+/// A contiguous memory allocation with a specified size and alignment, with a
+/// Drop impl to perform the deallocation.
+///
+/// Conceptually this is like a Box<[u8]> but for which we can select a minimum
+/// required alignment at the time of allocation.
+///
+/// # Example
+///
+/// ```
+/// use std::alloc::Layout;
+/// use std::mem;
+/// use sys_util::LayoutAllocation;
+///
+/// #[repr(C)]
+/// struct Header {
+///     q: usize,
+///     entries: [Entry; 0], // flexible array member
+/// }
+///
+/// #[repr(C)]
+/// struct Entry {
+///     e: usize,
+/// }
+///
+/// fn demo(num_entries: usize) {
+///     let size = mem::size_of::<Header>() + num_entries * mem::size_of::<Entry>();
+///     let layout = Layout::from_size_align(size, mem::align_of::<Header>()).unwrap();
+///     let mut allocation = LayoutAllocation::zeroed(layout);
+///
+///     // Safe to obtain an exclusive reference because there are no other
+///     // references to the allocation yet and all-zero is a valid bit pattern for
+///     // our header.
+///     let header = unsafe { allocation.as_mut::<Header>() };
+/// }
+/// ```
+pub struct LayoutAllocation {
+    ptr: *mut u8,
+    layout: Layout,
+}
+
+impl LayoutAllocation {
+    /// Allocates memory with the specified size and alignment. The content is
+    /// not initialized.
+    ///
+    /// Uninitialized data is not safe to read. Further, it is not safe to
+    /// obtain a reference to data potentially holding a bit pattern
+    /// incompatible with its type, for example an uninitialized bool or enum.
+    pub fn uninitialized(layout: Layout) -> Self {
+        let ptr = if layout.size() > 0 {
+            unsafe {
+                // Safe as long as we guarantee layout.size() > 0.
+                alloc(layout)
+            }
+        } else {
+            layout.align() as *mut u8
+        };
+        LayoutAllocation { ptr, layout }
+    }
+
+    /// Allocates memory with the specified size and alignment and initializes
+    /// the content to all zero-bytes.
+    ///
+    /// Note that zeroing the memory does not necessarily make it safe to obtain
+    /// a reference to the allocation. Depending on the intended type T,
+    /// all-zero may or may not be a legal bit pattern for that type. For
+    /// example obtaining a reference would immediately be undefined behavior if
+    /// one of the fields has type NonZeroUsize.
+    pub fn zeroed(layout: Layout) -> Self {
+        let ptr = if layout.size() > 0 {
+            unsafe {
+                // Safe as long as we guarantee layout.size() > 0.
+                alloc_zeroed(layout)
+            }
+        } else {
+            layout.align() as *mut u8
+        };
+        LayoutAllocation { ptr, layout }
+    }
+
+    /// Returns a raw pointer to the allocated data.
+    pub fn as_ptr<T>(&self) -> *mut T {
+        self.ptr as *mut T
+    }
+
+    /// Returns a shared reference to the allocated data.
+    ///
+    /// # Safety
+    ///
+    /// Caller is responsible for ensuring that the data behind this pointer has
+    /// been initialized as much as necessary and that there are no already
+    /// existing mutable references to any part of the data.
+    pub unsafe fn as_ref<T>(&self) -> &T {
+        &*self.as_ptr()
+    }
+
+    /// Returns an exclusive reference to the allocated data.
+    ///
+    /// # Safety
+    ///
+    /// Caller is responsible for ensuring that the data behind this pointer has
+    /// been initialized as much as necessary and that there are no already
+    /// existing references to any part of the data.
+    pub unsafe fn as_mut<T>(&mut self) -> &mut T {
+        &mut *self.as_ptr()
+    }
+}
+
+impl Drop for LayoutAllocation {
+    fn drop(&mut self) {
+        if self.layout.size() > 0 {
+            unsafe {
+                // Safe as long as we guarantee layout.size() > 0.
+                dealloc(self.ptr, self.layout);
+            }
+        }
+    }
+}

sys_util/src/lib.rs

@@ -5,6 +5,7 @@
 //! Small system utility modules for usage by other modules.
 
 pub mod affinity;
+mod alloc;
 #[macro_use]
 pub mod handle_eintr;
 #[macro_use]
@@ -38,6 +39,7 @@ mod timerfd;
 mod write_zeroes;
 
 pub use crate::affinity::*;
+pub use crate::alloc::LayoutAllocation;
 pub use crate::capabilities::drop_capabilities;
 pub use crate::clock::{Clock, FakeClock};
 use crate::errno::errno_result;

vhost/Cargo.toml

@@ -5,6 +5,7 @@ authors = ["The Chromium OS Authors"]
 edition = "2018"
 
 [dependencies]
+assertions = { path = "../assertions" }
 libc = "*"
 net_util = { path = "../net_util" }
 sys_util = { path = "../sys_util" }

vhost/src/lib.rs

@@ -9,14 +9,16 @@ pub use crate::net::Net;
 pub use crate::net::NetT;
 pub use crate::vsock::Vsock;
 
+use std::alloc::Layout;
 use std::fmt::{self, Display};
 use std::io::Error as IoError;
 use std::mem;
 use std::os::unix::io::AsRawFd;
 use std::ptr::null;
 
+use assertions::const_assert;
 use sys_util::{ioctl, ioctl_with_mut_ref, ioctl_with_ptr, ioctl_with_ref};
-use sys_util::{EventFd, GuestAddress, GuestMemory, GuestMemoryError};
+use sys_util::{EventFd, GuestAddress, GuestMemory, GuestMemoryError, LayoutAllocation};
 
 #[derive(Debug)]
 pub enum Error {
@@ -108,14 +110,21 @@ pub trait Vhost: AsRawFd + std::marker::Sized {
 
     /// Set the guest memory mappings for vhost to use.
     fn set_mem_table(&self) -> Result<()> {
+        const SIZE_OF_MEMORY: usize = mem::size_of::<virtio_sys::vhost_memory>();
+        const SIZE_OF_REGION: usize = mem::size_of::<virtio_sys::vhost_memory_region>();
+        const ALIGN_OF_MEMORY: usize = mem::align_of::<virtio_sys::vhost_memory>();
+        const ALIGN_OF_REGION: usize = mem::align_of::<virtio_sys::vhost_memory_region>();
+        const_assert!(ALIGN_OF_MEMORY >= ALIGN_OF_REGION);
+
         let num_regions = self.mem().num_regions() as usize;
-        let vec_size_bytes = mem::size_of::<virtio_sys::vhost_memory>()
-            + (num_regions * mem::size_of::<virtio_sys::vhost_memory_region>());
-        let mut bytes: Vec<u8> = vec![0; vec_size_bytes];
-        // Convert bytes pointer to a vhost_memory mut ref. The vector has been
-        // sized correctly to ensure it can hold vhost_memory and N regions.
-        let vhost_memory: &mut virtio_sys::vhost_memory =
-            unsafe { &mut *(bytes.as_mut_ptr() as *mut virtio_sys::vhost_memory) };
+        let size = SIZE_OF_MEMORY + num_regions * SIZE_OF_REGION;
+        let layout = Layout::from_size_align(size, ALIGN_OF_MEMORY).expect("impossible layout");
+        let mut allocation = LayoutAllocation::zeroed(layout);
+
+        // Safe to obtain an exclusive reference because there are no other
+        // references to the allocation yet and all-zero is a valid bit pattern.
+        let vhost_memory = unsafe { allocation.as_mut::<virtio_sys::vhost_memory>() };
+
         vhost_memory.nregions = num_regions as u32;
         // regions is a zero-length array, so taking a mut slice requires that
         // we correctly specify the size to match the amount of backing memory.
@@ -136,12 +145,14 @@ pub trait Vhost: AsRawFd + std::marker::Sized {
         // This ioctl is called with a pointer that is valid for the lifetime
         // of this function. The kernel will make its own copy of the memory
         // tables. As always, check the return value.
-        let ret =
-            unsafe { ioctl_with_ptr(self, virtio_sys::VHOST_SET_MEM_TABLE(), bytes.as_ptr()) };
+        let ret = unsafe { ioctl_with_ptr(self, virtio_sys::VHOST_SET_MEM_TABLE(), vhost_memory) };
         if ret < 0 {
             return ioctl_result();
         }
+
         Ok(())
+
+        // vhost_memory allocation is deallocated.
     }
 
     /// Set the number of descriptors in the vring.

x86_64/Cargo.toml

@@ -7,6 +7,7 @@ build = "build.rs"
 
 [dependencies]
 arch = { path = "../arch" }
+assertions = { path = "../assertions" }
 byteorder = "*"
 data_model = { path = "../data_model" }
 devices = { path = "../devices" }

x86_64/src/regs.rs

@@ -2,17 +2,18 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+use std::alloc::Layout;
 use std::fmt::{self, Display};
 use std::{mem, result};
 
+use assertions::const_assert;
 use kvm;
 use kvm_sys::kvm_fpu;
 use kvm_sys::kvm_msr_entry;
 use kvm_sys::kvm_msrs;
 use kvm_sys::kvm_regs;
 use kvm_sys::kvm_sregs;
-use sys_util;
-use sys_util::{GuestAddress, GuestMemory};
+use sys_util::{self, GuestAddress, GuestMemory, LayoutAllocation};
 
 use crate::gdt;
 
@@ -129,15 +130,20 @@ fn create_msr_entries() -> Vec<kvm_msr_entry> {
 ///
 /// * `vcpu` - Structure for the vcpu that holds the vcpu fd.
 pub fn setup_msrs(vcpu: &kvm::Vcpu) -> Result<()> {
+    const SIZE_OF_MSRS: usize = mem::size_of::<kvm_msrs>();
+    const SIZE_OF_ENTRY: usize = mem::size_of::<kvm_msr_entry>();
+    const ALIGN_OF_MSRS: usize = mem::align_of::<kvm_msrs>();
+    const ALIGN_OF_ENTRY: usize = mem::align_of::<kvm_msr_entry>();
+    const_assert!(ALIGN_OF_MSRS >= ALIGN_OF_ENTRY);
+
     let entry_vec = create_msr_entries();
-    let vec_size_bytes =
-        mem::size_of::<kvm_msrs>() + (entry_vec.len() * mem::size_of::<kvm_msr_entry>());
-    let vec: Vec<u8> = Vec::with_capacity(vec_size_bytes);
-    let msrs: &mut kvm_msrs = unsafe {
-        // Converting the vector's memory to a struct is unsafe.  Carefully using the read-only
-        // vector to size and set the members ensures no out-of-bounds erros below.
-        &mut *(vec.as_ptr() as *mut kvm_msrs)
-    };
+    let size = SIZE_OF_MSRS + entry_vec.len() * SIZE_OF_ENTRY;
+    let layout = Layout::from_size_align(size, ALIGN_OF_MSRS).expect("impossible layout");
+    let mut allocation = LayoutAllocation::zeroed(layout);
+
+    // Safe to obtain an exclusive reference because there are no other
+    // references to the allocation yet and all-zero is a valid bit pattern.
+    let msrs = unsafe { allocation.as_mut::<kvm_msrs>() };
 
     unsafe {
         // Mapping the unsized array to a slice is unsafe becase the length isn't known.  Providing
@@ -150,6 +156,8 @@ pub fn setup_msrs(vcpu: &kvm::Vcpu) -> Result<()> {
     vcpu.set_msrs(msrs).map_err(Error::MsrIoctlFailed)?;
 
     Ok(())
+
+    // msrs allocation is deallocated.
 }
 
 /// Configure FPU registers for x86