Limit types that can be read from guest memory
Not all types are safe to read from guest memory. Any type with a
reference or pointer will be initialized to random bits that don't refer
to a valid address. This can cause dangling pointer and general
unsafe behavior.
To fix this, limit types that can be read with read_obj to those that
implement the unsafe trait `DataInit`. Provide implementations of
`DataInit` for intrinsic types that are obviously safe to initialize
with random data.
Implement the needed traits for bootparam types as they are read from
the kernel image directly.
Change-Id: I1040f5bc1b2fc4c58c87d8a2ce3f618edcf6f9b1
Signed-off-by: Dylan Reid <dgreid@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/540750
Reviewed-by: Zach Reizner <zachr@chromium.org>
diff --git a/sys_util/Cargo.toml b/sys_util/Cargo.toml
index de38025..a91b145 100644
--- a/sys_util/Cargo.toml
+++ b/sys_util/Cargo.toml
@@ -4,5 +4,6 @@
authors = ["The Chromium OS Authors"]
[dependencies]
+data_model = { path = "../data_model" }
libc = "*"
diff --git a/sys_util/src/guest_memory.rs b/sys_util/src/guest_memory.rs
index 0dfb709..f7ad298 100644
--- a/sys_util/src/guest_memory.rs
+++ b/sys_util/src/guest_memory.rs
@@ -8,6 +8,7 @@
use std::result;
use std::sync::Arc;
+use data_model::DataInit;
use guest_address::GuestAddress;
use mmap::MemoryMapping;
@@ -160,7 +161,7 @@
/// # Ok(num1 + num2)
/// # }
/// ```
- pub fn read_obj_from_addr<T: Copy>(&self, guest_addr: GuestAddress) -> Result<T> {
+ pub fn read_obj_from_addr<T: DataInit>(&self, guest_addr: GuestAddress) -> Result<T> {
self.do_in_region(guest_addr, |mapping, offset| {
mapping
.read_obj(offset)
@@ -183,7 +184,7 @@
/// .map_err(|_| ())
/// # }
/// ```
- pub fn write_obj_at_addr<T>(&self, val: T, guest_addr: GuestAddress) -> Result<()> {
+ pub fn write_obj_at_addr<T: DataInit>(&self, val: T, guest_addr: GuestAddress) -> Result<()> {
self.do_in_region(guest_addr, move |mapping, offset| {
mapping
.write_obj(val, offset)
diff --git a/sys_util/src/lib.rs b/sys_util/src/lib.rs
index f4bf241..a915202 100644
--- a/sys_util/src/lib.rs
+++ b/sys_util/src/lib.rs
@@ -4,6 +4,7 @@
//! Small system utility modules for usage by other modules.
+extern crate data_model;
extern crate libc;
mod mmap;
diff --git a/sys_util/src/mmap.rs b/sys_util/src/mmap.rs
index 71b8d56..edd2a00 100644
--- a/sys_util/src/mmap.rs
+++ b/sys_util/src/mmap.rs
@@ -11,9 +11,10 @@
use std::os::unix::io::AsRawFd;
use libc;
-
use errno;
+use data_model::DataInit;
+
#[derive(Debug)]
pub enum Error {
/// Requested memory out of range.
@@ -141,7 +142,7 @@
/// let res = mem_map.write_obj(55u64, 16);
/// assert!(res.is_ok());
/// ```
- pub fn write_obj<T>(&self, val: T, offset: usize) -> Result<()> {
+ pub fn write_obj<T: DataInit>(&self, val: T, offset: usize) -> Result<()> {
unsafe {
// Guest memory can't strictly be modeled as a slice because it is
// volatile. Writing to it with what compiles down to a memcpy
@@ -170,7 +171,7 @@
/// let num: u64 = mem_map.read_obj(32).unwrap();
/// assert_eq!(55, num);
/// ```
- pub fn read_obj<T: Copy>(&self, offset: usize) -> Result<T> {
+ pub fn read_obj<T: DataInit>(&self, offset: usize) -> Result<T> {
if offset + std::mem::size_of::<T>() > self.size {
return Err(Error::InvalidAddress);
}
diff --git a/x86_64/Cargo.toml b/x86_64/Cargo.toml
index a444277..85ff1b7 100644
--- a/x86_64/Cargo.toml
+++ b/x86_64/Cargo.toml
@@ -4,6 +4,7 @@
authors = ["The Chromium OS Authors"]
[dependencies]
+data_model = { path = "../data_model" }
kvm_sys = { path = "../kvm_sys" }
kvm = { path = "../kvm" }
sys_util = { path = "../sys_util" }
diff --git a/x86_64/src/lib.rs b/x86_64/src/lib.rs
index 26de9e5..ad3ec46 100644
--- a/x86_64/src/lib.rs
+++ b/x86_64/src/lib.rs
@@ -3,6 +3,7 @@
// found in the LICENSE file.
extern crate byteorder;
+extern crate data_model;
extern crate kvm;
extern crate kvm_sys;
extern crate libc;
@@ -13,6 +14,21 @@
#[allow(non_camel_case_types)]
#[allow(non_snake_case)]
mod bootparam;
+// Bindgen didn't implement copy for boot_params because edid_info contains an array with len > 32.
+impl Copy for bootparam::edid_info {}
+impl Clone for bootparam::edid_info {
+ fn clone(&self) -> Self {
+ *self
+ }
+}
+impl Copy for bootparam::boot_params {}
+impl Clone for bootparam::boot_params {
+ fn clone(&self) -> Self {
+ *self
+ }
+}
+// boot_params is just a series of ints, it is safe to initialize it.
+unsafe impl data_model::DataInit for bootparam::boot_params {}
#[allow(dead_code)]
#[allow(non_upper_case_globals)]
@@ -22,6 +38,14 @@
#[allow(non_upper_case_globals)]
#[allow(non_camel_case_types)]
mod mpspec;
+// These mpspec types are only data, reading them from data is a safe initialization.
+unsafe impl data_model::DataInit for mpspec::mpc_bus {}
+unsafe impl data_model::DataInit for mpspec::mpc_cpu {}
+unsafe impl data_model::DataInit for mpspec::mpc_intsrc {}
+unsafe impl data_model::DataInit for mpspec::mpc_ioapic {}
+unsafe impl data_model::DataInit for mpspec::mpc_table {}
+unsafe impl data_model::DataInit for mpspec::mpc_lintsrc {}
+unsafe impl data_model::DataInit for mpspec::mpf_intel {}
mod cpuid;
mod gdt;