Received-SPF: pass (google.com: domain of linux-kernel+bounces-148827-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240415-alice-mm-v5-0-6f55e4d8ef51@google.com>
 <20240415-alice-mm-v5-1-6f55e4d8ef51@google.com> <20240417152802.6b7a7384@eugeo>
 <CAH5fLgjcO0mrQ=X2QG1qS+sTpWDnBfGxXGWn-3wBQzn3MP8pQQ@mail.gmail.com> <b7305f21-3e0b-4bfe-8902-5fc9367c2291@proton.me>
In-Reply-To: <b7305f21-3e0b-4bfe-8902-5fc9367c2291@proton.me>
From: Alice Ryhl <aliceryhl@google.com>
Date: Wed, 17 Apr 2024 17:35:58 +0200
Message-ID: <CAH5fLgjCbvZxSnjzibiFzyEz1Qjw3bAw=qU4sjrBrgosMBKKnA@mail.gmail.com>
Subject: Re: [PATCH v5 1/4] rust: uaccess: add userspace pointers
To: Benno Lossin <benno.lossin@proton.me>
Cc: Gary Guo <gary@garyguo.net>, 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?Q?Bj=C3=B6rn_Roy_Baron?= <bjorn3_gh@protonmail.com>, 
	Andreas Hindborg <a.hindborg@samsung.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	=?UTF-8?B?QXJ2ZSBIasO4bm5ldsOlZw==?= <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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Apr 17, 2024 at 5:27=E2=80=AFPM Benno Lossin <benno.lossin@proton.m=
e> wrote:
>
> On 17.04.24 16:40, Alice Ryhl wrote:
> > On Wed, Apr 17, 2024 at 4:28=E2=80=AFPM Gary Guo <gary@garyguo.net> wro=
te:
> >>
> >> On Mon, 15 Apr 2024 07:13:53 +0000
> >> Alice Ryhl <aliceryhl@google.com> wrote:
> >>
> >>> From: Wedson Almeida Filho <wedsonaf@gmail.com>
> >>>
> >>> A pointer to an area in userspace memory, which can be either read-on=
ly
> >>> or read-write.
> >>>
> >>> All methods on this struct are safe: attempting to read or write on b=
ad
> >>> addresses (either out of the bound of the slice or unmapped addresses=
)
> >>> will return `EFAULT`. Concurrent access, *including data races to/fro=
m
> >>> userspace memory*, is permitted, because fundamentally another usersp=
ace
> >>> thread/process could always be modifying memory at the same time (in =
the
> >>> same way that userspace Rust's `std::io` permits data races with the
> >>> contents of files on disk). In the presence of a race, the exact byte
> >>> values read/written are unspecified but the operation is well-defined=
.
> >>> Kernelspace code should validate its copy of data after completing a
> >>> read, and not expect that multiple reads of the same address will ret=
urn
> >>> the same value.
> >>>
> >>> These APIs are designed to make it difficult to accidentally write
> >>> TOCTOU bugs. Every time you read from a memory location, the pointer =
is
> >>> advanced by the length so that you cannot use that reader to read the
> >>> same memory location twice. Preventing double-fetches avoids TOCTOU
> >>> bugs. This is accomplished by taking `self` by value to prevent
> >>> obtaining multiple readers on a given `UserSlicePtr`, and the readers
> >>> only permitting forward reads. If double-fetching a memory location i=
s
> >>> necessary for some reason, then that is done by creating multiple
> >>> readers to the same memory location.
> >>>
> >>> Constructing a `UserSlicePtr` performs no checks on the provided
> >>> address and length, it can safely be constructed inside a kernel thre=
ad
> >>> with no current userspace process. Reads and writes wrap the kernel A=
PIs
> >>> `copy_from_user` and `copy_to_user`, which check the memory map of th=
e
> >>> current process and enforce that the address range is within the user
> >>> range (no additional calls to `access_ok` are needed).
> >>>
> >>> This code is based on something that was originally written by Wedson=
 on
> >>> the old rust branch. It was modified by Alice by removing the
> >>> `IoBufferReader` and `IoBufferWriter` traits, and various other chang=
es.
> >>>
> >>> Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
> >>> Co-developed-by: Alice Ryhl <aliceryhl@google.com>
> >>> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> >>> ---
> >>>   rust/helpers.c         |  14 +++
> >>>   rust/kernel/lib.rs     |   1 +
> >>>   rust/kernel/uaccess.rs | 304 ++++++++++++++++++++++++++++++++++++++=
+++++++++++
> >>>   3 files changed, 319 insertions(+)
> >>>
> >>> diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
> >>
> >>> +/// [`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,
> >>> +}
> >>
> >> How useful is the `c_void` in the struct and new signature? They tend
> >> to not be very useful in Rust. Given that provenance doesn't matter
> >> for userspace pointers, could this be `usize` simply?
> >>
> >> I think `*mut u8` or `*mut ()` makes more sense than `*mut c_void` for
> >> Rust code even if we don't want to use `usize`.
> >
> > I don't have a strong opinion here. I suppose a usize could make
> > sense. But I also think c_void is fine, and I lean towards not
> > changing it. :)
> >
> >> Some thinking aloud and brainstorming bits about the API.
> >>
> >> I wonder if it make sense to have `User<[u8]>` instead of `UserSlice`?
> >> The `User` type can be defined like this:
> >>
> >> ```rust
> >> struct User<T: ?Sized> {
> >>     ptr: *mut T,
> >> }
> >> ```
> >>
> >> and this allows arbitrary T as long as it's POD. So we could have
> >> `User<[u8]>`, `User<u32>`, `User<PodStruct>`. I imagine the
> >> `User<[u8]>` would be the general usage and the latter ones can be
> >> especially helpful if you are trying to implement ioctl and need to
> >> copy fixed size data structs from userspace.
> >
> > Hmm, we have to be careful here. Generally, when you get a user slice
> > via an ioctl, you should make sure to use the length you get from
> > userspace. In binder, it looks like this:
> >
> > let user_slice =3D UserSlice::new(arg, _IOC_SIZE(cmd));
> >
> > so whichever API we use, we must make sure to get the length as an
> > argument in bytes. What should we do if the length is not a multiple
> > of size_of(T)?
>
> We could print a warning and then just floor to the next multiple of
> `size_of::<T>()`. I agree that is not perfect, but if one uses the
> current API, one also needs to do the length check eventually.

Right now, the length check happens when you call `read::<T>` and get
EFAULT if the size of T is greater than the length of the user slice.
That works pretty well. And there are real-world use-cases for
userspace passing in a length longer than what the kernel expects -
often adding fields to the end of the struct is how the kernel makes
ioctls extensible. So I don't think printing a warning in that case
would be good.

In Binder, I also have use-cases where I alternate between reading
bytes and various different structs. Basically, I read two user slices
in lockstep, where the next value in one userslice determines whether
I should read some amount of bytes or a specific struct from the other
user slice. That's much easier with the current API than this
proposal.

> > Another issue is that there's no stable way to get the length from a
> > `*mut [T]` without creating a reference, which is not okay for a user
> > slice.
>
> Seems like `<* const [T]>::len` (feature `slice_ptr_len`) [1] was just
> stabilized 5 days ago [1].
>
> [1]: https://doc.rust-lang.org/std/primitive.pointer.html#method.len-1
> [2]: https://github.com/rust-lang/rust/pull/123868

Okay.

Alice