Re: [PATCH v6 3/4] rust: uaccess: add typed accessors for userspace pointers

[Gary Guo <gary@garyguo.net>]

On Thu, 18 Apr 2024 08:59:19 +0000
Alice Ryhl <aliceryhl@google.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@gmail.com>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> Reviewed-by: Benno Lossin <benno.lossin@proton.me>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Reviewed-by: Trevor Gross <tmgross@umich.edu>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/types.rs   | 64 ++++++++++++++++++++++++++++++++++++++++
>  rust/kernel/uaccess.rs | 79 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 141 insertions(+), 2 deletions(-)
> 
> diff --git a/rust/kernel/types.rs b/rust/kernel/types.rs
> index 8fad61268465..9c57c6c75553 100644
> --- a/rust/kernel/types.rs
> +++ b/rust/kernel/types.rs
>
> +/// 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.
> +///
> +/// # Safety
> +///
> +/// Values of this type may not contain any uninitialized bytes. This type must not have interior
> +/// mutability.
> +pub unsafe trait AsBytes {}
> +
> +// SAFETY: Instances of the following types have no uninitialized portions.
> +unsafe impl AsBytes for u8 {}
> +unsafe impl AsBytes for u16 {}
> +unsafe impl AsBytes for u32 {}
> +unsafe impl AsBytes for u64 {}
> +unsafe impl AsBytes for usize {}
> +unsafe impl AsBytes for i8 {}
> +unsafe impl AsBytes for i16 {}
> +unsafe impl AsBytes for i32 {}
> +unsafe impl AsBytes for i64 {}
> +unsafe impl AsBytes for isize {}
> +unsafe impl AsBytes for bool {}
> +unsafe impl AsBytes for char {}
> +unsafe impl AsBytes for str {}
> +// SAFETY: If individual values in an array have no uninitialized portions, then the array itself
> +// does not have any uninitialized portions either.
> +unsafe impl<T: AsBytes> AsBytes for [T] {}

nit: I would move `str` to here, since `str` is essentially `[u8]` with
UTF-8 guarantee.

> +unsafe impl<T: AsBytes, const N: usize> AsBytes for [T; N] {}