From nobody Wed Dec 17 07:08:01 2025 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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A880C4167B for ; Wed, 29 Nov 2023 13:12:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233891AbjK2NMf (ORCPT ); Wed, 29 Nov 2023 08:12:35 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39954 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233865AbjK2NMc (ORCPT ); Wed, 29 Nov 2023 08:12:32 -0500 Received: from mail-yw1-x114a.google.com (mail-yw1-x114a.google.com [IPv6:2607:f8b0:4864:20::114a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4D72010D4 for ; Wed, 29 Nov 2023 05:12:38 -0800 (PST) Received: by mail-yw1-x114a.google.com with SMTP id 00721157ae682-5caf86963ecso99698837b3.3 for ; Wed, 29 Nov 2023 05:12:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1701263557; x=1701868357; darn=vger.kernel.org; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=cXjNNXUM6pnjCl+pZVw51JBLkEWhDl2ninKP9/3jCWk=; b=oYuOv5ZjXM2gp+/9ZyBoFqjtsRiuviJsdaBfN9uJXwB4StI7ZGoTrDcMDOn2xOgSgh HNsH4oc9LS0WsSXKknQhklnYgz2UZ88tdyOkfFScREW6TzUERdcrpcbFf3cV8ZY3HXhf 7CXUw2pahhGExQF6Hbt/m1Eq7O1F51bK/nv8H/0gxCyx3f/1kH2XHHxYvTpM0qQIRrF1 SZGGd4wr3n5yO7uNGPObDqo6xYZZ+qdgN147MQ+yk6XdVbbkUIxcpRxTTxvq/w5eI1Te wADhTTSIVRetrumwd2sczVTGMcyUyfUYw9caaTi3bEDAu6Zwu7NGZlZW0yNJHTimGC95 ixEg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701263557; x=1701868357; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=cXjNNXUM6pnjCl+pZVw51JBLkEWhDl2ninKP9/3jCWk=; b=i7UG+W0upjsaW5FR32NEQJ+++8CPLGdyv5GIjk6kFRgeVHXSWlj1cLuIXWo+tr5T6E fEBLnaOGsizU7zFET3iJ0pWUEvsn8X7I3F81p+dcoX7jLdV6V+2fUOF4yMMFX7RjbRX1 syXkuZzweO76zKPE+l0CrhjoFd4u9Jukn4OgCoulUl/xM60J5nQ0McEHzZoy3ExZNa3d ecNlbnSTLl9KsYGfLTg+ZLrRUSrkCkfD/A2DEW3iq0+BNFqDsMLlqbWGT8mXNQK870da Q8SeKkiJNiffRbu46CPe/dapTBezVc5++zBtzsbXy7cTdAvVYx1C87P71czvA/RhO1N+ hVrA== X-Gm-Message-State: AOJu0YzWNpsYjPNvQCwS91IT+80U+KJIpU/A9zC5KAofe0K3SYmXMheS p+V7ojXSqrpLwRTqmingGROn22R6grGbjvQ= X-Google-Smtp-Source: AGHT+IGZ/DrVd6szNZuraGj6OJEwLI9AdSB0/YtOT+pXPvBEUA8z9WVNI5YOCH1nSunvGf16rHqPlCJprjA2dyY= X-Received: from aliceryhl2.c.googlers.com ([fda3:e722:ac3:cc00:68:949d:c0a8:572]) (user=aliceryhl job=sendgmr) by 2002:a05:690c:4283:b0:5ca:8688:a088 with SMTP id gj3-20020a05690c428300b005ca8688a088mr458584ywb.9.1701263557556; Wed, 29 Nov 2023 05:12:37 -0800 (PST) Date: Wed, 29 Nov 2023 13:12:32 +0000 In-Reply-To: <20231129-alice-file-v1-0-f81afe8c7261@google.com> Mime-Version: 1.0 References: <20231129-alice-file-v1-0-f81afe8c7261@google.com> X-Developer-Key: i=aliceryhl@google.com; a=openpgp; fpr=49F6C1FAA74960F43A5B86A1EE7A392FDE96209F X-Developer-Signature: v=1; a=openpgp-sha256; l=7762; i=aliceryhl@google.com; h=from:subject:message-id; bh=wm4v/CZhyQkEj/WgGp9wB8kmjgTXL9eNiFeuaaFW8cA=; b=owEBbQKS/ZANAwAKAQRYvu5YxjlGAcsmYgBlZzMyIu8t9+uRKkvsDVwPd7WtQJgdkH8MOJ0Kt /e6KpAU/aCJAjMEAAEKAB0WIQSDkqKUTWQHCvFIvbIEWL7uWMY5RgUCZWczMgAKCRAEWL7uWMY5 Ro5vD/wLbe54Jjz/IxjFEJCY0ktIiNyNy2iSlK3mOLv/fhrdrJHbYqss5c6VmUWo2BVovvV9ZOi 02pYVCtMyM87CZg41oeeU13aB+Loz49/Ht3Adb9Vdj2vKcStC27h+CI+2V6bqPgle81kalHY2X8 qpxHvRaaweKd1sJ59O0fOo+UOvG+fOFnA3WaVNgsRAPgNChRKMsIkj/3GlHauCNLDmghNkJv/ho qjHocDdWkJc9tVY8c0/w3EYPBafgwJiyE4kkPqRlpe3m5jFh3k3lnxZwtERUU/OE79r4y9hdPaG b5PbLvscC5kvAkB3v4oLZnZOV9OrZ3Y1oKkJHWDaJ1XLe+JyD49E3SlH1yhSufvbK+GdG4aXrva SeWHVm5sC08bY0Cb8AsI65fKo9uepAkESCSnjI5y3LpUR3Jd9ENQ/h3O7bcCcnZZwfNX73E4q9G jUmnwetQEx78xh+PgST7eZv6RFEQazDEgYsrJrrgnPpXsquMvW8vNR+jPyXUoSh7c3V7dZ3JaQ3 Zd4wHG/Ic7Urzz6+C4BSwvNL4myhbdDOLqxqHigs/4CZQJiUMSA+6T+inDgXrjtdI3RVEGKEgEt R03VOXCt0B2c3mep5G0kHp4eOwVGUp/6jbU/428pk2pK/crQwzg8fmf6dZbIkEcAtRq2lL455N6 KOu7FHxEUjoKpcQ== X-Mailer: git-send-email 2.43.0.rc1.413.gea7ed67945-goog Message-ID: <20231129-alice-file-v1-6-f81afe8c7261@google.com> Subject: [PATCH 6/7] rust: file: add `DeferredFdCloser` From: Alice Ryhl To: Miguel Ojeda , Alex Gaynor , Wedson Almeida Filho , Boqun Feng , Gary Guo , "=?utf-8?q?Bj=C3=B6rn_Roy_Baron?=" , Benno Lossin , Andreas Hindborg , Peter Zijlstra , Alexander Viro , Christian Brauner , Greg Kroah-Hartman , "=?utf-8?q?Arve_Hj=C3=B8nnev=C3=A5g?=" , Todd Kjos , Martijn Coenen , Joel Fernandes , Carlos Llamas , Suren Baghdasaryan Cc: Alice Ryhl , Dan Williams , Kees Cook , Matthew Wilcox , Thomas Gleixner , Daniel Xu , linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org, linux-fsdevel@vger.kernel.org Content-Type: text/plain; charset="utf-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: quoted-printable To close an fd from kernel space, we could call `ksys_close`. However, if we do this to an fd that is held using `fdget`, then we may trigger a use-after-free. Introduce a helper that can be used to close an fd even if the fd is currently held with `fdget`. This is done by grabbing an extra refcount to the file and dropping it in a task work once we return to userspace. This is necessary for Rust Binder because otherwise the user might try to have Binder close its fd for /dev/binder, which would cause problems as this happens inside an ioctl on /dev/binder, and ioctls hold the fd using `fdget`. Additional motivation can be found in commit 80cd795630d6 ("binder: fix use-after-free due to ksys_close() during fdget()") and in the comments on `binder_do_fd_close`. If there is some way to detect whether an fd is currently held with `fdget`, then this could be optimized to skip the allocation and task work when this is not the case. Another possible optimization would be to combine several fds into a single task work, since this is used with fd arrays that might hold several fds. That said, it might not be necessary to optimize it, because Rust Binder has two ways to send fds: BINDER_TYPE_FD and BINDER_TYPE_FDA. With BINDER_TYPE_FD, it is userspace's responsibility to close the fd, so this mechanism is used only by BINDER_TYPE_FDA, but fd arrays are used rarely these days. Signed-off-by: Alice Ryhl --- rust/bindings/bindings_helper.h | 2 + rust/helpers.c | 8 ++++ rust/kernel/file.rs | 84 +++++++++++++++++++++++++++++++++++++= +++- 3 files changed, 93 insertions(+), 1 deletion(-) diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helpe= r.h index 700f01840188..c8daee341df6 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -17,6 +18,7 @@ #include #include #include +#include #include =20 /* `bindgen` gets confused at certain things. */ diff --git a/rust/helpers.c b/rust/helpers.c index 58e3a9dff349..d146bbf25aec 100644 --- a/rust/helpers.c +++ b/rust/helpers.c @@ -32,6 +32,7 @@ #include #include #include +#include #include #include =20 @@ -243,6 +244,13 @@ void rust_helper_security_release_secctx(char *secdata= , u32 seclen) EXPORT_SYMBOL_GPL(rust_helper_security_release_secctx); #endif =20 +void rust_helper_init_task_work(struct callback_head *twork, + task_work_func_t func) +{ + init_task_work(twork, func); +} +EXPORT_SYMBOL_GPL(rust_helper_init_task_work); + /* * `bindgen` binds the C `size_t` type as the Rust `usize` type, so we can * use it in contexts where Rust expects a `usize` like slice (array) indi= ces. diff --git a/rust/kernel/file.rs b/rust/kernel/file.rs index 2186a6ea3f2f..578ee307093f 100644 --- a/rust/kernel/file.rs +++ b/rust/kernel/file.rs @@ -11,7 +11,8 @@ error::{code::*, Error, Result}, types::{ARef, AlwaysRefCounted, Opaque}, }; -use core::{marker::PhantomData, ptr}; +use alloc::boxed::Box; +use core::{alloc::AllocError, marker::PhantomData, mem, ptr}; =20 /// Flags associated with a [`File`]. pub mod flags { @@ -242,6 +243,87 @@ fn drop(&mut self) { } } =20 +/// Helper used for closing file descriptors in a way that is safe even if= the file is currently +/// held using `fdget`. +/// +/// Additional motivation can be found in commit 80cd795630d6 ("binder: fi= x use-after-free due to +/// ksys_close() during fdget()") and in the comments on `binder_do_fd_clo= se`. +pub struct DeferredFdCloser { + inner: Box, +} + +/// SAFETY: This just holds an allocation with no real content, so there's= no safety issue with +/// moving it across threads. +unsafe impl Send for DeferredFdCloser {} +unsafe impl Sync for DeferredFdCloser {} + +#[repr(C)] +struct DeferredFdCloserInner { + twork: mem::MaybeUninit, + file: *mut bindings::file, +} + +impl DeferredFdCloser { + /// Create a new [`DeferredFdCloser`]. + pub fn new() -> Result { + Ok(Self { + inner: Box::try_new(DeferredFdCloserInner { + twork: mem::MaybeUninit::uninit(), + file: core::ptr::null_mut(), + })?, + }) + } + + /// Schedule a task work that closes the file descriptor when this tas= k returns to userspace. + pub fn close_fd(mut self, fd: u32) { + use bindings::task_work_notify_mode_TWA_RESUME as TWA_RESUME; + + let file =3D unsafe { bindings::close_fd_get_file(fd) }; + if file.is_null() { + // Nothing further to do. The allocation is freed by the destr= uctor of `self.inner`. + return; + } + + self.inner.file =3D file; + + // SAFETY: Since `DeferredFdCloserInner` is `#[repr(C)]`, casting = the pointers gives a + // pointer to the `twork` field. + let inner =3D Box::into_raw(self.inner) as *mut bindings::callback= _head; + + // SAFETY: Getting a pointer to current is always safe. + let current =3D unsafe { bindings::get_current() }; + // SAFETY: The `file` pointer points at a valid file. + unsafe { bindings::get_file(file) }; + // SAFETY: Due to the above `get_file`, even if the current task h= olds an `fdget` to + // this file right now, the refcount will not drop to zero until a= fter it is released + // with `fdput`. This is because when using `fdget`, you must alwa= ys use `fdput` before + // returning to userspace, and our task work runs after any `fdget= ` users have returned + // to userspace. + // + // Note: fl_owner_t is currently a void pointer. + unsafe { bindings::filp_close(file, (*current).files as bindings::= fl_owner_t) }; + // SAFETY: The `inner` pointer is compatible with the `do_close_fd= ` method. + unsafe { bindings::init_task_work(inner, Some(Self::do_close_fd)) = }; + // SAFETY: The `inner` pointer points at a valid and fully initial= ized task work that is + // ready to be scheduled. + unsafe { bindings::task_work_add(current, inner, TWA_RESUME) }; + } + + // SAFETY: This function is an implementation detail of `close_fd`, so= its safety comments + // should be read in extension of that method. + unsafe extern "C" fn do_close_fd(inner: *mut bindings::callback_head) { + // SAFETY: In `close_fd` we use this method together with a pointe= r that originates from a + // `Box`, and we have just been given owner= ship of that allocation. + let inner =3D unsafe { Box::from_raw(inner as *mut DeferredFdClose= rInner) }; + // SAFETY: This drops a refcount we acquired in `close_fd`. Since = this callback runs in a + // task work after we return to userspace, it is guaranteed that t= he current thread doesn't + // hold this file with `fdget`, as `fdget` must be released before= returning to userspace. + unsafe { bindings::fput(inner.file) }; + // Free the allocation. + drop(inner); + } +} + /// Represents the `EBADF` error code. /// /// Used for methods that can only fail with `EBADF`. --=20 2.43.0.rc1.413.gea7ed67945-goog