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 DCC00C4345F for ; Wed, 17 Apr 2024 15:36:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 3CD886B0089; Wed, 17 Apr 2024 11:36:14 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 355FE6B008A; Wed, 17 Apr 2024 11:36:14 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1F6896B0092; Wed, 17 Apr 2024 11:36:14 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id F1CB46B0089 for ; Wed, 17 Apr 2024 11:36:13 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id B076C120EB6 for ; Wed, 17 Apr 2024 15:36:13 +0000 (UTC) X-FDA: 82019425026.04.BBB79D5 Received: from mail-oo1-f47.google.com (mail-oo1-f47.google.com [209.85.161.47]) by imf25.hostedemail.com (Postfix) with ESMTP id B6625A001A for ; Wed, 17 Apr 2024 15:36:11 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=zd7qvY1t; spf=pass (imf25.hostedemail.com: domain of aliceryhl@google.com designates 209.85.161.47 as permitted sender) smtp.mailfrom=aliceryhl@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1713368171; 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=7STbkb0kc4uO9A+FOCYz3iBH+d6D+yCiN6yb3DAcBlQ=; b=v4X2Wqj30FtZDhwtYr0mszDA1l5NtUJZ1x8qduUsR4Onrj3j0w+uagdCqHQmZAHLn6bmhM hef6MgVUoiKdA7KDLqXZ0FMPuCFJGjbjQsEg1qA1Bg56KvhJBnu8In9+7X/C9pZVhS1AEb xlNnzAesbDfy3b77TLi4zGVVbpyq24g= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1713368171; a=rsa-sha256; cv=none; b=lvj/I1YXnszBX4GkE4h2FB26ayeqaxCL7mX8NkjKQgAGpKgFF3h2g1iklkH+KNIR5DGB99 jKz3zUIeXe5zljBp7Lg00CcrZkF3zmFzRze+HRdwSmYzNE1kBxpFdqt3+UbtiJA+i4f8IN i6g6u7v5abIEuoGiz4yGTN0LQn0jElw= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=zd7qvY1t; spf=pass (imf25.hostedemail.com: domain of aliceryhl@google.com designates 209.85.161.47 as permitted sender) smtp.mailfrom=aliceryhl@google.com; dmarc=pass (policy=reject) header.from=google.com Received: by mail-oo1-f47.google.com with SMTP id 006d021491bc7-5aa25e99414so3836665eaf.3 for ; Wed, 17 Apr 2024 08:36:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1713368170; x=1713972970; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=7STbkb0kc4uO9A+FOCYz3iBH+d6D+yCiN6yb3DAcBlQ=; b=zd7qvY1tJXs5Uao9T1RgHgSuWC8XIwa9r3cFOc/g+chuKnIRVl46R+dKTMGQD0ULcJ Eb4iKIE7kHt+0Rl1hBxownwa4H/8enZF7ziANZe55m5CGQwUdT8CporCHsBrtpKXgcs8 58q+uCnR/MA2wr4l4C4e7j2mgCh6n7+3pjYyHZ8R5d5OeZy0ycFZ7QC8d5uvrpYT4uZC yfxRCNpPJWanH2SWRIw3rbJo/NC8e33IC8Xeq25j3GUcak4f3VXac10K7x7j6VvbSYRD XOnZ3e00c+2Rtm1Mg+TGoYa6DA04f17ZGsRAcF6y/buvM3MFpNzGfFEmDD3w3/YLcr8B 05jg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1713368170; x=1713972970; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=7STbkb0kc4uO9A+FOCYz3iBH+d6D+yCiN6yb3DAcBlQ=; b=mtWtF1Cs9+XDr74VrgDlqfNK1QYvZlACrGmgUuJwROlpcf6hnMSVahno2jw21HVOl9 1f37V0DPcVfTwlEmfMiWDi2GLachSs7IQczZx0Okmc4caRxtwpxRYWmxT9hH2evEtG+S o8GPk+eYFuR5Qpkb/QC79iZUq/fVWU6n40+nQL3maX4R2uHg1bAdcUrmj55WoXiSMNjh n+pzmnAzSKYrh1qpvDmigFUwNf/G5IjWIZlucfaHT1/ZvqXHZSLOVAHhA0as/PWCgQg+ Fb0aeZ2EglxdkD65CEGFTxSBxZKCtf8Bt3XRwAZ09nCOva0/pEu7rboaRV/4pLcrK6Ut 29Mg== X-Forwarded-Encrypted: i=1; AJvYcCUMnPGSxDQOTeKNSjqW69bAEUUHdJ9sujzh9umr7T1qVQOPtON/iQGfTIMmetUEcPvrKfImEvgH8hgmLuqkUA+sAHs= X-Gm-Message-State: AOJu0Yyq9qqApBDk7xpV7vEj7Kl9mscwZr7Poy34krPsf+VddVC8wIiS ttENi5HraSgYIL8MerEh4nWMJ8QWiOK7N54vXsN/f2af/WBA8tZYpJmoLlTYJE0FSMe90BzYBMn UbvWi1LAVPPN82BGRNWETExpD1XFg+2PFj/+c X-Google-Smtp-Source: AGHT+IEMzi3flnGGABxiUZthG7GHk4bJeCL8d7G0v1u9soflo+40Qg2LO67Au8RewgmGKPOf25nQ1dU2JqYa7YwdIKc= X-Received: by 2002:a05:6358:688b:b0:186:2720:2122 with SMTP id z11-20020a056358688b00b0018627202122mr13281274rwh.2.1713368170317; Wed, 17 Apr 2024 08:36:10 -0700 (PDT) MIME-Version: 1.0 References: <20240415-alice-mm-v5-0-6f55e4d8ef51@google.com> <20240415-alice-mm-v5-1-6f55e4d8ef51@google.com> <20240417152802.6b7a7384@eugeo> In-Reply-To: From: Alice Ryhl Date: Wed, 17 Apr 2024 17:35:58 +0200 Message-ID: Subject: Re: [PATCH v5 1/4] rust: uaccess: add userspace pointers To: Benno Lossin Cc: Gary Guo , 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?B?QXJ2ZSBIasO4bm5ldsOlZw==?= , 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 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: B6625A001A X-Stat-Signature: a8xn94tg65tgn3w841ke5bie59wbxxot X-Rspam-User: X-HE-Tag: 1713368171-423289 X-HE-Meta: U2FsdGVkX19tQFcd4fpXicK0u6ZiMbkHL/cnk+LUxyCGRxP9Dhi46Gl1rvr2faHOy2AF3iaLhvMLpOucXUprM9EE6+pSfnCYkVJc4sigT7G8IIPFdHuf/hWbYyVgXxp1GIzpeUQZJjKlJT6HhYJIMWUkZTxCki33KtgsHNfeHB/XKr9rOenQromRUH79p24+x+c6VQ06pXo2kBHk/T3xaVeZK+IRohZZ+K4Kw80alUdVEdXTDjxrHnPSCXb0+oR0Xk3i8+fSlrkszYpnEog+ZqUllbprlC/CGZf/6/lil8q7wP7quFPhETasZeV3KN7SzIlQAvadDSQFfs4deER4LXAXtbB+G9gwU8fChYvy+MqmdfHIrqz8HPw5dGF6MzuUnW+PNkqny7guot46cEHvw2GIgA0sI6sDGa0vfGuUCvMXGFCU0sSno657p8zvii6uZvSCuFgvntJ07yiMUFPi8dUKKhrzc72JM4FjCwMXZd7Z2IiNuoZ7BG+In3/694gQHs/qkGsiuzVlwZAOLVz2vKzOIWqwGjh/MNl0/6hGlywpPyqtWCuS6x+eBG7t10nfLLWz5AFPWO3H5eHbRjKTbDuisNUCDzPu89VAAspQXX6lVywqTCmhR32f5YohGwNLsvUqTJRFYqHD/nyJA1uw0mMMu4oEWD3qFEYL8akMOMZZ6uAAF17kHUuo9ChYxpJ9182uP27MQrxED0TdLI/955p0fbAtaIsDVElxcwXTWievMNzizZESKz4c6173DUNJUf4soubD214hDfSRgnB69ulZ+BsIkLXs3IJ9tAiXVQOtAzgQvmjsuiL48Lj2wEubuomXEBogWkdCwBaeGXkrut6Q+F5tloZ1Fbw5FEx3kTq8BeCnQdH733lge1ajHClufRDf97/TB3IHIXflz79IDpRTG7SN+pCA4Y3VSigE010aML/8Pl8mogJBZan5GlOIIDUX3ZuF+GI0yRnXMbM v03NlUbG z+JPY3b/0l7BTlvL3wHCqje4pB8qH9gITXR0d9n3o0FCt9G/0UfRaWEQ7C6ZvSVaRHxDhcfic13Ph3FtqgptK2PN7ifwkLogclc84bBNeT3EM8W2ssJ2yqJZtrRccS0YO9DWg87sAjfgyBpNj3ba6S4vo3rPa+WZPxpsUDjaukC0fZz7Jj9iEY4Pbw76u6lNo8xexTS+tLGMyeZiiRGC468B9ZjSzh22tpTEMlD7IdABmqqlZeaSksbGHja/r3qUOam4WisnABhX2Vk2UOy5yrp6PYrxXcEsoTJ5XucXUVaxle0N2uqO/XwReg2JLzPtNWEif5QuUmDN+ojCASGd4eviyGykOzabFvr4dp1wt2MOQRd50/mR5z0EClCfruF5ZpQA0ixxYCuDWjZp0ziIlH7gKxtgMRiG88aoPOGYNvQMTG4SjuJMJuu9RQnmP7mdzXLGgXSkIEmz/V+EgM8izpWSa0rAbjiE3fFt5QRmno/Iw1d0YxbuMz0vDY0dC6X28qWtQ 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 Wed, Apr 17, 2024 at 5:27=E2=80=AFPM Benno Lossin wrote: > > On 17.04.24 16:40, Alice Ryhl wrote: > > On Wed, Apr 17, 2024 at 4:28=E2=80=AFPM Gary Guo wro= te: > >> > >> 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-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 > >>> 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`. > > > > 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 { > >> 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. > > > > 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::()`. 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::` 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