Received-SPF: pass (google.com: domain of linux-kernel+bounces-158877-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 25 Apr 2024 17:13:58 +0100
From: Gary Guo <gary@garyguo.net>
To: Alice Ryhl <aliceryhl@google.com>
Cc: Miguel Ojeda <ojeda@kernel.org>, Matthew Wilcox <willy@infradead.org>,
 Al Viro <viro@zeniv.linux.org.uk>, Andrew Morton
 <akpm@linux-foundation.org>, Kees Cook <keescook@chromium.org>, Alex Gaynor
 <alex.gaynor@gmail.com>, Wedson Almeida Filho <wedsonaf@gmail.com>, Boqun
 Feng <boqun.feng@gmail.com>, "=?UTF-8?B?QmrDtnJu?= Roy Baron"
 <bjorn3_gh@protonmail.com>, Benno Lossin <benno.lossin@proton.me>, Andreas
 Hindborg <a.hindborg@samsung.com>, Greg Kroah-Hartman
 <gregkh@linuxfoundation.org>, "Arve =?UTF-8?B?SGrDuG5uZXbDpWc=?="
 <arve@android.com>, Todd Kjos <tkjos@android.com>, Martijn Coenen
 <maco@android.com>, Joel Fernandes <joel@joelfernandes.org>, Carlos Llamas
 <cmllamas@google.com>, Suren Baghdasaryan <surenb@google.com>, Arnd
 Bergmann <arnd@arndb.de>, Trevor Gross <tmgross@umich.edu>,
 linux-mm@kvack.org, linux-kernel@vger.kernel.org,
 rust-for-linux@vger.kernel.org, Christian Brauner <brauner@kernel.org>
Subject: Re: [PATCH v6 3/4] rust: uaccess: add typed accessors for userspace
 pointers
Message-ID: <20240425171358.54dc96e4@eugeo>
In-Reply-To: <20240418-alice-mm-v6-3-cb8f3e5d688f@google.com>
References: <20240418-alice-mm-v6-0-cb8f3e5d688f@google.com>
	<20240418-alice-mm-v6-3-cb8f3e5d688f@google.com>
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?OP/iGgRX26+mOxvzl/+tMM+FDkSPSuV+HKQ7zg/4IogwWprAI44tiuDL/TQ/?=
 =?us-ascii?Q?bFGQ3wsPFzO2V/SEGBe5bSX4HjjiUVwbQuEZUrUacm+5QtlXpR7xTnSVFdQJ?=
 =?us-ascii?Q?PG3aMD7JiONduTHYFCzxHM24+yP+hfTWYxlrX4InwDi5Isi/3SmXzDAmRkjz?=
 =?us-ascii?Q?OacLpdP5Ucz4FVasA8a9vXC/fBM38kiQbyUXFYvwAIv+lNbZuaAbiXN+vtxe?=
 =?us-ascii?Q?3cmop3V49wJvteCvVKo0kUlnz6DSdUefArEzd2nGmk8F0pwPUCO02Ma9AsZ4?=
 =?us-ascii?Q?0peOJwRwupBHHAfp662NmFMJOszyemhsaupbur0c/ObVEFsTbtmoq1ElGcvA?=
 =?us-ascii?Q?WeltL+X8/x76f/7BNPYXI61tB5I3jwYSDsVimPmfbmo2AxmNlL1F27Tp3+5q?=
 =?us-ascii?Q?xeaIBIXdhTz/5tiw9EH8Q7GQNY0I9LnLEGXPpEBwMXSUuWCrxVs7wpGXVPN+?=
 =?us-ascii?Q?z4HkUdunSra9NxV2T2igAze27NMqX//efNLJa5O3cx6NIVuWw9zaMysGlBwF?=
 =?us-ascii?Q?xXB9xP298c2R0VaHqhbIrbfw1rgXP/50JDopKWPL3I1w26Wa0LfH2ZciMAHM?=
 =?us-ascii?Q?O36EAq8enMXMCTWWQIvTsYOuUw0ISBBZapN4om9cm0/ZPxfZXL+lUZRNJ7a9?=
 =?us-ascii?Q?wcxFcKtWTdajRPQlSHo5AWIcMp9nTo/+WLPuNlWUjiYQTZ0zGy7Fu3F9xkZM?=
 =?us-ascii?Q?16zoQLEpbdM06hz0UrLs5Zb/MQfDHgR0TR9I0BG/c3wyDxkiQUffhtqXkNJo?=
 =?us-ascii?Q?cDlKvfbI7NYMiijKSwiDKtH6VBjuBe52V+F1UkjL/2PTfBWn9Wfwd7qhwlEs?=
 =?us-ascii?Q?RrB0yTx/UuNvat57W/KZD175r0GnMU69mDHidExB1XKEhOBWC72VFxfcaupn?=
 =?us-ascii?Q?nBx9JCOWlko+hWT0X7cH24eGZI3G31b0W2p3QE2bQ3q3IDsPbM4eqTlt5BCI?=
 =?us-ascii?Q?wgP4JkC9o1aJWNDic3BuEwy9nv6lSRvyb/ZDJ7iFc2iaSC/OT7T5K52NKlgn?=
 =?us-ascii?Q?XIetrz7B07AhSfxct5EmV10JP52zBCo/c/I0gY+7cyB4t1mJVmvAVpdZLvXc?=
 =?us-ascii?Q?7NuYaSq1I54d4/PwN3P9Skgm1hMh7tFn2x1kK+Je9PjxprVBtkHBVEbc/7zl?=
 =?us-ascii?Q?tyNzYkF/y3V5+VRsuAbLLgnAzZ6tlQpbAt57Bp9NQog0BFnw0FYpbbbl6vP4?=
 =?us-ascii?Q?TSUD/uunkPwX4dIHRcrXXZchVNKt7HanYEWON5cCJ43ujVMlNgJbB6Pooqm2?=
 =?us-ascii?Q?NXoSu8ufH9lq2g8aofGyV//xrrfD0Qlg8x761fVz4PQ0kOK1FfqGWD3vyybX?=
 =?us-ascii?Q?3IrsL0SQk4OTQ6BHEQ8AyU7mqlTTY7Jy3RHKcoKFF2LF5HJZq6knilmq2V32?=
 =?us-ascii?Q?wjv5/HSqe6LsjkWzc4Rv4mKm8lTj4b87Jo2in9jad61fvtoMBJ6qGkSsbouF?=
 =?us-ascii?Q?JsA8rFJrFrcsKu1UHPv+va9VryqtfZNRw32uzkmBaATPsZvH79albQ4vwf95?=
 =?us-ascii?Q?qicW6M0LhbwSGnxjzpTdb+VO0ZImONh1K10fpYBBZF6nJ3CEKvqFMAg7ZHla?=
 =?us-ascii?Q?JmnxYZHbvH19Y727+QSRrzIOesU97OUV7F04H+nH?=
X-OriginatorOrg: garyguo.net
X-MS-Exchange-CrossTenant-Network-Message-Id: 3eb642ba-7e25-4c00-4741-08dc6542b905
X-MS-Exchange-CrossTenant-AuthSource: LO2P265MB5183.GBRP265.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Apr 2024 16:14:02.7654
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bbc898ad-b10f-4e10-8552-d9377b823d45
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: eQMgL4gAAKImO8lMZuR6fOwwEVPmh8ORlY6NXvY1saT97ma12sXvs8Mro8sx4KUr11UEngJY6Bd6RNVE/mvP0w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CWLP265MB5970

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] {}