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

[Benno Lossin <benno.lossin@proton.me>]

On 17.04.24 16:40, Alice Ryhl wrote:
> On Wed, Apr 17, 2024 at 4:28 PM Gary Guo <gary@garyguo.net> wrote:
>>
>> 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`.
> 
> I don't have a strong opinion here. I suppose a usize could make
> sense. But I also think c_void is fine, and I lean towards not
> changing it. :)
> 
>> 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.
> 
> Hmm, we have to be careful here. Generally, when you get a user slice
> via an ioctl, you should make sure to use the length you get from
> userspace. In binder, it looks like this:
> 
> let user_slice = UserSlice::new(arg, _IOC_SIZE(cmd));
> 
> so whichever API we use, we must make sure to get the length as an
> argument in bytes. What should we do if the length is not a multiple
> of size_of(T)?

We could print a warning and then just floor to the next multiple of
`size_of::<T>()`. I agree that is not perfect, but if one uses the
current API, one also needs to do the length check eventually.

> Another issue is that there's no stable way to get the length from a
> `*mut [T]` without creating a reference, which is not okay for a user
> slice.

Seems like `<* const [T]>::len` (feature `slice_ptr_len`) [1] was just
stabilized 5 days ago [1].

[1]: https://doc.rust-lang.org/std/primitive.pointer.html#method.len-1
[2]: https://github.com/rust-lang/rust/pull/123868

-- 
Cheers,
Benno