From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id BAA51C4345F for ; Wed, 17 Apr 2024 15:27:18 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3EAA76B008C; Wed, 17 Apr 2024 11:27:18 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 399386B0092; Wed, 17 Apr 2024 11:27:18 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 261DF6B0095; Wed, 17 Apr 2024 11:27:18 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 03A546B0092 for ; Wed, 17 Apr 2024 11:27:17 -0400 (EDT) Received: from smtpin30.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id BF4C5A0E16 for ; Wed, 17 Apr 2024 15:27:17 +0000 (UTC) X-FDA: 82019402514.30.99D13FF Received: from mail-4316.protonmail.ch (mail-4316.protonmail.ch [185.70.43.16]) by imf09.hostedemail.com (Postfix) with ESMTP id 03DC314001C for ; Wed, 17 Apr 2024 15:27:14 +0000 (UTC) Authentication-Results: imf09.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b=SEuodbfR; spf=pass (imf09.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.43.16 as permitted sender) smtp.mailfrom=benno.lossin@proton.me; dmarc=pass (policy=quarantine) header.from=proton.me ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713367635; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=yE1RsTT/3QhtCokhgtHPH8FIR5On7XGmjEDodbAFAQI=; b=4sNzUU3drvgOgBmcs9v8dAl7lcBEsDdxyEmivV6c00T/6/+2U09PTuKElYLoE9cGGelJ+u 3G2EOMG9gk0ZetWcKuQq+l6k3YHoTQ00K9QshfSNEppU1crPSubIpvA9FN/fzR14A4i/2V 5/GdnZqzl49VXdLu66GgSNsiYSOVFdI= ARC-Authentication-Results: i=1; imf09.hostedemail.com; dkim=pass header.d=proton.me header.s=protonmail header.b=SEuodbfR; spf=pass (imf09.hostedemail.com: domain of benno.lossin@proton.me designates 185.70.43.16 as permitted sender) smtp.mailfrom=benno.lossin@proton.me; dmarc=pass (policy=quarantine) header.from=proton.me ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713367635; a=rsa-sha256; cv=none; b=7VxuBd7tDw2PMjbvZMRRew3zYF5oUha++vLW/ci5xNow71ahNHaw7+/ihuA167GPugY54V TADPUAShYk/eChjFeRd14/cuSC0Gz3vlf50snilTsbhUp3uMJlwBVAHYI03WMLpYuSRhuy sZ2I6pIYSMCkC5aac/8xz1Trs0TB9DI= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=proton.me; s=protonmail; t=1713367632; x=1713626832; bh=yE1RsTT/3QhtCokhgtHPH8FIR5On7XGmjEDodbAFAQI=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector; b=SEuodbfR8tDN1iztrdISMh608nxV7gNjcXC3IkbviYQDqAG/K7kbEYXVYFyVpsZED oTSH49hr/DqRrbM+whOMt8FqBzDbVrhicQLAQ7v3k5ZBc99gTS4EaNQRkemL8/nsRD eEM1WDBbocmVXAvCYGqjAZe4/gqJx0o9XcUfkwS7MqgRJsfb9iO1HHCZjR0/OBu422 vTFuAD3XuK/TnkqCwLlu0OUdK/QlEdYjy3hWUjgH27rf87Ma10qYMRAaW9TXb5QzZ5 CN3DYniWeLD6vNaOPDLllM7o6ZmTPkA2rfgRtGipuPZkhwu/sF/4qlg6Phoqn4Jv2y Aaw9y+q8TerZg== Date: Wed, 17 Apr 2024 15:27:02 +0000 To: Alice Ryhl , Gary Guo From: Benno Lossin Cc: Miguel Ojeda , Matthew Wilcox , Al Viro , Andrew Morton , Kees Cook , Alex Gaynor , Wedson Almeida Filho , Boqun Feng , =?utf-8?Q?Bj=C3=B6rn_Roy_Baron?= , Andreas Hindborg , Greg Kroah-Hartman , =?utf-8?Q?Arve_Hj=C3=B8nnev=C3=A5g?= , Todd Kjos , Martijn Coenen , Joel Fernandes , Carlos Llamas , Suren Baghdasaryan , Arnd Bergmann , linux-mm@kvack.org, linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org, Christian Brauner Subject: Re: [PATCH v5 1/4] rust: uaccess: add userspace pointers Message-ID: In-Reply-To: References: <20240415-alice-mm-v5-0-6f55e4d8ef51@google.com> <20240415-alice-mm-v5-1-6f55e4d8ef51@google.com> <20240417152802.6b7a7384@eugeo> Feedback-ID: 71780778:user:proton MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Stat-Signature: k9aqsn1w4hh6qb9p1b84m6m39zd3xdma X-Rspamd-Queue-Id: 03DC314001C X-Rspamd-Server: rspam10 X-Rspam-User: X-HE-Tag: 1713367634-458675 X-HE-Meta: U2FsdGVkX194NtoWGFB/Ew7B9nTim5lBbvXM6vBBSkk9duopvDQaJdrkfq9Whjo556i+DBMseT421hRyOFPLhxt6sZjIkcLTUm/kDVoflps8/oLb/jWNJgWRuDNHZqhueNuP2JMT1GdFgfVTBfBTm073G7XGHKuZnADWF+F4cQF1Ds2c6wRVZyt9iKgO0FyND7mg3aLD2mYHaVfqAtgCbGexdjzdeAIJkcK5NRqbD1AGQNdrWsyy1FCiNmFvSGGPYZBSrxEqhNCjn5rfMneBj7RuOIhIgTFtLEVidEFpv8O5jZDrBtTP38Z+6dUexs0pJ4HwqFYjeFROljjbNbS+00P3ULs3BXnCkoUJk2PGh9OqewOI8Wi0Y710md+6wZNyE5NkAWbwaex6frsViMWb/rcyxOfSAv1FkMuYnrLjbc6+slh8HTTrMWtM6XZ7mqmcbXW02YovBhpfswma2zrjJKXy1EWdBJLwIW0VToabW8789eNxcvN6vdMVv4kbwKs2Zq4EpeDoIt+K5kP9ZBqP7YmzxTgt1LgcNIjOZ2g1Ck53uz0jHHAWcOB2W0yj7qT2AP04bom0QFvPPCoqQicdFEsutdGu5PS5TtBe9fhWbkYWUgId/7PkI3BdGqcloNUJfOkqFQ0CU/qBbibmTjR0gVYRUkMX8LURVejbxfKcNJLmowCKKo+0vIkxXJQQ5hQ45FJAU7qNveo5P4L9tOGwK2gbRNLVtlHfVveDIj5IOovu/2IVCsS828NhH9I8kqSDS/am4w83ISMYSVux/KvVqq7XysvuqEPQTXg8q2cnulWpsugqdDIYeDprTUS+GdRg00SrFvdrOiFm8fwfkz6Onbt2nBKEA+719+L4e1vSopJnnC8xw1GmmU8K4fELK5UvK3+xGTk0cA98pzfebhx3tQ7zzzXALCnHb80soCzVwOwu7qB+NmD0QFPTVoO8NLKOQB1LDapQfrkTlBoGPgS cIhZKciV CEdJmHHj3RRkQd2tPAfdrKuZpxy0Ic8XaSPbF5p3LL2kqh8IMRG77lCAMQNRICqUHjWPnGJPj1S3ak0xDSlM7+WZdz8Fn+Z8KMM7X6joleeVFBHLb7HgzgoWJNEbJR/ZHRRKxw7pXu0Wj0fefHLWUeMJ6yaafmBePhl9bXOOCiMpyGz2vtU9XFAH2sMeY1aNe0WJbO4R5H/v9CYZjUnHZZYXe2lAgVsCkoljXBKrHlmxXEo24nh4nNkgv2VD5t5YO4Z0STVsspYW52kxHKsTlLmypV2TV2s3Gpm4dGTX0VHKx/4cL8G6f1mJJTfMq/rE65cSGibIn5DyMSJdp7DQwbv/NlakSBy0TVUgoJ4HoEaGnz5t4I1fq8QhZxi8Sng1c8XAf5WGu2qlNkPzaFxPIxbbYTcMUJcE//eabbQzG4kKID5kUZnjjXHQdyXEHo504MC+zBeH52kcjon5bftVGiDZwPh0r7zgulPrNBNe9jB60ZlIclhJo6vfFGWFVshZocZNwuFuDG2T1pnTWlAunJ0XE+0pka8E16fwC X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 17.04.24 16:40, Alice Ryhl wrote: > On Wed, Apr 17, 2024 at 4:28=E2=80=AFPM Gary Guo wrote= : >> >> On Mon, 15 Apr 2024 07:13:53 +0000 >> Alice Ryhl wrote: >> >>> From: Wedson Almeida Filho >>> >>> A pointer to an area in userspace memory, which can be either read-only >>> or read-write. >>> >>> All methods on this struct are safe: attempting to read or write on bad >>> addresses (either out of the bound of the slice or unmapped addresses) >>> will return `EFAULT`. Concurrent access, *including data races to/from >>> userspace memory*, is permitted, because fundamentally another userspac= e >>> thread/process could always be modifying memory at the same time (in th= e >>> 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 retur= n >>> 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 is >>> 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 thread >>> with no current userspace process. Reads and writes wrap the kernel API= s >>> `copy_from_user` and `copy_to_user`, which check the memory map of the >>> 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 o= n >>> the old rust branch. It was modified by Alice by removing the >>> `IoBufferReader` and `IoBufferWriter` traits, and various other changes= . >>> >>> Signed-off-by: Wedson Almeida Filho >>> Co-developed-by: Alice Ryhl >>> Signed-off-by: Alice Ryhl >>> --- >>> 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`. >=20 > 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. :) >=20 >> 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 { >> ptr: *mut T, >> } >> ``` >> >> and this allows arbitrary T as long as it's POD. So we could have >> `User<[u8]>`, `User`, `User`. 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. >=20 > 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: >=20 > let user_slice =3D UserSlice::new(arg, _IOC_SIZE(cmd)); >=20 > 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::()`. I agree that is not perfect, but if one uses the current API, one also needs to do the length check eventually. > 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 --=20 Cheers, Benno