| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZvVA3cCcqJr70OV4@phenom.ffwll.local>
Date: Thu, 26 Sep 2024 13:09:17 +0200
From: Simona Vetter <simona.vetter@...ll.ch>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: 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>,
Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
Greg KH <gregkh@...uxfoundation.org>,
Simona Vetter <simona.vetter@...ll.ch>,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] rust: switch uaccess to untrusted data API
On Wed, Sep 25, 2024 at 08:53:11PM +0000, Benno Lossin wrote:
> 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))?;
See below and in my previous mail, I think with the right amount of traits
you can rewrite this as
write.write(&buf)?;
and it will all just neatly work.
> +/// 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)
> }
>
You didn't update the typed read here, intentionally? Should be something
like:
- pub fn read<T: FromBytes>(&mut self) -> Result<T> {
+ pub fn read<T: FromBytes>(&mut self) -> Result<Untrusted<T>> {
> @@ -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 {
I think this one we don't need, but instead we need:
unsafe impl<T: AsBytes> AsBytes for Untrusted<T> {}
and you get the untrusted write for free.
Cheers, Sima
> + 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
>
>
--
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Powered by blists - more mailing lists