Message-ID: <20240416095330.1108574-1-aliceryhl@google.com>
Date: Tue, 16 Apr 2024 09:53:30 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: tmgross@...ch.edu
Cc: a.hindborg@...sung.com, akpm@...ux-foundation.org, alex.gaynor@...il.com, 
	aliceryhl@...gle.com, arnd@...db.de, arve@...roid.com, benno.lossin@...ton.me, 
	bjorn3_gh@...tonmail.com, boqun.feng@...il.com, brauner@...nel.org, 
	cmllamas@...gle.com, gary@...yguo.net, gregkh@...uxfoundation.org, 
	joel@...lfernandes.org, keescook@...omium.org, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org, maco@...roid.com, ojeda@...nel.org, 
	rust-for-linux@...r.kernel.org, surenb@...gle.com, tkjos@...roid.com, 
	viro@...iv.linux.org.uk, wedsonaf@...il.com, willy@...radead.org
Subject: Re: [PATCH v5 3/4] rust: uaccess: add typed accessors for userspace pointers

Trevor Gross <tmgross@...ch.edu> writes:
> On Mon, Apr 15, 2024 at 3:15 AM Alice Ryhl <aliceryhl@...gle.com> wrote:
>>
>> Add safe methods for reading and writing Rust values to and from
>> userspace pointers.
>>
>> The C methods for copying to/from userspace use a function called
>> `check_object_size` to verify that the kernel pointer is not dangling.
>> However, this check is skipped when the length is a compile-time
>> constant, with the assumption that such cases trivially have a correct
>> kernel pointer.
>>
>> In this patch, we apply the same optimization to the typed accessors.
>> For both methods, the size of the operation is known at compile time to
>> be size_of of the type being read or written. Since the C side doesn't
>> provide a variant that skips only this check, we create custom helpers
>> for this purpose.
>>
>> The majority of reads and writes to userspace pointers in the Rust
>> Binder driver uses these accessor methods. Benchmarking has found that
>> skipping the `check_object_size` check makes a big difference for the
>> cases being skipped here. (And that the check doesn't make a difference
>> for the cases that use the raw read/write methods.)
>>
>> This code is based on something that was originally written by Wedson on
>> the old rust branch. It was modified by Alice to skip the
>> `check_object_size` check, and to update various comments, including the
>> notes about kernel pointers in `WritableToBytes`.
>>
>> Co-developed-by: Wedson Almeida Filho <wedsonaf@...il.com>
>> Signed-off-by: Wedson Almeida Filho <wedsonaf@...il.com>
>> Reviewed-by: Benno Lossin <benno.lossin@...ton.me>
>> Reviewed-by: Boqun Feng <boqun.feng@...il.com>
>> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> 
> Couple of docs nits but this looks good to me.
> 
> Reviewed-by: Trevor Gross <tmgross@...ch.edu>

Thanks for taking a look!

>> +/// Types for which any bit pattern is valid.
>> +///
>> +/// Not all types are valid for all values. For example, a `bool` must be either zero or one, so
>> +/// reading arbitrary bytes into something that contains a `bool` is not okay.
>> +///
>> +/// It's okay for the type to have padding, as initializing those bytes has no effect.
>> +///
>> +/// # Safety
>> +///
>> +/// All bit-patterns must be valid for this type.
>> +pub unsafe trait FromBytes {}
> 
> No `UnsafeCell` is also a requirement in zerocopy/bytemuck

I can add that requirement.

>> +/// Types that can be viewed as an immutable slice of initialized bytes.
>> +///
>> +/// If a struct implements this trait, then it is okay to copy it byte-for-byte to userspace. This
>> +/// means that it should not have any padding, as padding bytes are uninitialized. Reading
>> +/// uninitialized memory is not just undefined behavior, it may even lead to leaking sensitive
>> +/// information on the stack to userspace.
>> +///
>> +/// The struct should also not hold kernel pointers, as kernel pointer addresses are also considered
>> +/// sensitive. However, leaking kernel pointers is not considered undefined behavior by Rust, so
>> +/// this is a correctness requirement, but not a safety requirement.
> 
> I don't think mentions of userspace are relevant here since the trait
> is more general. Maybe a `# Interfacing with userspace` section if
> there is enough relevant information.

I think it is relevant. It is the main purpose of the trait right now,
and it is also part of the justification for why the rules are what they
are.

>> +/// # Safety
>> +///
>> +/// Values of this type may not contain any uninitialized bytes.
> 
> No UnsafeCell

Will add.

>> +pub unsafe trait AsBytes {}
> 
>> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
>> index c97029cdeba1..e3953eec61a3 100644
>> --- a/rust/kernel/uaccess.rs
>> +++ b/rust/kernel/uaccess.rs
>> @@ -4,10 +4,15 @@
>>  //!
>>  //! C header: [`include/linux/uaccess.h`](srctree/include/linux/uaccessh)
>>
>> -use crate::{bindings, error::code::*, error::Result};
>> +use crate::{
>> +    bindings,
>> +    error::code::*,
>> +    error::Result,
>> +    types::{AsBytes, FromBytes},
>> +};
>>  use alloc::vec::Vec;
>>  use core::ffi::{c_ulong, c_void};
>> -use core::mem::MaybeUninit;
>> +use core::mem::{size_of, MaybeUninit};
>>
>>  /// A pointer to an area in userspace memory, which can be either read-only or read-write.
>>  ///
>> @@ -238,6 +243,38 @@ pub fn read_slice(&mut self, out: &mut [u8]) -> Result {
>>          self.read_raw(out)
>>      }
>>
>> +    /// Reads a value of the specified type.
>> +    ///
>> +    /// Fails with `EFAULT` if the read encounters a page fault.
>> +    pub fn read<T: FromBytes>(&mut self) -> Result<T> {
>> [...]
>> +    /// Writes the provided Rust value to this userspace pointer.
>> +    ///
>> +    /// Fails with `EFAULT` if the write encounters a page fault.
>> +    pub fn write<T: AsBytes>(&mut self, value: &T) -> Result {
> 
> Read & write could use an example if you are up for it

I may or may not add an example.

Alice

Powered by blists - more mailing lists