Merge pull request #157 from wedsonaf/access_ok

`access_ok` is no longer needed.

Author: ojeda

rust/helpers.c

@@ -10,11 +10,6 @@ void rust_helper_BUG(void)
 	BUG();
 }
 
-int rust_helper_access_ok(const void __user *addr, unsigned long n)
-{
-	return access_ok(addr, n);
-}
-
 unsigned long rust_helper_copy_from_user(void *to, const void __user *from, unsigned long n)
 {
 	return copy_from_user(to, from, n);

rust/kernel/file_operations.rs

@@ -100,7 +100,7 @@ unsafe extern "C" fn read_callback<T: FileOperations>(
     offset: *mut bindings::loff_t,
 ) -> c_types::c_ssize_t {
     from_kernel_result! {
-        let mut data = UserSlicePtr::new(buf as *mut c_types::c_void, len)?.writer();
+        let mut data = UserSlicePtr::new(buf as *mut c_types::c_void, len).writer();
         let f = &*((*file).private_data as *const T);
         // No `FMODE_UNSIGNED_OFFSET` support, so `offset` must be in [0, 2^63).
         // See discussion in https://github.com/fishinabarrel/linux-kernel-module-rust/pull/113
@@ -117,7 +117,7 @@ unsafe extern "C" fn write_callback<T: FileOperations>(
     offset: *mut bindings::loff_t,
 ) -> c_types::c_ssize_t {
     from_kernel_result! {
-        let mut data = UserSlicePtr::new(buf as *mut c_types::c_void, len)?.reader();
+        let mut data = UserSlicePtr::new(buf as *mut c_types::c_void, len).reader();
         let f = &*((*file).private_data as *const T);
         // No `FMODE_UNSIGNED_OFFSET` support, so `offset` must be in [0, 2^63).
         // See discussion in https://github.com/fishinabarrel/linux-kernel-module-rust/pull/113
@@ -359,26 +359,12 @@ pub struct IoctlCommand {
 
 impl IoctlCommand {
     /// Constructs a new [`IoctlCommand`].
-    ///
-    /// # Safety
-    ///
-    /// The caller must ensure that `fs` is compatible with `arg` and the original caller's
-    /// context. For example, if the original caller is from userland (e.g., through the ioctl
-    /// syscall), then `arg` is untrusted and `fs` should therefore be `USER_DS`.
-    unsafe fn new(cmd: u32, arg: usize) -> Self {
-        let user_slice = {
-            let dir = (cmd >> bindings::_IOC_DIRSHIFT) & bindings::_IOC_DIRMASK;
-            if dir == bindings::_IOC_NONE {
-                None
-            } else {
-                let size = (cmd >> bindings::_IOC_SIZESHIFT) & bindings::_IOC_SIZEMASK;
-
-                // SAFETY: We only create one instance of the user slice, so TOCTOU issues are not
-                // possible. The `set_fs` requirements are imposed on the caller.
-                UserSlicePtr::new(arg as _, size as _).ok()
-            }
-        };
+    fn new(cmd: u32, arg: usize) -> Self {
+        let size = (cmd >> bindings::_IOC_SIZESHIFT) & bindings::_IOC_SIZEMASK;
 
+        // SAFETY: We only create one instance of the user slice per ioctl call, so TOCTOU issues
+        // are not possible.
+        let user_slice = Some(unsafe { UserSlicePtr::new(arg as _, size as _) });
         Self {
             cmd,
             arg,
@@ -398,7 +384,7 @@ impl IoctlCommand {
             return T::pure(handler, file, self.cmd, self.arg);
         }
 
-        let data = self.user_slice.take().ok_or(Error::EFAULT)?;
+        let data = self.user_slice.take().ok_or(Error::EINVAL)?;
         const READ_WRITE: u32 = bindings::_IOC_READ | bindings::_IOC_WRITE;
         match dir {
             bindings::_IOC_WRITE => T::write(handler, file, self.cmd, &mut data.reader()),

rust/kernel/sysctl.rs

@@ -106,10 +106,7 @@ unsafe extern "C" fn proc_handler<T: SysctlStorage>(
         return 0;
     }
 
-    let data = match UserSlicePtr::new(buffer, *len) {
-        Ok(ptr) => ptr,
-        Err(e) => return e.to_kernel_errno(),
-    };
+    let data = UserSlicePtr::new(buffer, *len);
     let storage = &*((*ctl).data as *const T);
     let (bytes_processed, result) = if write != 0 {
         let data = match data.read_all() {

rust/kernel/user_ptr.rs

@@ -9,9 +9,6 @@ use alloc::vec::Vec;
 use core::mem::{size_of, MaybeUninit};
 
 extern "C" {
-    fn rust_helper_access_ok(addr: *const c_types::c_void, len: c_types::c_ulong)
-        -> c_types::c_int;
-
     fn rust_helper_copy_from_user(
         to: *mut c_types::c_void,
         from: *const c_types::c_void,
@@ -69,42 +66,27 @@ unsafe impl ReadableFromBytes for isize {}
 /// obtaining multiple readers on a given [`UserSlicePtr`], and the readers
 /// only permitting forward reads.
 ///
-/// 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.
+/// Constructing a [`UserSlicePtr`] performs no checks on the provided
+/// address and length, it 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`, which check the memory map of the
+/// current process and enforce that the address range is within the user
+/// range (no additional calls to `access_ok` are needed).
 ///
 /// [`std::io`]: https://doc.rust-lang.org/std/io/index.html
 pub struct UserSlicePtr(*mut c_types::c_void, usize);
 
 impl UserSlicePtr {
     /// Constructs 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 GiB), 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.
-    ///
     /// # Safety
     ///
-    /// This is `unsafe` because if it is called within `set_fs(KERNEL_DS)`
-    /// context then `access_ok` will not do anything. As a result the only
-    /// place you can safely use this is with a `__user` pointer that was
-    /// provided by the kernel.
-    ///
-    /// Callers must also be careful to avoid time-of-check-time-of-use
+    /// Callers must be careful to avoid time-of-check-time-of-use
     /// (TOCTOU) issues. The simplest way is to create a single instance of
     /// [`UserSlicePtr`] per user memory block as it reads each byte at
     /// most once.
-    pub unsafe fn new(ptr: *mut c_types::c_void, length: usize) -> KernelResult<UserSlicePtr> {
-        if rust_helper_access_ok(ptr, length as c_types::c_ulong) == 0 {
-            return Err(error::Error::EFAULT);
-        }
-        Ok(UserSlicePtr(ptr, length))
+    pub unsafe fn new(ptr: *mut c_types::c_void, length: usize) -> Self {
+        UserSlicePtr(ptr, length)
     }
 
     /// Reads the entirety of the user slice.