Received-SPF: pass (google.com: domain of linux-kernel+bounces-109301-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Feedback-ID: iad51458e:Fastmail
Date: Wed, 20 Mar 2024 11:39:33 -0700
From: Boqun Feng <boqun.feng@gmail.com>
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>,
	Gary Guo <gary@garyguo.net>,
	=?iso-8859-1?Q?Bj=F6rn?= 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 =?iso-8859-1?B?SGr4bm5lduVn?= <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>, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org,
	Christian Brauner <brauner@kernel.org>
Subject: Re: [PATCH v3 1/4] rust: uaccess: add userspace pointers
Message-ID: <ZfstZZbagFLj7dqq@boqun-archlinux>
References: <20240311-alice-mm-v3-0-cdf7b3a2049c@google.com>
 <20240311-alice-mm-v3-1-cdf7b3a2049c@google.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240311-alice-mm-v3-1-cdf7b3a2049c@google.com>

On Mon, Mar 11, 2024 at 10:47:13AM +0000, Alice Ryhl wrote:
> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> 
[...]
> +/// # Examples
> +///
> +/// Takes a region of userspace memory from the current process, and modify it
> +/// by adding one to every byte in the region.
> +///
> +/// ```no_run
> +/// use alloc::vec::Vec;
> +/// use core::ffi::c_void;
> +/// use kernel::error::Result;
> +/// use kernel::uaccess::UserSlice;
> +///
> +/// pub fn bytes_add_one(uptr: *mut c_void, len: usize) -> Result<()> {

I hit the following compile error when trying to run kunit test:

	ERROR:root:error: unreachable `pub` item
	    --> rust/doctests_kernel_generated.rs:4167:1
	     |
	4167 | pub fn bytes_add_one(uptr: *mut c_void, len: usize) -> Result<()> {
	     | ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	     | |
	     | help: consider restricting its visibility: `pub(crate)`
	     |
	     = help: or consider exporting it for use by other crates
	     = note: requested on the command line with `-D unreachable-pub`

	error: unreachable `pub` item
	    --> rust/doctests_kernel_generated.rs:4243:1
	     |
	4243 | pub fn get_bytes_if_valid(uptr: *mut c_void, len: usize) -> Result<Vec<u8>> {
	     | ---^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	     | |
	     | help: consider restricting its visibility: `pub(crate)`
	     |
	     = help: or consider exporting it for use by other crates

	error: aborting due to 2 previous errors

, which should be fixed if we make the function in the example not
`pub`.

> +///     let (read, mut write) = UserSlice::new(uptr, len).reader_writer();
> +///
> +///     let mut buf = Vec::new();
> +///     read.read_all(&mut buf)?;
> +///
> +///     for b in &mut buf {
> +///         *b = b.wrapping_add(1);
> +///     }
> +///
> +///     write.write_slice(&buf)?;
> +///     Ok(())
> +/// }
> +/// ```
> +///
> +/// Example illustrating a TOCTOU (time-of-check to time-of-use) bug.
> +///
> +/// ```no_run
> +/// use alloc::vec::Vec;
> +/// use core::ffi::c_void;
> +/// use kernel::error::{code::EINVAL, Result};
> +/// use kernel::uaccess::UserSlice;
> +///
> +/// /// Returns whether the data in this region is valid.
> +/// fn is_valid(uptr: *mut c_void, len: usize) -> Result<bool> {
> +///     let read = UserSlice::new(uptr, len).reader();
> +///
> +///     let mut buf = Vec::new();
> +///     read.read_all(&mut buf)?;
> +///
> +///     todo!()
> +/// }
> +///
> +/// /// Returns the bytes behind this user pointer if they are valid.
> +/// pub fn get_bytes_if_valid(uptr: *mut c_void, len: usize) -> Result<Vec<u8>> {

Ditto here.

> +///     if !is_valid(uptr, len)? {
> +///         return Err(EINVAL);
> +///     }
> +///
> +///     let read = UserSlice::new(uptr, len).reader();
> +///
> +///     let mut buf = Vec::new();
> +///     read.read_all(&mut buf)?;
> +///
> +///     // THIS IS A BUG! The bytes could have changed since we checked them.
> +///     //
> +///     // To avoid this kind of bug, don't call `UserSlice::new` multiple
> +///     // times with the same address.
> +///     Ok(buf)
> +/// }
> +/// ```
> +///
> +/// [`std::io`]: https://doc.rust-lang.org/std/io/index.html
> +/// [`clone_reader`]: UserSliceReader::clone_reader
> +pub struct UserSlice {
> +    ptr: *mut c_void,
> +    length: usize,
> +}
> +

Regards,
Boqun

[...]