From 934f4abbc1602c73399836fa4a72536e02920216 Mon Sep 17 00:00:00 2001 From: Geoffrey Thomas Date: Sun, 10 Jun 2018 13:10:27 -0400 Subject: [PATCH 1/3] example-sysctl does not require liballoc --- example-sysctl/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/example-sysctl/src/lib.rs b/example-sysctl/src/lib.rs index 79390ce4..60c996fb 100644 --- a/example-sysctl/src/lib.rs +++ b/example-sysctl/src/lib.rs @@ -1,7 +1,5 @@ #![no_std] -#![feature(alloc)] -extern crate alloc; use core::sync::atomic::{AtomicBool, Ordering}; #[macro_use] From 70867989b131788378f05e4824e8827c768846f4 Mon Sep 17 00:00:00 2001 From: Geoffrey Thomas Date: Sun, 10 Jun 2018 13:11:05 -0400 Subject: [PATCH 2/3] user_ptr: Use wrapping_add instead of add to avoid UB `pointer::add` and `pointer::offset` turn into a `getelementptr inbounds`, which is UB if it does not point to a valid object or one past a valid object (i.e., it enables compiler optimizations that make that assumption). Raw pointers to userspace are not pointers to valid objects. `pointer::wrapping_add` and `pointer::wrapping_offset` turn into a `getelementptr`, which is always defined (and so they're both safe). --- src/user_ptr.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/user_ptr.rs b/src/user_ptr.rs index 937458c3..c01cb609 100644 --- a/src/user_ptr.rs +++ b/src/user_ptr.rs @@ -61,9 +61,10 @@ impl UserSlicePtrReader { if res != 0 { return Err(error::Error::EFAULT); } - unsafe { - self.0 = self.0.add(data.len()); - } + // Since this is not a pointer to a valid object in our program, + // we cannot use `add`, which has C-style rules for defined + // behavior. + self.0 = self.0.wrapping_offset(data.len()); self.1 -= data.len(); return Ok(()); } @@ -86,9 +87,10 @@ impl UserSlicePtrWriter { if res != 0 { return Err(error::Error::EFAULT); } - unsafe { - self.0 = self.0.add(data.len()); - } + // Since this is not a pointer to a valid object in our program, + // we cannot use `add`, which has C-style rules for defined + // behavior. + self.0 = self.0.wrapping_offset(data.len()); self.1 -= data.len(); Ok(()) } From 831d7d60068fe5f375845365b7bf65018a20e02c Mon Sep 17 00:00:00 2001 From: Geoffrey Thomas Date: Sun, 10 Jun 2018 13:18:37 -0400 Subject: [PATCH 3/3] Add comments, plus one style cleanup --- src/sysctl.rs | 2 ++ src/user_ptr.rs | 48 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 1 deletion(-) diff --git a/src/sysctl.rs b/src/sysctl.rs index b88bca0f..689236e2 100644 --- a/src/sysctl.rs +++ b/src/sysctl.rs @@ -68,6 +68,8 @@ unsafe extern "C" fn proc_handler( len: *mut usize, ppos: *mut bindings::loff_t, ) -> c_types::c_int { + // If we're reading from some offset other than the beginning of the file, + // return an empty read to signal EOF. if *ppos != 0 && write == 0 { *len = 0; return 0; diff --git a/src/user_ptr.rs b/src/user_ptr.rs index c01cb609..08d0223e 100644 --- a/src/user_ptr.rs +++ b/src/user_ptr.rs @@ -14,10 +14,42 @@ extern "C" { ) -> c_types::c_int; } +/// A reference to an area in userspace memory, which can be either +/// read-only or read-write. +/// +/// All methods on this struct are safe: invalid pointers return +/// `EFAULT`. Concurrent access, _including data races to/from userspace +/// memory_, is permitted, because fundamentally another userspace +/// thread / process could always be modifying memory at the same time +/// (in the same way that userspace Rust's std::io permits data races +/// with the contents of files on disk). In the presence of a race, the +/// exact byte values read/written are unspecified but the operation is +/// well-defined. Kernelspace code should validate its copy of data +/// after completing a read, and not expect that multiple reads of the +/// same address will return the same value. +/// +/// Constructing a `UserSlicePtr` only checks that the range is in valid +/// userspace memory, and does not depend on the current process (and +/// can safely be constructed inside a kernel thread with no current +/// userspace process). Reads and writes wrap the kernel APIs +/// `copy_from_user` and `copy_to_user`, and check the memory map of the +/// current process. pub struct UserSlicePtr(*mut c_types::c_void, usize); impl UserSlicePtr { + /// Construct a user slice from a raw pointer and a length in bytes. + /// + /// Checks that the provided range is within the legal area for + /// userspace memory, using `access_ok` (e.g., on i386, the range + /// must be within the first 3 gigabytes), but does not check that + /// the actual pages are mapped in the current process with + /// appropriate permissions. Those checks are handled in the read + /// and write methods. pub fn new(ptr: *mut c_types::c_void, length: usize) -> error::KernelResult { + // No current access_ok implementation actually distinguishes + // between VERIFY_READ and VERIFY_WRITE, so passing VERIFY_WRITE + // is fine in practice and fails safe if a future implementation + // bothers. if unsafe { access_ok_helper(bindings::VERIFY_WRITE, ptr, length as c_types::c_ulong) } == 0 { return Err(error::Error::EFAULT); @@ -25,20 +57,34 @@ impl UserSlicePtr { return Ok(UserSlicePtr(ptr, length)); } + /// Read the entirety of the user slice and return it in a `Vec`. + /// + /// Returns EFAULT if the address does not currently point to + /// mapped, readable memory. pub fn read_all(self) -> error::KernelResult> { let mut data = vec![0; self.1]; self.reader().read(&mut data)?; return Ok(data); } + /// Construct a `UserSlicePtrReader` that can incrementally read + /// from the user slice. pub fn reader(self) -> UserSlicePtrReader { return UserSlicePtrReader(self.0, self.1); } + /// Write the provided slice into the user slice. + /// + /// Returns EFAULT if the address does not currently point to + /// mapped, writable memory (in which case some data from before the + /// fault may be written), or `data` is larger than the user slice + /// (in which case no data is written). pub fn write_all(self, data: &[u8]) -> error::KernelResult<()> { return self.writer().write(data); } + /// Construct a `UserSlicePtrWrite` that can incrementally write + /// into the user slice. pub fn writer(self) -> UserSlicePtrWriter { return UserSlicePtrWriter(self.0, self.1); } @@ -66,7 +112,7 @@ impl UserSlicePtrReader { // behavior. self.0 = self.0.wrapping_offset(data.len()); self.1 -= data.len(); - return Ok(()); + Ok(()) } }