On Sun, 15 Sep 2024 14:31:33 +0000 Alice Ryhl <aliceryhl@xxxxxxxxxx> 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@xxxxxxxxx> > Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@xxxxxxxxx> > Reviewed-by: Trevor Gross <tmgross@xxxxxxxxx> > Signed-off-by: Alice Ryhl <aliceryhl@xxxxxxxxxx> > --- > 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.
