On 12/23/20 1:10 PM, Paul Moore wrote:

Hi Paul,

...

quoted

diff --git a/security/selinux/Makefile b/security/selinux/Makefile
index 4d8e0e8adf0b..83d512116341 100644
--- a/security/selinux/Makefile
+++ b/security/selinux/Makefile

@@ -16,6 +16,8 @@ selinux-$(CONFIG_NETLABEL) += netlabel.o

  selinux-$(CONFIG_SECURITY_INFINIBAND) += ibpkey.o

+selinux-$(CONFIG_IMA) += measure.o

Naming things is hard, I get that, but I would prefer if we just
called this file "ima.c" or something similar.  The name "measure.c"
implies a level of abstraction or general use which simply doesn't
exist here.  Let's help make it a bit more obvious what should belong
in this file.

Agreed - I will rename the file to ima.c

quoted

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 3cc8bab31ea8..18ee65c98446 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h

@@ -229,7 +229,8 @@ void selinux_policy_cancel(struct selinux_state *state,
                         struct selinux_policy *policy);
  int security_read_policy(struct selinux_state *state,
                          void **data, size_t *len);
-
+int security_read_policy_kernel(struct selinux_state *state,
+                               void **data, size_t *len);
  int security_policycap_supported(struct selinux_state *state,
                                  unsigned int req_cap);

@@ -446,4 +447,12 @@ extern void ebitmap_cache_init(void);
  extern void hashtab_cache_init(void);
  extern int security_sidtab_hash_stats(struct selinux_state *state, char *page);

+#ifdef CONFIG_IMA
+extern void selinux_measure_state(struct selinux_state *selinux_state);
+#else
+static inline void selinux_measure_state(struct selinux_state *selinux_state)
+{
+}
+#endif

If you are going to put the SELinux/IMA function(s) into a separate
source file, please put the function declarations into a separate
header file too.  For example, look at
security/selinux/include/{netif,netnode,netport,etc.}.h.

I will create a new header file "security/selinux/include/ima.h" and 
move the function declarations for IMA functions to that header.

quoted

diff --git a/security/selinux/measure.c b/security/selinux/measure.c
new file mode 100644
index 000000000000..b7e24358e11d
--- /dev/null
+++ b/security/selinux/measure.c

@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Measure SELinux state using IMA subsystem.
+ */
+#include <linux/vmalloc.h>
+#include <linux/ktime.h>
+#include <linux/ima.h>
+#include "security.h"
+
+/*
+ * This function creates a unique name by appending the timestamp to
+ * the given string. This string is passed as "event_name" to the IMA
+ * hook to measure the given SELinux data.
+ *
+ * The data provided by SELinux to the IMA subsystem for measuring may have
+ * already been measured (for instance the same state existed earlier).
+ * But for SELinux the current data represents a state change and hence
+ * needs to be measured again. To enable this, pass a unique "event_name"
+ * to the IMA hook so that IMA subsystem will always measure the given data.
+ *
+ * For example,
+ * At time T0 SELinux data to be measured is "foo". IMA measures it.
+ * At time T1 the data is changed to "bar". IMA measures it.
+ * At time T2 the data is changed to "foo" again. IMA will not measure it
+ * (since it was already measured) unless the event_name, for instance,
+ * is different in this call.
+ */
+static char *selinux_event_name(const char *name_prefix)
+{
+       struct timespec64 cur_time;
+
+       ktime_get_real_ts64(&cur_time);
+       return kasprintf(GFP_KERNEL, "%s-%lld:%09ld", name_prefix,
+                        cur_time.tv_sec, cur_time.tv_nsec);
+}

Why is this a separate function?  It's three lines long and only
called from selinux_measure_state().  Do you ever see the SELinux/IMA
code in this file expanding to the point where this function is nice
from a reuse standpoint?

Earlier I had two measurements - one for SELinux configuration/state and 
another for SELinux policy. selinux_event_name() was used to generate 
event name for each of them.

In this patch set I have included only one measurement - for SELinux 
policy. I plan to add "SELinux configuration/state" measurement in a 
separate patch - I can reuse selinux_event_name() in that patch.

Also, I think the comment in the function header for 
selinux_event_name() is useful.

I prefer to have a separate function, if that's fine by you.

Also, I assume you are not concerned about someone circumventing the
IMA measurements by manipulating the time?  In most systems I would
expect the time to be a protected entity, but with many systems
getting their time from remote systems I thought it was worth
mentioning.

I am using time function to generate a unique name for the IMA 
measurement event, such as, "selinux-policy-hash-1609790281:860232824". 
This is to ensure that state changes in SELinux data are always measured.

If you think time manipulation can be an issue, please let me know a 
better way to generate unique event names.

quoted

+/*
+ * selinux_measure_state - Measure hash of the SELinux policy
+ *
+ * @state: selinux state struct
+ *
+ * NOTE: This function must be called with policy_mutex held.
+ */
+void selinux_measure_state(struct selinux_state *state)

Similar to the name of this source file, let's make it clear this is
for IMA.  How about calling this selinux_ima_measure_state() or
similar?

Sure - I will change the function name to selinux_ima_measure_state().

quoted

+{
+       void *policy = NULL;
+       char *policy_event_name = NULL;
+       size_t policy_len;
+       int rc = 0;
+       bool initialized = selinux_initialized(state);

Why bother with the initialized variable?  Unless I'm missing
something it is only used once in the code below.

You are right - I will remove "initialized" variable and directly get 
the state using selinux_initialized().

quoted

+       /*
+        * Measure SELinux policy only after initialization is completed.
+        */
+       if (!initialized)
+               goto out;
+
+       policy_event_name = selinux_event_name("selinux-policy-hash");
+       if (!policy_event_name) {
+               pr_err("SELinux: %s: event name for policy not allocated.\n",
+                      __func__);
+               rc = -ENOMEM;

This function doesn't return an error code, why bother with setting
the rc variable here?

Yes - it is not necessary. I will remove the line.

quoted

+               goto out;
+       }
+
+       rc = security_read_policy_kernel(state, &policy, &policy_len);
+       if (rc) {
+               pr_err("SELinux: %s: failed to read policy %d.\n", __func__, rc);
+               goto out;
+       }
+
+       ima_measure_critical_data("selinux", policy_event_name,
+                                 policy, policy_len, true);
+
+       vfree(policy);
+
+out:
+       kfree(policy_event_name);
+}

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 9704c8a32303..dfa2e00894ae 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c

@@ -2180,6 +2180,7 @@ static void selinux_notify_policy_change(struct selinux_state *state,
         selinux_status_update_policyload(state, seqno);
         selinux_netlbl_cache_invalidate();
         selinux_xfrm_notify_policyload();
+       selinux_measure_state(state);
  }

  void selinux_policy_commit(struct selinux_state *state,

@@ -3875,8 +3876,33 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
  }
  #endif /* CONFIG_NETLABEL */

+/**
+ * security_read_selinux_policy - read the policy.
+ * @policy: SELinux policy
+ * @data: binary policy data
+ * @len: length of data in bytes
+ *
+ */
+static int security_read_selinux_policy(struct selinux_policy *policy,
+                                       void *data, size_t *len)

Let's just call this "security_read_policy()".

There is another function in this file with the name security_read_policy().

How about changing the above function name to "read_selinux_policy()" 
since this is a local/static function.

quoted

+{
+       int rc;
+       struct policy_file fp;
+
+       fp.data = data;
+       fp.len = *len;
+
+       rc = policydb_write(&policy->policydb, &fp);
+       if (rc)
+               return rc;
+
+       *len = (unsigned long)fp.data - (unsigned long)data;
+       return 0;
+}
+
  /**
   * security_read_policy - read the policy.
+ * @state: selinux_state
   * @data: binary policy data
   * @len: length of data in bytes
   *

@@ -3885,8 +3911,6 @@ int security_read_policy(struct selinux_state *state,
                          void **data, size_t *len)
  {
         struct selinux_policy *policy;
-       int rc;
-       struct policy_file fp;

         policy = rcu_dereference_protected(
                         state->policy, lockdep_is_held(&state->policy_mutex));

@@ -3898,14 +3922,43 @@ int security_read_policy(struct selinux_state *state,
         if (!*data)> --

2.17.1

quoted

                 return -ENOMEM;

-       fp.data = *data;
-       fp.len = *len;
+       return security_read_selinux_policy(policy, *data, len);
+}

-       rc = policydb_write(&policy->policydb, &fp);
-       if (rc)
-               return rc;
+/**
+ * security_read_policy_kernel - read the policy.
+ * @state: selinux_state
+ * @data: binary policy data
+ * @len: length of data in bytes
+ *
+ * Allocates kernel memory for reading SELinux policy.
+ * This function is for internal use only and should not
+ * be used for returning data to user space.
+ *
+ * This function must be called with policy_mutex held.
+ */
+int security_read_policy_kernel(struct selinux_state *state,
+                               void **data, size_t *len)

Let's call this "security_read_state_kernel()".

Sure - I will rename the function.

quoted

+{
+       struct selinux_policy *policy;
+       int rc = 0;

See below, the rc variable is not needed.

quoted

-       *len = (unsigned long)fp.data - (unsigned long)*data;
-       return 0;
+       policy = rcu_dereference_protected(
+                       state->policy, lockdep_is_held(&state->policy_mutex));
+       if (!policy) {
+               rc = -EINVAL;
+               goto out;

Jumping to the out label is a little silly since it is just a return;
do a "return -EINVAL;" here instead.

quoted

+       }
+
+       *len = policy->policydb.len;
+       *data = vmalloc(*len);
+       if (!*data) {
+               rc = -ENOMEM;
+               goto out;

Same as above, "return -ENOMEM;" please.

quoted

+       }

+       rc = security_read_selinux_policy(policy, *data, len);

You should be able to do "return security_read_selinux_policy(...);" here.

I will remove the local variable "rc" and make the three changes you've 
stated above.

Thanks for reviewing the changes.

  -lakshmi

quoted

+
+out:
+       return rc;
  }