On Mon, Apr 15, 2024 at 07:13:53AM +0000, Alice Ryhl 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> Thanks! Reviewed-by: Boqun Feng <boqun.feng@xxxxxxxxx> Two small nits below.. > --- > rust/helpers.c | 14 +++ > rust/kernel/lib.rs | 1 + > rust/kernel/uaccess.rs | 304 +++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 319 insertions(+) > [...] > + /// Reads raw data from the user slice into a kernel buffer. > + /// > + /// Fails with `EFAULT` if the read happens on a bad address. ... we probably want to mention that `out` may get modified even in failure cases. > + pub fn read_slice(&mut self, out: &mut [u8]) -> Result { > + // SAFETY: The types are compatible and `read_raw` doesn't write uninitialized bytes to > + // `out`. > + let out = unsafe { &mut *(out as *mut [u8] as *mut [MaybeUninit<u8>]) }; > + self.read_raw(out) > + } > + [...] > + > +impl UserSliceWriter { [...] > + > + /// Writes raw data to this user pointer from a kernel buffer. > + /// > + /// Fails with `EFAULT` if the write happens on a bad address. Same here, probably mention that: the userspace memory may be modified even in failure cases. Anyway, they are not correctness critical, so we can do these in later patches. Regards, Boqun > + pub fn write_slice(&mut self, data: &[u8]) -> Result { > + let len = data.len(); > + let data_ptr = data.as_ptr().cast::<c_void>(); > + if len > self.length { > + return Err(EFAULT); > + } > + let Ok(len_ulong) = c_ulong::try_from(len) else { > + return Err(EFAULT); > + }; > + // SAFETY: `data_ptr` points into an immutable slice of length `len_ulong`, so we may read > + // that many bytes from it. > + let res = unsafe { bindings::copy_to_user(self.ptr, data_ptr, len_ulong) }; > + if res != 0 { > + return Err(EFAULT); > + } > + // Userspace pointers are not directly dereferencable by the kernel, so > + // we cannot use `add`, which has C-style rules for defined behavior. > + self.ptr = self.ptr.wrapping_byte_add(len); > + self.length -= len; > + Ok(()) > + } > +} > > -- > 2.44.0.683.g7961c838ac-goog > >