Received-SPF: pass (google.com: domain of linux-kernel+bounces-47600-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240124-alice-mm-v1-0-d1abcec83c44@google.com> <20240124-alice-mm-v1-1-d1abcec83c44@google.com>
In-Reply-To: <20240124-alice-mm-v1-1-d1abcec83c44@google.com>
From: Trevor Gross <tmgross@umich.edu>
Date: Wed, 31 Jan 2024 23:06:17 -0500
Message-ID: <CALNs47uDaD05oD8TtZoSqUBc4SaRig80u2_1P0qMCXtE3H9_Vw@mail.gmail.com>
Subject: Re: [PATCH 1/3] rust: add userspace pointers
To: Alice Ryhl <aliceryhl@google.com>
Cc: Miguel Ojeda <ojeda@kernel.org>, Alex Gaynor <alex.gaynor@gmail.com>, 
	Wedson Almeida Filho <wedsonaf@gmail.com>, Boqun Feng <boqun.feng@gmail.com>, Gary Guo <gary@garyguo.net>, 
	=?UTF-8?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, 
	Benno Lossin <benno.lossin@proton.me>, Andreas Hindborg <a.hindborg@samsung.com>, 
	Kees Cook <keescook@chromium.org>, Al Viro <viro@zeniv.linux.org.uk>, 
	Andrew Morton <akpm@linux-foundation.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	=?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= <arve@android.com>, 
	Todd Kjos <tkjos@android.com>, Martijn Coenen <maco@android.com>, 
	Joel Fernandes <joel@joelfernandes.org>, Carlos Llamas <cmllamas@google.com>, 
	Suren Baghdasaryan <surenb@google.com>, Arnd Bergmann <arnd@arndb.de>, linux-mm@kvack.org, 
	linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org, 
	Christian Brauner <brauner@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Jan 24, 2024 at 6:21=E2=80=AFAM Alice Ryhl <aliceryhl@google.com> w=
rote:
> --- /dev/null
> +++ b/rust/kernel/user_ptr.rs
> @@ -0,0 +1,222 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! User pointers.
> +//!
> +//! C header: [`include/linux/uaccess.h`](../../../../include/linux/uacc=
ess.h)
> +
> +// Comparison with MAX_USER_OP_LEN triggers this lint on platforms
> +// where `c_ulong =3D=3D usize`.
> +#![allow(clippy::absurd_extreme_comparisons)]
> +
> +use crate::{bindings, error::code::*, error::Result};
> +use alloc::vec::Vec;
> +use core::ffi::{c_ulong, c_void};
> +
> +/// The maximum length of a operation using `copy_[from|to]_user`.
> +///
> +/// If a usize is not greater than this constant, then casting it to `c_=
ulong`
> +/// is guaranteed to be lossless.
> +const MAX_USER_OP_LEN: usize =3D c_ulong::MAX as usize;
> +
> +/// A pointer to an area in userspace memory, which can be either read-o=
nly or
> +/// read-write.
> +///
> +/// All methods on this struct are safe: invalid pointers return `EFAULT=
`.

Maybe

    ... attempting to read or write invalid pointers will return `EFAULT`.

> +/// Concurrent access, *including data races to/from userspace memory*, =
is
> +/// permitted, because fundamentally another userspace thread/process co=
uld
> +/// always be modifying memory at the same time (in the same way that us=
erspace
> +/// 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 shou=
ld
> +/// validate its copy of data after completing a read, and not expect th=
at
> +/// multiple reads of the same address will return the same value.
> +///
> +/// These APIs are designed to make it difficult to accidentally write T=
OCTOU
> +/// bugs. Every time you read from a memory location, the pointer is adv=
anced by
> +/// the length so that you cannot use that reader to read the same memor=
y
> +/// location twice. Preventing double-fetches avoids TOCTOU bugs. This i=
s
> +/// accomplished by taking `self` by value to prevent obtaining multiple=
 readers
> +/// on a given [`UserSlicePtr`], and the readers only permitting forward=
 reads.
> +/// If double-fetching a memory location is necessary for some reason, t=
hen that
> +/// is done by creating multiple readers to the same memory location, e.=
g. using
> +/// [`clone_reader`].

Maybe something like

    Every time a memory location is read, the reader's position is advanced=
 by
    the read length and the next read will start from there. This helps pre=
vent
    accidentally reading the same location twice and causing a TOCTOU bug.

    Creating a [`UserSlicePtrReader`] and/or [`UserSlicePtrWriter`]
    consumes the `UserSlicePtr`, helping ensure that there aren't multiple
    readers or writers to the same location.

    If double fetching a memory location...

> +///
> +/// Constructing a [`UserSlicePtr`] performs no checks on the provided a=
ddress
> +/// and length, it can safely be constructed inside a kernel thread with=
 no
> +/// current userspace process. Reads and writes wrap the kernel APIs

Maybe some of this is better documented on `new` rather than the type?

> +/// `copy_from_user` and `copy_to_user`, which check the memory map of t=
he
> +/// current process and enforce that the address range is within the use=
r range
> +/// (no additional calls to `access_ok` are needed).
> +///
> +/// [`std::io`]: https://doc.rust-lang.org/std/io/index.html
> +/// [`clone_reader`]: UserSlicePtrReader::clone_reader
> +pub struct UserSlicePtr(*mut c_void, usize);

I think just `UserSlice` could work for the name here, since
`SlicePtr` is a bit redundant (slices automatically containing a
pointer). Also makes the Reader/Writer types a bit less wordy. Though
I understand wanting to make it discoverable for C users.

Is some sort of `Debug` implementation useful?

> +impl UserSlicePtr {
> +    /// Constructs a user slice from a raw pointer and a length in bytes=
.
> +    ///
> +    /// 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 fn new(ptr: *mut c_void, length: usize) -> Self {
> +        UserSlicePtr(ptr, length)
> +    }
> +
> +    /// Reads the entirety of the user slice.
> +    ///
> +    /// Returns `EFAULT` if the address does not currently point to
> +    /// mapped, readable memory.
> +    pub fn read_all(self) -> Result<Vec<u8>> {
> +        self.reader().read_all()
> +    }

std::io uses the signature

    fn read_to_end(&mut self, buf: &mut Vec<u8>) -> Result<usize> { ... }

Maybe this could be better here too, since it allows reusing the
vector without going through `read_raw`.

https://doc.rust-lang.org/std/io/trait.Read.html#method.read_to_end

> +    /// Constructs a [`UserSlicePtrReader`].
> +    pub fn reader(self) -> UserSlicePtrReader {
> +        UserSlicePtrReader(self.0, self.1)
> +    }

I wonder if this should be called `into_reader` to note that it is a
consuming method. Indifferent here, since there aren't any non-`self`
variants.

> +
> +    /// Constructs a [`UserSlicePtrWriter`].
> +    pub fn writer(self) -> UserSlicePtrWriter {
> +        UserSlicePtrWriter(self.0, self.1)
> +    }
> +
> +    /// Constructs both a [`UserSlicePtrReader`] and a [`UserSlicePtrWri=
ter`].

Could you add a brief about what is or isn't allowed when you have a
reader and a writer to the same location?

> +    pub fn reader_writer(self) -> (UserSlicePtrReader, UserSlicePtrWrite=
r) {
> +        (
> +            UserSlicePtrReader(self.0, self.1),
> +            UserSlicePtrWriter(self.0, self.1),
> +        )
> +    }
> +}
> +
> +/// A reader for [`UserSlicePtr`].
> +///
> +/// Used to incrementally read from the user slice.
> +pub struct UserSlicePtrReader(*mut c_void, usize);
> +
> +impl UserSlicePtrReader {
> +    /// Skip the provided number of bytes.
> +    ///
> +    /// Returns an error if skipping more than the length of the buffer.
> +    pub fn skip(&mut self, num_skip: usize) -> Result {
> +        // Update `self.1` first since that's the fallible one.
> +        self.1 =3D self.1.checked_sub(num_skip).ok_or(EFAULT)?;
> +        self.0 =3D self.0.wrapping_add(num_skip);

I think it would be better to change to named structs to make this more cle=
ar.

> +        Ok(())
> +    }
> +
> +    /// Create a reader that can access the same range of data.
> +    ///
> +    /// Reading from the clone does not advance the current reader.
> +    ///
> +    /// The caller should take care to not introduce TOCTOU issues.

Could you add an example of what should be avoided?

> +    pub fn clone_reader(&self) -> UserSlicePtrReader {
> +        UserSlicePtrReader(self.0, self.1)
> +    }
> +
> +    /// Returns the number of bytes left to be read from this.

Remove "from this" or change to "from this reader".

> +    ///
> +    /// Note that even reading less than this number of bytes may fail.>=
 +    pub fn len(&self) -> usize {
> +        self.1
> +    }
> +
> +    /// Returns `true` if no data is available in the io buffer.
> +    pub fn is_empty(&self) -> bool {
> +        self.1 =3D=3D 0
> +    }
> +
> +    /// Reads raw data from the user slice into a raw kernel buffer.
> +    ///
> +    /// Fails with `EFAULT` if the read encounters a page fault.

This is raised if the address is invalid right, not just page faults?
Or, are page faults just the only mode of indication that a pointer
isn't valid.

I know this is the same as copy_from_user, but I don't understand that
too well myself. I'm also a bit curious what happens if you pass a
kernel pointer to these methods.

> +    /// # Safety
> +    ///
> +    /// The `out` pointer must be valid for writing `len` bytes.
> +    pub unsafe fn read_raw(&mut self, out: *mut u8, len: usize) -> Resul=
t {

What is the motivation for taking raw pointers rather than a slice
here? In which case it could just be called `read`.

> +        if len > self.1 || len > MAX_USER_OP_LEN {
> +            return Err(EFAULT);
> +        }
> +        // SAFETY: The caller promises that `out` is valid for writing `=
len` bytes.
> +        let res =3D unsafe { bindings::copy_from_user(out.cast::<c_void>=
(), self.0, len as c_ulong) };

To me, it would be more clear to `c_ulong::try_from(len).map_err(|_|
EFAULT)?` rather than using MAX_USER_OP_LEN with an `as` cast, since
it makes it immediately clear that the conversion is ok (and is doing
that same comparison)

> +        if res !=3D 0 {
> +            return Err(EFAULT);
> +        }
> +        // 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 =3D self.0.wrapping_add(len);
> +        self.1 -=3D len;
> +        Ok(())
> +    }
> +
> +    /// Reads all remaining data in the buffer into a vector.
> +    ///
> +    /// Fails with `EFAULT` if the read encounters a page fault.
> +    pub fn read_all(&mut self) -> Result<Vec<u8>> {

Same as the above note about read_to_end

> +        let len =3D self.len();
> +        let mut data =3D Vec::<u8>::try_with_capacity(len)?;
> +
> +        // SAFETY: The output buffer is valid for `len` bytes as we just=
 allocated that much space.
> +        unsafe { self.read_raw(data.as_mut_ptr(), len)? };

If making the change above (possibly even if not), going through
https://doc.rust-lang.org/std/vec/struct.Vec.html#method.spare_capacity_mut
can be more clear about uninitialized data.

> +
> +        // SAFETY: Since the call to `read_raw` was successful, the firs=
t `len` bytes of the vector
> +        // have been initialized.
> +        unsafe { data.set_len(len) };
> +        Ok(data)
> +    }
> +}
> +
> +/// A writer for [`UserSlicePtr`].
> +///
> +/// Used to incrementally write into the user slice.
> +pub struct UserSlicePtrWriter(*mut c_void, usize);
> +
> +impl UserSlicePtrWriter {
> +    /// Returns the amount of space remaining in this buffer.
> +    ///
> +    /// Note that even writing less than this number of bytes may fail.
> +    pub fn len(&self) -> usize {
> +        self.1
> +    }
> +
> +    /// Returns `true` if no more data can be written to this buffer.
> +    pub fn is_empty(&self) -> bool {
> +        self.1 =3D=3D 0
> +    }
> +
> +    /// Writes raw data to this user pointer from a raw kernel buffer.
> +    ///
> +    /// Fails with `EFAULT` if the write encounters a page fault.
> +    ///
> +    /// # Safety
> +    ///
> +    /// The `data` pointer must be valid for reading `len` bytes.
> +    pub unsafe fn write_raw(&mut self, data: *const u8, len: usize) -> R=
esult {

Same as for Reader regarding signature

> +        if len > self.1 || len > MAX_USER_OP_LEN {
> +            return Err(EFAULT);
> +        }
> +        let res =3D unsafe { bindings::copy_to_user(self.0, data.cast::<=
c_void>(), len as c_ulong) };

Same as for Reader regarding `as` casts.

> +        if res !=3D 0 {
> +            return Err(EFAULT);
> +        }
> +        // 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 =3D self.0.wrapping_add(len);
> +        self.1 -=3D len;
> +        Ok(())
> +    }
> +
> +    /// Writes the provided slice to this user pointer.
> +    ///
> +    /// Fails with `EFAULT` if the write encounters a page fault.
> +    pub fn write_slice(&mut self, data: &[u8]) -> Result {

This could probably just be called `write`, which would be consistent
with std::io::Write.

> +        let len =3D data.len();
> +        let ptr =3D data.as_ptr();
> +        // SAFETY: The pointer originates from a reference to a slice of=
 length
> +        // `len`, so the pointer is valid for reading `len` bytes.
> +        unsafe { self.write_raw(ptr, len) }
> +    }
> +}
>
> --
> 2.43.0.429.g432eaa2c6b-goog

The docs could use usage examples, but I am sure you are planning on
that already :)

- Trevor