From 4b292afafcd44ca3fc34f483a8edb455a3212cb5 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Fri, 12 Apr 2019 16:57:48 -0700 Subject: [PATCH] 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 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 Tested-by: David Tolnay Tested-by: kokoro Reviewed-by: David Tolnay --- Cargo.lock | 3 + Cargo.toml | 1 + bin/clippy | 3 - crosvm_plugin/src/lib.rs | 17 ++++-- src/plugin/vcpu.rs | 29 ++++++---- sys_util/src/alloc.rs | 119 +++++++++++++++++++++++++++++++++++++++ sys_util/src/lib.rs | 2 + vhost/Cargo.toml | 1 + vhost/src/lib.rs | 31 ++++++---- x86_64/Cargo.toml | 1 + x86_64/src/regs.rs | 28 +++++---- 11 files changed, 197 insertions(+), 38 deletions(-) create mode 100644 sys_util/src/alloc.rs diff --git a/Cargo.lock b/Cargo.lock index fa8218b299..db79114a4b 100644 --- a/Cargo.lock +++ b/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", diff --git a/Cargo.toml b/Cargo.toml index dbb203c4ee..048681f348 100644 --- a/Cargo.toml +++ b/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" diff --git a/bin/clippy b/bin/clippy index 0ffb0497d9..a19b6ebd11 100755 --- a/bin/clippy +++ b/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 diff --git a/crosvm_plugin/src/lib.rs b/crosvm_plugin/src/lib.rs index eb2cf212f7..a334505aa9 100644 --- a/crosvm_plugin/src/lib.rs +++ b/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 { 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) diff --git a/src/plugin/vcpu.rs b/src/plugin/vcpu.rs index eb7128c9fb..d19a5f88ad 100644 --- a/src/plugin/vcpu.rs +++ b/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::(); + const SIZE_OF_ENTRY: usize = mem::size_of::(); + const ALIGN_OF_MSRS: usize = mem::align_of::(); + const ALIGN_OF_ENTRY: usize = mem::align_of::(); + 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::() + (request_entries.len() * size_of::()); - let vec: Vec = 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::() }; + 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 diff --git a/sys_util/src/alloc.rs b/sys_util/src/alloc.rs new file mode 100644 index 0000000000..ad73065244 --- /dev/null +++ b/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::
() + num_entries * mem::size_of::(); +/// let layout = Layout::from_size_align(size, mem::align_of::
()).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::
() }; +/// } +/// ``` +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(&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(&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(&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); + } + } + } +} diff --git a/sys_util/src/lib.rs b/sys_util/src/lib.rs index a1dffea65c..e2942af9df 100644 --- a/sys_util/src/lib.rs +++ b/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; diff --git a/vhost/Cargo.toml b/vhost/Cargo.toml index 21ebe50626..567e880a4f 100644 --- a/vhost/Cargo.toml +++ b/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" } diff --git a/vhost/src/lib.rs b/vhost/src/lib.rs index c1fb4cd8dd..04ba65565b 100644 --- a/vhost/src/lib.rs +++ b/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::(); + const SIZE_OF_REGION: usize = mem::size_of::(); + const ALIGN_OF_MEMORY: usize = mem::align_of::(); + const ALIGN_OF_REGION: usize = mem::align_of::(); + const_assert!(ALIGN_OF_MEMORY >= ALIGN_OF_REGION); + let num_regions = self.mem().num_regions() as usize; - let vec_size_bytes = mem::size_of::() - + (num_regions * mem::size_of::()); - let mut bytes: Vec = 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::() }; + 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. diff --git a/x86_64/Cargo.toml b/x86_64/Cargo.toml index 4aa97a90e7..58d688b57d 100644 --- a/x86_64/Cargo.toml +++ b/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" } diff --git a/x86_64/src/regs.rs b/x86_64/src/regs.rs index e1bd1f389b..8879a0cd7c 100644 --- a/x86_64/src/regs.rs +++ b/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 { /// /// * `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::(); + const SIZE_OF_ENTRY: usize = mem::size_of::(); + const ALIGN_OF_MSRS: usize = mem::align_of::(); + const ALIGN_OF_ENTRY: usize = mem::align_of::(); + const_assert!(ALIGN_OF_MSRS >= ALIGN_OF_ENTRY); + let entry_vec = create_msr_entries(); - let vec_size_bytes = - mem::size_of::() + (entry_vec.len() * mem::size_of::()); - let vec: Vec = 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::() }; 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