Re: [PATCH v5 1/4] rust: uaccess: add userspace pointers

[Gary Guo <gary@garyguo.net>]

On Mon, 15 Apr 2024 07:13:53 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
> A pointer to an area in userspace memory, which can be either read-only
> or read-write.
> 
> All methods on this struct are safe: attempting to read or write on bad
> addresses (either out of the bound of the slice or unmapped addresses)
> will 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.
> 
> These APIs are designed to make it difficult to accidentally write
> TOCTOU bugs. Every time you read from a memory location, the pointer is
> advanced by the length so that you cannot use that reader to read the
> same memory location twice. Preventing double-fetches avoids TOCTOU
> bugs. This is 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, then that is done by creating multiple
> readers to the same memory location.
> 
> 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).
> 
> This code is based on something that was originally written by Wedson on
> the old rust branch. It was modified by Alice by removing the
> `IoBufferReader` and `IoBufferWriter` traits, and various other changes.
> 
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/helpers.c         |  14 +++
>  rust/kernel/lib.rs     |   1 +
>  rust/kernel/uaccess.rs | 304 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 319 insertions(+)
> 
> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs

> +/// [`std::io`]: https://doc.rust-lang.org/std/io/index.html
> +/// [`clone_reader`]: UserSliceReader::clone_reader
> +pub struct UserSlice {
> +    ptr: *mut c_void,
> +    length: usize,
> +}

How useful is the `c_void` in the struct and new signature? They tend
to not be very useful in Rust. Given that provenance doesn't matter
for userspace pointers, could this be `usize` simply?

I think `*mut u8` or `*mut ()` makes more sense than `*mut c_void` for
Rust code even if we don't want to use `usize`.

---

Some thinking aloud and brainstorming bits about the API.

I wonder if it make sense to have `User<[u8]>` instead of `UserSlice`?
The `User` type can be defined like this:

```rust
struct User<T: ?Sized> {
   ptr: *mut T,
}
```

and this allows arbitrary T as long as it's POD. So we could have
`User<[u8]>`, `User<u32>`, `User<PodStruct>`. I imagine the
`User<[u8]>` would be the general usage and the latter ones can be
especially helpful if you are trying to implement ioctl and need to
copy fixed size data structs from userspace.

Best,
Gary