diff mbox series

[v2,2/7] rust: cred: add Rust abstraction for `struct cred`

Message ID 20231206-alice-file-v2-2-af617c0d9d94@google.com (mailing list archive)
State New, archived
Headers show
Series File abstractions needed by Rust Binder | expand

Commit Message

Alice Ryhl Dec. 6, 2023, 11:59 a.m. UTC
From: Wedson Almeida Filho <wedsonaf@gmail.com>

Add a wrapper around `struct cred` called `Credential`, and provide
functionality to get the `Credential` associated with a `File`.

Rust Binder must check the credentials of processes when they attempt to
perform various operations, and these checks usually take a
`&Credential` as parameter. The security_binder_set_context_mgr function
would be one example. This patch is necessary to access these security_*
methods from Rust.

Signed-off-by: Wedson Almeida Filho <wedsonaf@gmail.com>
Co-developed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/helpers.c                  | 13 +++++++++
 rust/kernel/cred.rs             | 64 +++++++++++++++++++++++++++++++++++++++++
 rust/kernel/file.rs             | 16 +++++++++++
 rust/kernel/lib.rs              |  1 +
 5 files changed, 95 insertions(+)

Comments

Benno Lossin Dec. 8, 2023, 4:13 p.m. UTC | #1
On 12/6/23 12:59, Alice Ryhl wrote:
> +/// Wraps the kernel's `struct cred`.
> +///
> +/// # Invariants
> +///
> +/// Instances of this type are always ref-counted, that is, a call to `get_cred` ensures that the
> +/// allocation remains valid at least until the matching call to `put_cred`.
> +#[repr(transparent)]
> +pub struct Credential(pub(crate) Opaque<bindings::cred>);

Why is the field `pub(crate)`?

[...]

> +// SAFETY: The type invariants guarantee that `Credential` is always ref-counted.
> +unsafe impl AlwaysRefCounted for Credential {
> +    fn inc_ref(&self) {
> +        // SAFETY: The existence of a shared reference means that the refcount is nonzero.
> +        unsafe { bindings::get_cred(self.0.get()) };
> +    }
> +
> +    unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) {
> +        // SAFETY: The safety requirements guarantee that the refcount is nonzero.

Can you also justify the `cast()`?
Boqun Feng Dec. 11, 2023, 1:19 a.m. UTC | #2
On Wed, Dec 06, 2023 at 11:59:47AM +0000, Alice Ryhl wrote:
[...]
> @@ -151,6 +152,21 @@ pub fn as_ptr(&self) -> *mut bindings::file {
>          self.0.get()
>      }
>  
> +    /// Returns the credentials of the task that originally opened the file.
> +    pub fn cred(&self) -> &Credential {

I wonder whether it would be helpful if we use explicit lifetime here:

    pub fn cred<'file>(&'file self) -> &'file Credential

It might be easier for people to get. For example, the lifetime of the
returned Credential reference is constrainted by 'file, the lifetime of
the file reference.

But yes, maybe need to hear others' feedback first.

Regards,
Boqun

> +        // SAFETY: Since the caller holds a reference to the file, it is guaranteed that its
> +        // refcount does not hit zero during this function call.
> +        //
> +        // It's okay to read the `f_cred` field without synchronization as `f_cred` is never
> +        // changed after initialization of the file.
> +        let ptr = unsafe { (*self.as_ptr()).f_cred };
> +
> +        // SAFETY: The signature of this function ensures that the caller will only access the
> +        // returned credential while the file is still valid, and the C side ensures that the
> +        // credential stays valid at least as long as the file.
> +        unsafe { Credential::from_ptr(ptr) }
> +    }
> +
>      /// Returns the flags associated with the file.
>      ///
>      /// The flags are a combination of the constants in [`flags`].
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index ce9abceab784..097fe9bb93ed 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -33,6 +33,7 @@
>  #[cfg(not(testlib))]
>  mod allocator;
>  mod build_assert;
> +pub mod cred;
>  pub mod error;
>  pub mod file;
>  pub mod init;
> 
> -- 
> 2.43.0.rc2.451.g8631bc7472-goog
> 
>
Alice Ryhl Dec. 11, 2023, 3:34 p.m. UTC | #3
Benno Lossin <benno.lossin@proton.me> writes:
> On 12/6/23 12:59, Alice Ryhl wrote:
> > +/// Wraps the kernel's `struct cred`.
> > +///
> > +/// # Invariants
> > +///
> > +/// Instances of this type are always ref-counted, that is, a call to `get_cred` ensures that the
> > +/// allocation remains valid at least until the matching call to `put_cred`.
> > +#[repr(transparent)]
> > +pub struct Credential(pub(crate) Opaque<bindings::cred>);
> 
> Why is the field `pub(crate)`?

Historical accident. It isn't needed anymore. I'll remove it.

> > +    unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) {
> > +        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
> 
> Can you also justify the `cast()`?

Will do.

Alice
Alice Ryhl Dec. 11, 2023, 3:34 p.m. UTC | #4
Boqun Feng <boqun.feng@gmail.com> writes:
> On Wed, Dec 06, 2023 at 11:59:47AM +0000, Alice Ryhl wrote:
> [...]
> > @@ -151,6 +152,21 @@ pub fn as_ptr(&self) -> *mut bindings::file {
> >          self.0.get()
> >      }
> >  
> > +    /// Returns the credentials of the task that originally opened the file.
> > +    pub fn cred(&self) -> &Credential {
> 
> I wonder whether it would be helpful if we use explicit lifetime here:
> 
>     pub fn cred<'file>(&'file self) -> &'file Credential
> 
> It might be easier for people to get. For example, the lifetime of the
> returned Credential reference is constrainted by 'file, the lifetime of
> the file reference.
> 
> But yes, maybe need to hear others' feedback first.
> 
> Regards,
> Boqun

That would trigger a compiler warning because the lifetime is
unnecessary.

The safety comment explains what the signature means. I think that
should be enough.

Alice
Boqun Feng Dec. 11, 2023, 5:35 p.m. UTC | #5
On Mon, Dec 11, 2023 at 03:34:29PM +0000, Alice Ryhl wrote:
> Boqun Feng <boqun.feng@gmail.com> writes:
> > On Wed, Dec 06, 2023 at 11:59:47AM +0000, Alice Ryhl wrote:
> > [...]
> > > @@ -151,6 +152,21 @@ pub fn as_ptr(&self) -> *mut bindings::file {
> > >          self.0.get()
> > >      }
> > >  
> > > +    /// Returns the credentials of the task that originally opened the file.
> > > +    pub fn cred(&self) -> &Credential {
> > 
> > I wonder whether it would be helpful if we use explicit lifetime here:
> > 
> >     pub fn cred<'file>(&'file self) -> &'file Credential
> > 
> > It might be easier for people to get. For example, the lifetime of the
> > returned Credential reference is constrainted by 'file, the lifetime of
> > the file reference.
> > 
> > But yes, maybe need to hear others' feedback first.
> > 
> > Regards,
> > Boqun
> 
> That would trigger a compiler warning because the lifetime is
> unnecessary.
> 

We can disable that warning if people need the information. Code is
mostly for reading, less often for compilation and changes.

> The safety comment explains what the signature means. I think that
> should be enough.
> 

For someone who has a good understanding of Rust lifetime (and the
elision), yes. But I'm wondering whether all the people feel the same
way.

Regards,
Boqun

> Alice
Benno Lossin Dec. 11, 2023, 7:30 p.m. UTC | #6
On 12/11/23 18:35, Boqun Feng wrote:
> On Mon, Dec 11, 2023 at 03:34:29PM +0000, Alice Ryhl wrote:
>> Boqun Feng <boqun.feng@gmail.com> writes:
>>> On Wed, Dec 06, 2023 at 11:59:47AM +0000, Alice Ryhl wrote:
>>> [...]
>>>> @@ -151,6 +152,21 @@ pub fn as_ptr(&self) -> *mut bindings::file {
>>>>          self.0.get()
>>>>      }
>>>>
>>>> +    /// Returns the credentials of the task that originally opened the file.
>>>> +    pub fn cred(&self) -> &Credential {
>>>
>>> I wonder whether it would be helpful if we use explicit lifetime here:
>>>
>>>     pub fn cred<'file>(&'file self) -> &'file Credential
>>>
>>> It might be easier for people to get. For example, the lifetime of the
>>> returned Credential reference is constrainted by 'file, the lifetime of
>>> the file reference.
>>>
>>> But yes, maybe need to hear others' feedback first.
>>>
>>> Regards,
>>> Boqun
>>
>> That would trigger a compiler warning because the lifetime is
>> unnecessary.
>>
> 
> We can disable that warning if people need the information. Code is
> mostly for reading, less often for compilation and changes.
> 
>> The safety comment explains what the signature means. I think that
>> should be enough.
>>
> 
> For someone who has a good understanding of Rust lifetime (and the
> elision), yes. But I'm wondering whether all the people feel the same
> way.

So in this example, I think it should be straight forward what happens
to the lifetimes, since there is only one to begin with.
If we want to do this, I think we should have some rules around it.

A general piece of advice I can offer is this:
- when there are no lifetimes in the return type of a function, then you
  probably do not care about the lifetimes (they will all be distinct
  and there are no relationships between them).
- when there is a single input and a single output lifetime, then
  they are the same.
- the other cases are not so simple, but most of the time they will
  require explicit annotations.

I left out some details, you can read more at [1]. Most cases where it
is not obvious which lifetime relations exist are already rejected by
the compiler. The only exception is if there is a `&self` or `&mut self`
parameter, then that one has precedence. So we could also explicitly
annotate those. Since they are also rare, I think this would be fine.


[1]: https://doc.rust-lang.org/nomicon/lifetime-elision.html
Alice Ryhl Dec. 12, 2023, 9:40 a.m. UTC | #7
On Mon, Dec 11, 2023 at 6:35 PM Boqun Feng <boqun.feng@gmail.com> wrote:
> On Mon, Dec 11, 2023 at 03:34:29PM +0000, Alice Ryhl wrote:
> > The safety comment explains what the signature means. I think that
> > should be enough.
> >
>
> For someone who has a good understanding of Rust lifetime (and the
> elision), yes. But I'm wondering whether all the people feel the same
> way.

The safety comment doesn't require understanding of lifetime elision
to be understood: "The signature of this function ensures that the
caller will only access the returned credential while the file is
still valid."

Yes, if you don't know the syntax for lifetimes, you'll have to trust
me that this is what the signature means. But I think that's the case
either way. I don't think it needs to be changed.

Alice
diff mbox series

Patch

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index beed3ef1fbc3..6d1bd2229aab 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -7,6 +7,7 @@ 
  */
 
 #include <kunit/test.h>
+#include <linux/cred.h>
 #include <linux/errname.h>
 #include <linux/file.h>
 #include <linux/fs.h>
diff --git a/rust/helpers.c b/rust/helpers.c
index 03141a3608a4..10ed69f76424 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -23,6 +23,7 @@ 
 #include <kunit/test-bug.h>
 #include <linux/bug.h>
 #include <linux/build_bug.h>
+#include <linux/cred.h>
 #include <linux/err.h>
 #include <linux/errname.h>
 #include <linux/fs.h>
@@ -164,6 +165,18 @@  struct file *rust_helper_get_file(struct file *f)
 }
 EXPORT_SYMBOL_GPL(rust_helper_get_file);
 
+const struct cred *rust_helper_get_cred(const struct cred *cred)
+{
+	return get_cred(cred);
+}
+EXPORT_SYMBOL_GPL(rust_helper_get_cred);
+
+void rust_helper_put_cred(const struct cred *cred)
+{
+	put_cred(cred);
+}
+EXPORT_SYMBOL_GPL(rust_helper_put_cred);
+
 /*
  * `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) indices.
diff --git a/rust/kernel/cred.rs b/rust/kernel/cred.rs
new file mode 100644
index 000000000000..497058ec89bb
--- /dev/null
+++ b/rust/kernel/cred.rs
@@ -0,0 +1,64 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! Credentials management.
+//!
+//! C header: [`include/linux/cred.h`](../../../../include/linux/cred.h)
+//!
+//! Reference: <https://www.kernel.org/doc/html/latest/security/credentials.html>
+
+use crate::{
+    bindings,
+    types::{AlwaysRefCounted, Opaque},
+};
+
+/// Wraps the kernel's `struct cred`.
+///
+/// # Invariants
+///
+/// Instances of this type are always ref-counted, that is, a call to `get_cred` ensures that the
+/// allocation remains valid at least until the matching call to `put_cred`.
+#[repr(transparent)]
+pub struct Credential(pub(crate) Opaque<bindings::cred>);
+
+// SAFETY: By design, the only way to access a `Credential` is via an immutable reference or an
+// `ARef`. This means that the only situation in which a `Credential` can be accessed mutably is
+// when the refcount drops to zero and the destructor runs. It is safe for that to happen on any
+// thread, so it is ok for this type to be `Send`.
+unsafe impl Send for Credential {}
+
+// SAFETY: It's OK to access `Credential` through shared references from other threads because
+// we're either accessing properties that don't change or that are properly synchronised by C code.
+unsafe impl Sync for Credential {}
+
+impl Credential {
+    /// Creates a reference to a [`Credential`] from a valid pointer.
+    ///
+    /// # Safety
+    ///
+    /// The caller must ensure that `ptr` is valid and remains valid for the lifetime of the
+    /// returned [`Credential`] reference.
+    pub unsafe fn from_ptr<'a>(ptr: *const bindings::cred) -> &'a Credential {
+        // SAFETY: The safety requirements guarantee the validity of the dereference, while the
+        // `Credential` type being transparent makes the cast ok.
+        unsafe { &*ptr.cast() }
+    }
+
+    /// Returns the effective UID of the given credential.
+    pub fn euid(&self) -> bindings::kuid_t {
+        // SAFETY: By the type invariant, we know that `self.0` is valid.
+        unsafe { (*self.0.get()).euid }
+    }
+}
+
+// SAFETY: The type invariants guarantee that `Credential` is always ref-counted.
+unsafe impl AlwaysRefCounted for Credential {
+    fn inc_ref(&self) {
+        // SAFETY: The existence of a shared reference means that the refcount is nonzero.
+        unsafe { bindings::get_cred(self.0.get()) };
+    }
+
+    unsafe fn dec_ref(obj: core::ptr::NonNull<Self>) {
+        // SAFETY: The safety requirements guarantee that the refcount is nonzero.
+        unsafe { bindings::put_cred(obj.cast().as_ptr()) };
+    }
+}
diff --git a/rust/kernel/file.rs b/rust/kernel/file.rs
index 29e1aacacd06..a88140794a8d 100644
--- a/rust/kernel/file.rs
+++ b/rust/kernel/file.rs
@@ -7,6 +7,7 @@ 
 
 use crate::{
     bindings,
+    cred::Credential,
     error::{code::*, Error, Result},
     types::{ARef, AlwaysRefCounted, Opaque},
 };
@@ -151,6 +152,21 @@  pub fn as_ptr(&self) -> *mut bindings::file {
         self.0.get()
     }
 
+    /// Returns the credentials of the task that originally opened the file.
+    pub fn cred(&self) -> &Credential {
+        // SAFETY: Since the caller holds a reference to the file, it is guaranteed that its
+        // refcount does not hit zero during this function call.
+        //
+        // It's okay to read the `f_cred` field without synchronization as `f_cred` is never
+        // changed after initialization of the file.
+        let ptr = unsafe { (*self.as_ptr()).f_cred };
+
+        // SAFETY: The signature of this function ensures that the caller will only access the
+        // returned credential while the file is still valid, and the C side ensures that the
+        // credential stays valid at least as long as the file.
+        unsafe { Credential::from_ptr(ptr) }
+    }
+
     /// Returns the flags associated with the file.
     ///
     /// The flags are a combination of the constants in [`flags`].
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index ce9abceab784..097fe9bb93ed 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -33,6 +33,7 @@ 
 #[cfg(not(testlib))]
 mod allocator;
 mod build_assert;
+pub mod cred;
 pub mod error;
 pub mod file;
 pub mod init;