Message-ID: <20240425171358.54dc96e4@eugeo>
Date: Thu, 25 Apr 2024 17:13:58 +0100
From: Gary Guo <gary@...yguo.net>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Matthew Wilcox <willy@...radead.org>,
 Al Viro <viro@...iv.linux.org.uk>, Andrew Morton
 <akpm@...ux-foundation.org>, Kees Cook <keescook@...omium.org>, Alex Gaynor
 <alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Boqun
 Feng <boqun.feng@...il.com>, "Björn Roy Baron"
 <bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>, Andreas
 Hindborg <a.hindborg@...sung.com>, Greg Kroah-Hartman
 <gregkh@...uxfoundation.org>, "Arve Hjønnevåg"
 <arve@...roid.com>, Todd Kjos <tkjos@...roid.com>, Martijn Coenen
 <maco@...roid.com>, Joel Fernandes <joel@...lfernandes.org>, Carlos Llamas
 <cmllamas@...gle.com>, Suren Baghdasaryan <surenb@...gle.com>, Arnd
 Bergmann <arnd@...db.de>, Trevor Gross <tmgross@...ch.edu>,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org, Christian Brauner <brauner@...nel.org>
Subject: Re: [PATCH v6 3/4] rust: uaccess: add typed accessors for userspace
 pointers

On Thu, 18 Apr 2024 08:59:19 +0000
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>
> Reviewed-by: Trevor Gross <tmgross@...ch.edu>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>

Reviewed-by: Gary Guo <gary@...yguo.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] {}

Powered by blists - more mailing lists