On 7/15/20 11:04 AM, Stephen Smalley wrote:

quoted

+static inline bool selinux_checkreqprot(void)
+{
+       struct selinux_state *state = &selinux_state;
+
+       return state->checkreqprot;
+}

Probably should use READ_ONCE().

Will do.

quoted

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

@@ -0,0 +1,122 @@
+int selinux_security_state(void)

Let's call this selinux_measure_state() or similar.  Needs a verb. And
pass it a struct selinux_state * pointer argument to be measured, even
though initially it will always be passed &selinux_state.  The
encapsulation of selinux state within selinux_state was to support
multiple selinux namespaces in the future, each with their own state.

Will do.

quoted

+       static char *security_state_string =
+                       "enabled=%d;enforcing=%d;checkreqprot=%d;"        \
+                       "netpeer=%d;openperm=%d;extsockclass=%d;"         \
+                       "alwaysnetwork=%d;cgroupseclabel=%d;"             \
+                       "nnpnosuidtransition=%d;genfsseclabelsymlink=%d;";

Rather than hardcoding policy capability names here, I would recommend
using the selinux_policycap_names[] array for the names and the
selinux_state.policycap[] array for the values.  Also recommend
passing in a struct selinux_state * here to allow for future case
where there are multiple selinux states, one per selinux namespace.

Will do.

quoted

diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index ef0afd878bfc..0c289d13ef6a 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c

@@ -3724,10 +3724,11 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
   * security_read_policy - read the policy.
   * @data: binary policy data
   * @len: length of data in bytes
- *
+ * @alloc_kernel_memory: flag to indicate memory allocation
   */
-int security_read_policy(struct selinux_state *state,
-                        void **data, size_t *len)
+int security_read_selinux_policy(struct selinux_state *state,
+                                void **data, size_t *len,
+                                bool alloc_kernel_memory)

Instead of passing in a boolean to control how the memory is
allocated, split this into a helper function that takes an
already-allocated buffer and two
different front-end wrappers, one for kernel-internal use and one for
userspace use.

Will do.

quoted

@@ -3738,7 +3739,10 @@ int security_read_policy(struct selinux_state *state,

         *len = security_policydb_len(state);

-       *data = vmalloc_user(*len);
+       if (alloc_kernel_memory)
+               *data = kzalloc(*len, GFP_KERNEL);

You need vmalloc() since policy image size may exceed kmalloc max (or
at least that used to be the case).

Will do.

thanks,
  -lakshmi