On Thu, Mar 29, 2018 at 02:37:10PM -0700, Casey Schaufler wrote:

On 3/29/2018 2:14 PM, Sargun Dhillon wrote:

quoted

This patch introduces a mechanism to add mutable hooks and immutable
hooks to the callback chain. It adds an intermediary item to the
chain which separates mutable and immutable hooks. Immutable hooks
are then marked as read-only, as well as the hook heads. This does
not preclude some hooks being able to be mutated (removed).

It also wraps the hook unloading, and execution with an SRCU. One
SRCU is used across all hooks, as the SRCU struct can be memory
intensive, and hook execution time in general should be relatively
short.

Signed-off-by: Sargun Dhillon <redacted>
Signed-off-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
---
 include/linux/lsm_hooks.h  |  23 ++---
 security/Kconfig           |   2 +-
 security/apparmor/lsm.c    |   2 +-
 security/commoncap.c       |   2 +-
 security/security.c        | 210 ++++++++++++++++++++++++++++++++++++++-------
 security/selinux/hooks.c   |   5 +-
 security/smack/smack_lsm.c |   3 +-
 security/tomoyo/tomoyo.c   |   3 +-
 security/yama/yama_lsm.c   |   2 +-
 9 files changed, 196 insertions(+), 56 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 09bc60fb35f1..689e5e72fb38 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h

@@ -1981,9 +1981,12 @@ extern struct security_hook_heads security_hook_heads;
 extern char *lsm_names;
 
 extern void security_add_hooks(struct security_hook_list *hooks, int count,
-				char *lsm);
+				char *lsm, bool is_mutable);
 
-#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+#define __lsm_ro_after_init	__ro_after_init
+/* Currently required to handle SELinux runtime hook disable. */
+#ifdef CONFIG_SECURITY_WRITABLE_HOOKS
+#define __lsm_mutable_after_init
 /*
  * Assuring the safety of deleting a security module is up to
  * the security module involved. This may entail ordering the

@@ -1996,21 +1999,9 @@ extern void security_add_hooks(struct security_hook_list *hooks, int count,
  * disabling their module is a good idea needs to be at least as
  * careful as the SELinux team.
  */
-static inline void security_delete_hooks(struct security_hook_list *hooks,
-						int count)
-{
-	int i;
-
-	for (i = 0; i < count; i++)
-		hlist_del_rcu(&hooks[i].list);
-}
-#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
-
-/* Currently required to handle SELinux runtime hook disable. */
-#ifdef CONFIG_SECURITY_WRITABLE_HOOKS
-#define __lsm_ro_after_init
+extern void security_delete_hooks(struct security_hook_list *hooks, int count);
 #else
-#define __lsm_ro_after_init	__ro_after_init
+#define __lsm_mutable_after_init __ro_after_init
 #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
 
 extern int __init security_module_enable(const char *module);

diff --git a/security/Kconfig b/security/Kconfig
index c4302067a3ad..a3b8b1142e6f 100644
--- a/security/Kconfig
+++ b/security/Kconfig

@@ -32,7 +32,7 @@ config SECURITY
 	  If you are unsure how to answer this question, answer N.
 
 config SECURITY_WRITABLE_HOOKS
-	depends on SECURITY
+	depends on SECURITY && SRCU
 	bool
 	default n
 

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 9a65eeaf7dfa..d6cca8169df0 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c

@@ -1155,7 +1155,7 @@ static int __init apparmor_init(void)
 		goto buffers_out;
 	}
 	security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
-				"apparmor");
+				"apparmor", false);
 
 	/* Report that AppArmor successfully initialized */
 	apparmor_initialized = 1;

diff --git a/security/commoncap.c b/security/commoncap.c
index 48620c93d697..fe4b0d9d44ce 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c

@@ -1363,7 +1363,7 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
 void __init capability_add_hooks(void)
 {
 	security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
-				"capability");
+				"capability", false);
 }
 
 #endif /* CONFIG_SECURITY */

diff --git a/security/security.c b/security/security.c
index 3cafff61b049..2ddb64864e3e 100644
--- a/security/security.c
+++ b/security/security.c

@@ -29,6 +29,11 @@
 #include <linux/backing-dev.h>
 #include <linux/string.h>
 #include <net/flow.h>
+#include <linux/srcu.h>
+#include <linux/mutex.h>
+
+#define SECURITY_HOOK_COUNT \
+	(sizeof(security_hook_heads) / sizeof(struct hlist_head))
 
 #define MAX_LSM_EVM_XATTR	2
 

@@ -36,7 +41,10 @@
 #define SECURITY_NAME_MAX	10
 
 struct security_hook_heads security_hook_heads __lsm_ro_after_init;
+EXPORT_SYMBOL_GPL(security_hook_heads);
+
 static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
+static DEFINE_MUTEX(security_hook_mutex);
 
 char *lsm_names;
 /* Boot-time LSM user choice */

@@ -53,6 +61,103 @@ static void __init do_security_initcalls(void)
 	}
 }
 
+#ifdef CONFIG_SECURITY_WRITABLE_HOOKS
+DEFINE_STATIC_SRCU(security_hook_srcu);
+static struct security_hook_list	null_hooks[SECURITY_HOOK_COUNT];
+#define HAS_FUNC(SHL, FUNC)	(SHL->hook.FUNC)

The HAS_FUNC() macro will work, but it's awkward outside of the
call_..._hook() macros. I think you should document how to use it
properly somewhere in here. There are enough cases where the
call_..._hook() macros aren't used that someone could have trouble
figuring out how to use it.

What about something like:

 security/security.c | 58 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 42 insertions(+), 16 deletions(-)

diff --git a/security/security.c b/security/security.c
index 2ddb64864e3e..bc14125cfc78 100644
--- a/security/security.c
+++ b/security/security.c

@@ -62,9 +62,37 @@ static void __init do_security_initcalls(void)
 }
 
 #ifdef CONFIG_SECURITY_WRITABLE_HOOKS
-DEFINE_STATIC_SRCU(security_hook_srcu);
+/*
+ * With writable hooks, we setup a structure like this:
+ * +------+   +-----------+   +-----------+   +-----------+   +--------------+
+ * |      |   |           |   |           |   |           |   |              |
+ * | HEAD +---> Immutable +---> Immutable +---> Null hook +---> Mutable Hook |
+ * |      |   |  Hook 1   |   |  Hook 2   |   |           |   |              |
+ * +------+   +-----------+   +-----------+   +-----------+   +--------------+
+ *                  |               |                                |
+ *                  v               v                                v
+ *              Callback        Callback                         Callback
+ *
+ * The hooks before to null hook are marked only after kernel initialization.
+ * The null hook, as well as the hooks succeeding it are not marked read only,
+ * therefore allowing them be (un)loaded after initialization time.
+ *
+ * Since the null hook doesn't have a callback, we need to check if a hook
+ * is the null hook prior to invoking it.
+ */
 static struct security_hook_list	null_hooks[SECURITY_HOOK_COUNT];
-#define HAS_FUNC(SHL, FUNC)	(SHL->hook.FUNC)
+DEFINE_STATIC_SRCU(security_hook_srcu);
+
+static inline bool is_null_hook(struct security_hook_list *shl)
+{
+	union {
+		void *cb_ptr;
+		union security_list_options slo;
+	} hook_options;
+
+	hook_options.slo = shl->hook;
+	return !hook_options.cb_ptr;
+}
 
 static inline int lock_lsm(void)
 {

@@ -88,14 +116,9 @@ static inline void unlock_lsm(int idx)
 static void security_add_hook(struct security_hook_list *hook, bool is_mutable)
 {
 	struct security_hook_list *mutable_hook;
-	union {
-		void *cb_ptr;
-		union security_list_options slo;
-	} hook_options;
 
 	hlist_for_each_entry(mutable_hook, hook->head, list) {
-		hook_options.slo = mutable_hook->hook;
-		if (hook_options.cb_ptr)
+		if (!is_null_hook(mutable_hook))
 			continue;
 
 		if (is_mutable)

@@ -139,7 +162,10 @@ void security_delete_hooks(struct security_hook_list *hooks, int count)
 EXPORT_SYMBOL_GPL(security_delete_hooks);
 
 #else
-#define HAS_FUNC(SHL, FUNC)	true
+static inline bool is_null_hook(struct security_hook_list *shl)
+{
+	return false;
+}
 
 static inline int lock_lsm(void)
 {

@@ -309,7 +335,7 @@ EXPORT_SYMBOL(unregister_lsm_notifier);
 									\
 		srcu_idx = lock_lsm();					\
 		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
-			if (HAS_FUNC(P, FUNC))				\
+			if (!is_null_hook(P))			\
 				P->hook.FUNC(__VA_ARGS__);		\
 		unlock_lsm(srcu_idx);					\
 	} while (0)

@@ -322,7 +348,7 @@ EXPORT_SYMBOL(unregister_lsm_notifier);
 		struct security_hook_list *P;			\
 								\
 		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
-			if (HAS_FUNC(P, FUNC)) {		\
+			if (!is_null_hook(P)) {			\
 				RC = P->hook.FUNC(__VA_ARGS__);	\
 				if (RC != 0)			\
 					break;			\

@@ -434,7 +460,7 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 	 */
 	srcu_idx = lock_lsm();
 	hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) {
-		if (!HAS_FUNC(hp, vm_enough_memory))
+		if (is_null_hook(hp))
 			continue;
 		rc = hp->hook.vm_enough_memory(mm, pages);
 		if (rc <= 0) {

@@ -928,7 +954,7 @@ int security_inode_getsecurity(struct inode *inode, const char *name,
 	 */
 	srcu_idx = lock_lsm();
 	hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
-		if (!HAS_FUNC(hp, inode_getsecurity))
+		if (is_null_hook(hp))
 			continue;
 		rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
 		if (rc != -EOPNOTSUPP)

@@ -953,7 +979,7 @@ int security_inode_setsecurity(struct inode *inode, const char *name,
 	 */
 	srcu_idx = lock_lsm();
 	hlist_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) {
-		if (!HAS_FUNC(hp, inode_setsecurity))
+		if (is_null_hook(hp))
 			continue;
 		rc = hp->hook.inode_setsecurity(inode, name, value, size,
 						flags);

@@ -1264,7 +1290,7 @@ int security_task_prctl(int option, unsigned long arg2,
 
 	srcu_idx = lock_lsm();
 	hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
-		if (!HAS_FUNC(hp, task_prctl))
+		if (is_null_hook(hp))
 			continue;
 		thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
 		if (thisrc != -ENOSYS) {

@@ -1774,7 +1800,7 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
 	hlist_for_each_entry(hp,
 				&security_hook_heads.xfrm_state_pol_flow_match,
 				list) {
-		if (!HAS_FUNC(hp, xfrm_state_pol_flow_match))
+		if (is_null_hook(hp))
 			continue;
 		rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
 		break;

--

To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html