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)? 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. Alice
