On Tue, Sep 8, 2020 at 12:44 AM Lakshmi Ramasubramanian
[off-list ref] wrote:

On 9/7/20 3:32 PM, Stephen Smalley wrote:

quoted

quoted

Signed-off-by: Lakshmi Ramasubramanian <redacted>
Suggested-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Reported-by: kernel test robot <redacted> # error: implicit declaration of function 'vfree'
Reported-by: kernel test robot <redacted> # error: implicit declaration of function 'crypto_alloc_shash'
Reported-by: kernel test robot <redacted> # sparse: symbol 'security_read_selinux_policy' was not declared. Should it be static?

Not sure these Reported-by lines are useful since they were just on
submitted versions of the patch not on an actual merged commit.

I'll remove them when I update the patch.

quoted

quoted

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

<snip>

quoted

+void selinux_measure_state(struct selinux_state *state, bool policy_mutex_held)
+{

<snip>

quoted

+
+       if (!policy_mutex_held)
+               mutex_lock(&state->policy_mutex);
+
+       rc = security_read_policy_kernel(state, &policy, &policy_len);
+
+       if (!policy_mutex_held)
+               mutex_unlock(&state->policy_mutex);

This kind of conditional taking of a mutex is generally frowned upon
in my experience.
You should likely just always take the mutex in the callers of
selinux_measure_state() instead.
In some cases, it may be the caller of the caller.  Arguably selinuxfs
could be taking it around all state modifying operations (e.g.
enforce, checkreqprot) not just policy modifying ones although it
isn't strictly for that purpose.

Since currently policy_mutex is not used to synchronize access to state
variables (enforce, checkreqprot, etc.) I am wondering if
selinux_measure_state() should measure only state if policy_mutex is not
held by the caller - similar to how we skip measuring policy if
initialization is not yet completed.

No, we want to measure policy whenever there is a policy to measure.
Just move the taking of the mutex to the callers of
selinux_measure_state() so that it can be unconditional.