Message-ID: <20240925205244.873020-3-benno.lossin@proton.me>
Date: Wed, 25 Sep 2024 20:53:11 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, Björn Roy Baron <bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>
Cc: Greg KH <gregkh@...uxfoundation.org>, Simona Vetter <simona.vetter@...ll.ch>, rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH v2 2/2] rust: switch uaccess to untrusted data API

Userspace is an untrusted data source, so all data that can be read from
there is untrusted. Therefore use the new untrusted API for any data
returned from it.
This makes it significantly harder to write TOCTOU bug, the example bug
in the documentation cannot easily be converted to use the new API. Thus
it is removed.

A possible future improvement is to change how `UserSliceWriter` exposes
writing to userspace: a trait `ToUserspace` TBB (to be bikeshed) could
allow users to write untrusted data to a userpointer without adding more
methods to it.

Signed-off-by: Benno Lossin <benno.lossin@...ton.me>
---
 rust/kernel/page.rs    |   8 ++-
 rust/kernel/uaccess.rs | 135 +++++++++++++++++++++--------------------
 2 files changed, 74 insertions(+), 69 deletions(-)

diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
index 208a006d587c..74ce86326893 100644
--- a/rust/kernel/page.rs
+++ b/rust/kernel/page.rs
@@ -5,9 +5,9 @@
 use crate::{
     alloc::{AllocError, Flags},
     bindings,
-    error::code::*,
-    error::Result,
+    error::{code::*, Result},
     uaccess::UserSliceReader,
+    validate::Untrusted,
 };
 use core::ptr::{self, NonNull};
 
@@ -237,8 +237,10 @@ pub unsafe fn copy_from_user_slice_raw(
             // SAFETY: If `with_pointer_into_page` calls into this closure, then it has performed a
             // bounds check and guarantees that `dst` is valid for `len` bytes. Furthermore, we have
             // exclusive access to the slice since the caller guarantees that there are no races.
-            reader.read_raw(unsafe { core::slice::from_raw_parts_mut(dst.cast(), len) })
+            let slice = unsafe { core::slice::from_raw_parts_mut(dst.cast(), len) };
+            reader.read_raw(Untrusted::new_mut(slice))
         })
+        .map(|_| ())
     }
 }
 
diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index e9347cff99ab..3d312f845269 100644
--- a/rust/kernel/uaccess.rs
+++ b/rust/kernel/uaccess.rs
@@ -10,10 +10,12 @@
     error::Result,
     prelude::*,
     types::{AsBytes, FromBytes},
+    validate::Untrusted,
 };
 use alloc::vec::Vec;
 use core::ffi::{c_ulong, c_void};
 use core::mem::{size_of, MaybeUninit};
+use init::init_from_closure;
 
 /// The type used for userspace addresses.
 pub type UserPtr = usize;
@@ -47,59 +49,39 @@
 ///
 /// ```no_run
 /// use alloc::vec::Vec;
-/// use core::ffi::c_void;
-/// use kernel::error::Result;
-/// use kernel::uaccess::{UserPtr, UserSlice};
+/// use core::{convert::Infallible, ffi::c_void};
+/// use kernel::{error::Result, uaccess::{UserPtr, UserSlice}, validate::{Unvalidated, Untrusted, Validate}};
 ///
-/// fn bytes_add_one(uptr: UserPtr, len: usize) -> Result<()> {
-///     let (read, mut write) = UserSlice::new(uptr, len).reader_writer();
+/// struct AddOne<'a>(&'a mut u8);
 ///
-///     let mut buf = Vec::new();
-///     read.read_all(&mut buf, GFP_KERNEL)?;
+/// impl<'a> Validate<&'a mut Unvalidated<u8>> for AddOne<'a> {
+///     type Err = Infallible;
 ///
-///     for b in &mut buf {
-///         *b = b.wrapping_add(1);
+///     fn validate(unvalidated: &'a mut Unvalidated<u8>) -> Result<Self, Self::Err> {
+///         // We are not doing any kind of validation here on purpose. After all, we only want to
+///         // increment the value and write it back.
+///         Ok(Self(unvalidated.raw_mut()))
 ///     }
-///
-///     write.write_slice(&buf)?;
-///     Ok(())
 /// }
-/// ```
-///
-/// Example illustrating a TOCTOU (time-of-check to time-of-use) bug.
 ///
-/// ```no_run
-/// use alloc::vec::Vec;
-/// use core::ffi::c_void;
-/// use kernel::error::{code::EINVAL, Result};
-/// use kernel::uaccess::{UserPtr, UserSlice};
+/// impl AddOne<'_> {
+///     fn inc(&mut self) {
+///         *self.0 = self.0.wrapping_add(1);
+///     }
+/// }
 ///
-/// /// Returns whether the data in this region is valid.
-/// fn is_valid(uptr: UserPtr, len: usize) -> Result<bool> {
-///     let read = UserSlice::new(uptr, len).reader();
+/// fn bytes_add_one(uptr: UserPtr, len: usize) -> Result<()> {
+///     let (read, mut write) = UserSlice::new(uptr, len).reader_writer();
 ///
 ///     let mut buf = Vec::new();
 ///     read.read_all(&mut buf, GFP_KERNEL)?;
 ///
-///     todo!()
-/// }
-///
-/// /// Returns the bytes behind this user pointer if they are valid.
-/// fn get_bytes_if_valid(uptr: UserPtr, len: usize) -> Result<Vec<u8>> {
-///     if !is_valid(uptr, len)? {
-///         return Err(EINVAL);
+///     for b in &mut buf {
+///         b.validate_mut::<AddOne<'_>>()?.inc();
 ///     }
 ///
-///     let read = UserSlice::new(uptr, len).reader();
-///
-///     let mut buf = Vec::new();
-///     read.read_all(&mut buf, GFP_KERNEL)?;
-///
-///     // THIS IS A BUG! The bytes could have changed since we checked them.
-///     //
-///     // To avoid this kind of bug, don't call `UserSlice::new` multiple
-///     // times with the same address.
-///     Ok(buf)
+///     write.write_untrusted_slice(Untrusted::transpose_slice(&buf))?;
+///     Ok(())
 /// }
 /// ```
 ///
@@ -130,7 +112,7 @@ pub fn new(ptr: UserPtr, length: usize) -> Self {
     /// Reads the entirety of the user slice, appending it to the end of the provided buffer.
     ///
     /// Fails with [`EFAULT`] if the read happens on a bad address.
-    pub fn read_all(self, buf: &mut Vec<u8>, flags: Flags) -> Result {
+    pub fn read_all(self, buf: &mut Vec<Untrusted<u8>>, flags: Flags) -> Result {
         self.reader().read_all(buf, flags)
     }
 
@@ -218,38 +200,47 @@ pub fn is_empty(&self) -> bool {
     /// Fails with [`EFAULT`] if the read happens on a bad address, or if the read goes out of
     /// bounds of this [`UserSliceReader`]. This call may modify `out` even if it returns an error.
     ///
+    /// Returns a reference to the initialized bytes in `out`.
+    ///
     /// # Guarantees
     ///
     /// After a successful call to this method, all bytes in `out` are initialized.
-    pub fn read_raw(&mut self, out: &mut [MaybeUninit<u8>]) -> Result {
-        let len = out.len();
-        let out_ptr = out.as_mut_ptr().cast::<c_void>();
-        if len > self.length {
-            return Err(EFAULT);
-        }
-        let Ok(len_ulong) = c_ulong::try_from(len) else {
-            return Err(EFAULT);
+    pub fn read_raw<'a>(
+        &'a mut self,
+        out: &'a mut Untrusted<[MaybeUninit<u8>]>,
+    ) -> Result<&'a mut Untrusted<[u8]>> {
+        let init = |ptr: *mut [u8]| {
+            let out_ptr = ptr.cast::<c_void>();
+            let len = ptr.len();
+            if len > self.length {
+                return Err(EFAULT);
+            }
+            let Ok(len_ulong) = c_ulong::try_from(len) else {
+                return Err(EFAULT);
+            };
+            // SAFETY: `out_ptr` points into a mutable slice of length `len_ulong`, so we may write
+            // that many bytes to it.
+            let res =
+                unsafe { bindings::copy_from_user(out_ptr, self.ptr as *const c_void, len_ulong) };
+            if res != 0 {
+                return Err(EFAULT);
+            }
+            self.ptr = self.ptr.wrapping_add(len);
+            self.length -= len;
+            Ok(())
         };
-        // SAFETY: `out_ptr` points into a mutable slice of length `len_ulong`, so we may write
-        // that many bytes to it.
-        let res =
-            unsafe { bindings::copy_from_user(out_ptr, self.ptr as *const c_void, len_ulong) };
-        if res != 0 {
-            return Err(EFAULT);
-        }
-        self.ptr = self.ptr.wrapping_add(len);
-        self.length -= len;
-        Ok(())
+        out.write_uninit_slice(unsafe { init_from_closure(init) })
     }
 
     /// Reads raw data from the user slice into a kernel buffer.
     ///
     /// Fails with [`EFAULT`] if the read happens on a bad address, or if the read goes out of
     /// bounds of this [`UserSliceReader`]. This call may modify `out` even if it returns an error.
-    pub fn read_slice(&mut self, out: &mut [u8]) -> Result {
+    pub fn read_slice(&mut self, out: &mut Untrusted<[u8]>) -> Result<&mut Untrusted<[u8]>> {
         // SAFETY: The types are compatible and `read_raw` doesn't write uninitialized bytes to
         // `out`.
-        let out = unsafe { &mut *(out as *mut [u8] as *mut [MaybeUninit<u8>]) };
+        let out =
+            unsafe { &mut *(out as *mut Untrusted<[u8]> as *mut Untrusted<[MaybeUninit<u8>]>) };
         self.read_raw(out)
     }
 
@@ -291,13 +282,15 @@ pub fn read<T: FromBytes>(&mut self) -> Result<T> {
     /// Reads the entirety of the user slice, appending it to the end of the provided buffer.
     ///
     /// Fails with [`EFAULT`] if the read happens on a bad address.
-    pub fn read_all(mut self, buf: &mut Vec<u8>, flags: Flags) -> Result {
+    pub fn read_all(mut self, buf: &mut Vec<Untrusted<u8>>, flags: Flags) -> Result {
         let len = self.length;
-        VecExt::<u8>::reserve(buf, len, flags)?;
+        VecExt::<_>::reserve(buf, len, flags)?;
 
         // The call to `try_reserve` was successful, so the spare capacity is at least `len` bytes
         // long.
-        self.read_raw(&mut buf.spare_capacity_mut()[..len])?;
+        self.read_raw(Untrusted::transpose_slice_uninit_mut(
+            &mut buf.spare_capacity_mut()[..len],
+        ))?;
 
         // SAFETY: Since the call to `read_raw` was successful, so the next `len` bytes of the
         // vector have been initialized.
@@ -333,8 +326,18 @@ pub fn is_empty(&self) -> bool {
     /// bounds of this [`UserSliceWriter`]. This call may modify the associated userspace slice even
     /// if it returns an error.
     pub fn write_slice(&mut self, data: &[u8]) -> Result {
-        let len = data.len();
-        let data_ptr = data.as_ptr().cast::<c_void>();
+        self.write_untrusted_slice(Untrusted::new_ref(data))
+    }
+
+    /// Writes raw data to this user pointer from a kernel buffer.
+    ///
+    /// Fails with [`EFAULT`] if the write happens on a bad address, or if the write goes out of
+    /// bounds of this [`UserSliceWriter`]. This call may modify the associated userspace slice even
+    /// if it returns an error.
+    pub fn write_untrusted_slice(&mut self, data: &Untrusted<[u8]>) -> Result {
+        let data_ptr = (data as *const _) as *const [u8];
+        let len = data_ptr.len();
+        let data_ptr = data_ptr.cast::<c_void>();
         if len > self.length {
             return Err(EFAULT);
         }
-- 
2.46.0

Powered by blists - more mailing lists