On 17.04.24 16:40, Alice Ryhl wrote: > On Wed, Apr 17, 2024 at 4:28 PM Gary Guo <gary@xxxxxxxxxxx> wrote: >> >> On Mon, 15 Apr 2024 07:13:53 +0000 >> Alice Ryhl <aliceryhl@xxxxxxxxxx> wrote: >> >>> From: Wedson Almeida Filho <wedsonaf@xxxxxxxxx> >>> >>> 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@xxxxxxxxx> >>> Co-developed-by: Alice Ryhl <aliceryhl@xxxxxxxxxx> >>> Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx> >>> --- >>> 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
