On Thu, 18 Apr 2024 08:59:19 +0000
Alice Ryhl <aliceryhl@xxxxxxxxxx> 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@xxxxxxxxx>
> Signed-off-by: Wedson Almeida Filho <wedsonaf@xxxxxxxxx>
> Reviewed-by: Benno Lossin <benno.lossin@xxxxxxxxx>
> Reviewed-by: Boqun Feng <boqun.feng@xxxxxxxxx>
> Reviewed-by: Trevor Gross <tmgross@xxxxxxxxx>
> Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx>
Reviewed-by: Gary Guo <gary@xxxxxxxxxxx>
> ---
> 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] {}
