[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240915230211.420f48a9.gary@garyguo.net>
Date: Sun, 15 Sep 2024 23:02:11 +0100
From: Gary Guo <gary@...yguo.net>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Paul Moore <paul@...l-moore.com>, James Morris <jmorris@...ei.org>,
"Serge E. Hallyn" <serge@...lyn.com>, Miguel Ojeda <ojeda@...nel.org>,
Christian Brauner <brauner@...nel.org>, Alex Gaynor
<alex.gaynor@...il.com>, Wedson Almeida Filho <wedsonaf@...il.com>, Boqun
Feng <boqun.feng@...il.com>, "Björn Roy Baron"
<bjorn3_gh@...tonmail.com>, Benno Lossin <benno.lossin@...ton.me>, Andreas
Hindborg <a.hindborg@...sung.com>, Peter Zijlstra <peterz@...radead.org>,
Alexander Viro <viro@...iv.linux.org.uk>, Greg Kroah-Hartman
<gregkh@...uxfoundation.org>, "Arve Hjønnevåg"
<arve@...roid.com>, Todd Kjos <tkjos@...roid.com>, Martijn Coenen
<maco@...roid.com>, Joel Fernandes <joel@...lfernandes.org>, Carlos Llamas
<cmllamas@...gle.com>, Suren Baghdasaryan <surenb@...gle.com>, Dan Williams
<dan.j.williams@...el.com>, Matthew Wilcox <willy@...radead.org>, Thomas
Gleixner <tglx@...utronix.de>, Daniel Xu <dxu@...uu.xyz>, Martin Rodriguez
Reboredo <yakoyoku@...il.com>, Trevor Gross <tmgross@...ch.edu>,
linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org,
rust-for-linux@...r.kernel.org, linux-fsdevel@...r.kernel.org, Kees Cook
<kees@...nel.org>
Subject: Re: [PATCH v10 7/8] rust: file: add `Kuid` wrapper
On Sun, 15 Sep 2024 14:31:33 +0000
Alice Ryhl <aliceryhl@...gle.com> wrote:
> Adds a wrapper around `kuid_t` called `Kuid`. This allows us to define
> various operations on kuids such as equality and current_euid. It also
> lets us provide conversions from kuid into userspace values.
>
> Rust Binder needs these operations because it needs to compare kuids for
> equality, and it needs to tell userspace about the pid and uid of
> incoming transactions.
>
> To read kuids from a `struct task_struct`, you must currently use
> various #defines that perform the appropriate field access under an RCU
> read lock. Currently, we do not have a Rust wrapper for rcu_read_lock,
> which means that for this patch, there are two ways forward:
>
> 1. Inline the methods into Rust code, and use __rcu_read_lock directly
> rather than the rcu_read_lock wrapper. This gives up lockdep for
> these usages of RCU.
>
> 2. Wrap the various #defines in helpers and call the helpers from Rust.
>
> This patch uses the second option. One possible disadvantage of the
> second option is the possible introduction of speculation gadgets, but
> as discussed in [1], the risk appears to be acceptable.
>
> Of course, once a wrapper for rcu_read_lock is available, it is
> preferable to use that over either of the two above approaches.
>
> Link: https://lore.kernel.org/all/202312080947.674CD2DC7@keescook/ [1]
> Reviewed-by: Benno Lossin <benno.lossin@...ton.me>
> Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@...il.com>
> Reviewed-by: Trevor Gross <tmgross@...ch.edu>
> Signed-off-by: Alice Ryhl <aliceryhl@...gle.com>
> ---
> rust/bindings/bindings_helper.h | 1 +
> rust/helpers/task.c | 38 ++++++++++++++++++++++++
> rust/kernel/cred.rs | 5 ++--
> rust/kernel/task.rs | 66 +++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 108 insertions(+), 2 deletions(-)
>
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 51ec78c355c0..e854ccddecee 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -19,6 +19,7 @@
> #include <linux/jiffies.h>
> #include <linux/mdio.h>
> #include <linux/phy.h>
> +#include <linux/pid_namespace.h>
> #include <linux/refcount.h>
> #include <linux/sched.h>
> #include <linux/security.h>
> diff --git a/rust/helpers/task.c b/rust/helpers/task.c
> index 7ac789232d11..7d66487db831 100644
> --- a/rust/helpers/task.c
> +++ b/rust/helpers/task.c
> @@ -17,3 +17,41 @@ void rust_helper_put_task_struct(struct task_struct *t)
> {
> put_task_struct(t);
> }
> +
> +kuid_t rust_helper_task_uid(struct task_struct *task)
> +{
> + return task_uid(task);
> +}
> +
> +kuid_t rust_helper_task_euid(struct task_struct *task)
> +{
> + return task_euid(task);
> +}
> +
> +#ifndef CONFIG_USER_NS
> +uid_t rust_helper_from_kuid(struct user_namespace *to, kuid_t uid)
> +{
> + return from_kuid(to, uid);
> +}
> +#endif /* CONFIG_USER_NS */
nit: it's fine to omit this `ifndef`, see what we do for `errname`.
> +
> +bool rust_helper_uid_eq(kuid_t left, kuid_t right)
> +{
> + return uid_eq(left, right);
> +}
> +
> +kuid_t rust_helper_current_euid(void)
> +{
> + return current_euid();
> +}
> +
> +struct user_namespace *rust_helper_current_user_ns(void)
> +{
> + return current_user_ns();
> +}
> +
> +pid_t rust_helper_task_tgid_nr_ns(struct task_struct *tsk,
> + struct pid_namespace *ns)
> +{
> + return task_tgid_nr_ns(tsk, ns);
> +}
> diff --git a/rust/kernel/task.rs b/rust/kernel/task.rs
> index 367b4bbddd9f..1a36a9f19368 100644
> --- a/rust/kernel/task.rs
> +++ b/rust/kernel/task.rs
> @@ -9,6 +9,7 @@
> types::{NotThreadSafe, Opaque},
> };
> use core::{
> + cmp::{Eq, PartialEq},
> ffi::{c_int, c_long, c_uint},
> ops::Deref,
> ptr,
> @@ -96,6 +97,12 @@ unsafe impl Sync for Task {}
> /// The type of process identifiers (PIDs).
> type Pid = bindings::pid_t;
>
> +/// The type of user identifiers (UIDs).
> +#[derive(Copy, Clone)]
> +pub struct Kuid {
> + kuid: bindings::kuid_t,
> +}
> +
> impl Task {
> /// Returns a raw pointer to the current task.
> ///
> @@ -157,12 +164,31 @@ pub fn pid(&self) -> Pid {
> unsafe { *ptr::addr_of!((*self.0.get()).pid) }
> }
>
> + /// Returns the UID of the given task.
> + pub fn uid(&self) -> Kuid {
> + // SAFETY: By the type invariant, we know that `self.0` is valid.
> + Kuid::from_raw(unsafe { bindings::task_uid(self.0.get()) })
> + }
> +
> + /// Returns the effective UID of the given task.
> + pub fn euid(&self) -> Kuid {
> + // SAFETY: By the type invariant, we know that `self.0` is valid.
> + Kuid::from_raw(unsafe { bindings::task_euid(self.0.get()) })
> + }
> +
> /// Determines whether the given task has pending signals.
> pub fn signal_pending(&self) -> bool {
> // SAFETY: By the type invariant, we know that `self.0` is valid.
> unsafe { bindings::signal_pending(self.0.get()) != 0 }
> }
>
> + /// Returns the given task's pid in the current pid namespace.
> + pub fn pid_in_current_ns(&self) -> Pid {
> + // SAFETY: We know that `self.0.get()` is valid by the type invariant, and passing a null
> + // pointer as the namespace is correct for using the current namespace.
> + unsafe { bindings::task_tgid_nr_ns(self.0.get(), ptr::null_mut()) }
Do we want to rely on the behaviour of `task_tgid_nr_ns` with null
pointer as namespace, or use `task_tgid_vnr`?
Best,
Gary
> + }
> +
> /// Wakes up the task.
> pub fn wake_up(&self) {
> // SAFETY: By the type invariant, we know that `self.0.get()` is non-null and valid.
Powered by blists - more mailing lists