[3/7] rust: security: add abstraction for secctx

Commit Message

Alice Ryhl Nov. 29, 2023, 1:11 p.m. UTC

Adds an abstraction for viewing the string representation of a security
context.

This is needed by Rust Binder because it has feature where a process can
view the string representation of the security context for incoming
transactions. The process can use that to authenticate incoming
transactions, and since the feature is provided by the kernel, the
process can trust that the security context is legitimate.

Signed-off-by: Alice Ryhl <[e-mail]>
---
 rust/bindings/bindings_helper.h |  1 +
 rust/helpers.c                  | 21 +++++++++
 rust/kernel/cred.rs             |  8 ++++
 rust/kernel/lib.rs              |  1 +
 rust/kernel/security.rs         | 78 +++++++++++++++++++++++++++++++++
 5 files changed, 109 insertions(+)
 create mode 100644 rust/kernel/security.rs

Comments

Benno Lossin Nov. 30, 2023, 4:26 p.m. UTC | #1

On 11/29/23 14:11, Alice Ryhl wrote:
> +/// A security context string.
> +///
> +/// The struct has the invariant that it always contains a valid security context.

Refactor to use the `# Invariants` section:

    # Invariants
    `secdata` points to a valid security context.

I also do not know what a "valid security context" is, so a link to the
definition wouldn't hurt.

> +pub struct SecurityCtx {
> +    secdata: *mut core::ffi::c_char,
> +    seclen: usize,
> +}
> +
> +impl SecurityCtx {
> +    /// Get the security context given its id.
> +    pub fn from_secid(secid: u32) -> Result<Self> {
> +        let mut secdata = core::ptr::null_mut();
> +        let mut seclen = 0;
> +        // SAFETY: Just a C FFI call. The pointers are valid for writes.
> +        unsafe {
> +            to_result(bindings::security_secid_to_secctx(
> +                secid,
> +                &mut secdata,
> +                &mut seclen,
> +            ))?;
> +        }
> +
> +        // If the above call did not fail, then we have a valid security
> +        // context, so the invariants are not violated.

Should be tagged `INVARIANT`.

> +        Ok(Self {
> +            secdata,
> +            seclen: usize::try_from(seclen).unwrap(),
> +        })
> +    }
> +
> +    /// Returns whether the security context is empty.
> +    pub fn is_empty(&self) -> bool {
> +        self.seclen == 0
> +    }
> +
> +    /// Returns the length of this security context.
> +    pub fn len(&self) -> usize {
> +        self.seclen
> +    }
> +
> +    /// Returns the bytes for this security context.
> +    pub fn as_bytes(&self) -> &[u8] {
> +        let mut ptr = self.secdata;
> +        if ptr.is_null() {
> +            // Many C APIs will use null pointers for strings of length zero, but

I would just write that the secctx API uses null pointers to denote a
string of length zero.

> +            // `slice::from_raw_parts` doesn't allow the pointer to be null even if the length is
> +            // zero. Replace the pointer with a dangling but non-null pointer in this case.
> +            debug_assert_eq!(self.seclen, 0);

I am feeling a bit uncomfortable with this, why can't we just return
an empty slice in this case?

> +            ptr = core::ptr::NonNull::dangling().as_ptr();
> +        }
> +
> +        // SAFETY: The call to `security_secid_to_secctx` guarantees that the pointer is valid for
> +        // `seclen` bytes. Furthermore, if the length is zero, then we have ensured that the
> +        // pointer is not null.
> +        unsafe { core::slice::from_raw_parts(ptr.cast(), self.seclen) }
> +    }
> +}
> +
> +impl Drop for SecurityCtx {
> +    fn drop(&mut self) {
> +        // SAFETY: This frees a pointer that came from a successful call to
> +        // `security_secid_to_secctx`.

This should be part of the type invariant.

Alice Ryhl Dec. 1, 2023, 10:48 a.m. UTC | #2

Benno Lossin <[e-mail]> writes:
> On 11/29/23 14:11, Alice Ryhl wrote:
>> +    /// Returns the bytes for this security context.
>> +    pub fn as_bytes(&self) -> &[u8] {
>> +        let mut ptr = self.secdata;
>> +        if ptr.is_null() {
>> +            // Many C APIs will use null pointers for strings of length zero, but
> 
> I would just write that the secctx API uses null pointers to denote a
> string of length zero.

I don't actually know whether it can ever be null, I just wanted to stay
on the safe side.

>> +            // `slice::from_raw_parts` doesn't allow the pointer to be null even if the length is
>> +            // zero. Replace the pointer with a dangling but non-null pointer in this case.
>> +            debug_assert_eq!(self.seclen, 0);
> 
> I am feeling a bit uncomfortable with this, why can't we just return
> an empty slice in this case?

I can do that, but to be clear, what I'm doing here is also definitely
okay.

Alice

Benno Lossin Dec. 2, 2023, 10:03 a.m. UTC | #3

On 12/1/23 11:48, Alice Ryhl wrote:
> Benno Lossin <[e-mail]> writes:
>> On 11/29/23 14:11, Alice Ryhl wrote:
>>> +    /// Returns the bytes for this security context.
>>> +    pub fn as_bytes(&self) -> &[u8] {
>>> +        let mut ptr = self.secdata;
>>> +        if ptr.is_null() {
>>> +            // Many C APIs will use null pointers for strings of length zero, but
>>
>> I would just write that the secctx API uses null pointers to denote a
>> string of length zero.
> 
> I don't actually know whether it can ever be null, I just wanted to stay
> on the safe side.

I see, can someone from the C side confirm/refute this?

I found the comment a bit weird, since it is phrased in a general way.
If it turns out that the pointer can never be null, maybe use `NonNull`
instead (I would then also move the length check into the constructor)?
You can probably also do this if the pointer is allowed to be null,
assuming that you then do not need to call `security_release_secctx`.

>>> +            // `slice::from_raw_parts` doesn't allow the pointer to be null even if the length is
>>> +            // zero. Replace the pointer with a dangling but non-null pointer in this case.
>>> +            debug_assert_eq!(self.seclen, 0);
>>
>> I am feeling a bit uncomfortable with this, why can't we just return
>> an empty slice in this case?
> 
> I can do that, but to be clear, what I'm doing here is also definitely
> okay.

Yes it is okay, but I see this similar to avoiding `unsafe` code when it
can be done safely. In this example we are not strictly avoiding any
`unsafe` code, but we are avoiding a codepath with `unsafe` code. You
should of course still keep the `debug_assert_eq`.

Patch

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 6d1bd2229aab..81b13a953eae 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -11,6 +11,7 @@ 
 #include <linux/errname.h>
 #include <linux/file.h>
 #include <linux/fs.h>
+#include <linux/security.h>
 #include <linux/slab.h>
 #include <linux/refcount.h>
 #include <linux/wait.h>
diff --git a/rust/helpers.c b/rust/helpers.c
index 10ed69f76424..fd633d9db79a 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -30,6 +30,7 @@ 
 #include <linux/mutex.h>
 #include <linux/refcount.h>
 #include <linux/sched/signal.h>
+#include <linux/security.h>
 #include <linux/spinlock.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
@@ -177,6 +178,26 @@  void rust_helper_put_cred(const struct cred *cred)
 }
 EXPORT_SYMBOL_GPL(rust_helper_put_cred);
 
+#ifndef CONFIG_SECURITY
+void rust_helper_security_cred_getsecid(const struct cred *c, u32 *secid)
+{
+	security_cred_getsecid(c, secid);
+}
+EXPORT_SYMBOL_GPL(rust_helper_security_cred_getsecid);
+
+int rust_helper_security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
+{
+	return security_secid_to_secctx(secid, secdata, seclen);
+}
+EXPORT_SYMBOL_GPL(rust_helper_security_secid_to_secctx);
+
+void rust_helper_security_release_secctx(char *secdata, u32 seclen)
+{
+	security_release_secctx(secdata, seclen);
+}
+EXPORT_SYMBOL_GPL(rust_helper_security_release_secctx);
+#endif
+
 /*
  * `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
index 497058ec89bb..3794937b5294 100644
--- a/rust/kernel/cred.rs
+++ b/rust/kernel/cred.rs
@@ -43,6 +43,14 @@  pub unsafe fn from_ptr<'a>(ptr: *const bindings::cred) -> &'a Credential {
         unsafe { &*ptr.cast() }
     }
 
+    /// Get the id for this security context.
+    pub fn get_secid(&self) -> u32 {
+        let mut secid = 0;
+        // SAFETY: The invariants of this type ensures that the pointer is valid.
+        unsafe { bindings::security_cred_getsecid(self.0.get(), &mut secid) };
+        secid
+    }
+
     /// 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.
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 097fe9bb93ed..342cb02c495a 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -42,6 +42,7 @@ 
 pub mod kunit;
 pub mod prelude;
 pub mod print;
+pub mod security;
 mod static_assert;
 #[doc(hidden)]
 pub mod std_vendor;
diff --git a/rust/kernel/security.rs b/rust/kernel/security.rs
new file mode 100644
index 000000000000..69c10ed89a57
--- /dev/null
+++ b/rust/kernel/security.rs
@@ -0,0 +1,78 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! Linux Security Modules (LSM).
+//!
+//! C header: [`include/linux/security.h`](../../../../include/linux/security.h).
+
+use crate::{
+    bindings,
+    error::{to_result, Result},
+};
+
+/// A security context string.
+///
+/// The struct has the invariant that it always contains a valid security context.
+pub struct SecurityCtx {
+    secdata: *mut core::ffi::c_char,
+    seclen: usize,
+}
+
+impl SecurityCtx {
+    /// Get the security context given its id.
+    pub fn from_secid(secid: u32) -> Result<Self> {
+        let mut secdata = core::ptr::null_mut();
+        let mut seclen = 0;
+        // SAFETY: Just a C FFI call. The pointers are valid for writes.
+        unsafe {
+            to_result(bindings::security_secid_to_secctx(
+                secid,
+                &mut secdata,
+                &mut seclen,
+            ))?;
+        }
+
+        // If the above call did not fail, then we have a valid security
+        // context, so the invariants are not violated.
+        Ok(Self {
+            secdata,
+            seclen: usize::try_from(seclen).unwrap(),
+        })
+    }
+
+    /// Returns whether the security context is empty.
+    pub fn is_empty(&self) -> bool {
+        self.seclen == 0
+    }
+
+    /// Returns the length of this security context.
+    pub fn len(&self) -> usize {
+        self.seclen
+    }
+
+    /// Returns the bytes for this security context.
+    pub fn as_bytes(&self) -> &[u8] {
+        let mut ptr = self.secdata;
+        if ptr.is_null() {
+            // Many C APIs will use null pointers for strings of length zero, but
+            // `slice::from_raw_parts` doesn't allow the pointer to be null even if the length is
+            // zero. Replace the pointer with a dangling but non-null pointer in this case.
+            debug_assert_eq!(self.seclen, 0);
+            ptr = core::ptr::NonNull::dangling().as_ptr();
+        }
+
+        // SAFETY: The call to `security_secid_to_secctx` guarantees that the pointer is valid for
+        // `seclen` bytes. Furthermore, if the length is zero, then we have ensured that the
+        // pointer is not null.
+        unsafe { core::slice::from_raw_parts(ptr.cast(), self.seclen) }
+    }
+}
+
+impl Drop for SecurityCtx {
+    fn drop(&mut self) {
+        // SAFETY: This frees a pointer that came from a successful call to
+        // `security_secid_to_secctx`.
+        unsafe {
+            bindings::security_release_secctx(self.secdata, self.seclen as u32);
+        }
+    }
+}