[PATCH v3 0/1] Safe LSM (un)loading, and immutable hooks

[PATCH v3 0/1] Safe LSM (un)loading, and immutable hooks

[Sargun Dhillon]

The biggest security benefit of this patchset is the introduction of
read-only hooks. Currently, if you have any LSMs with mutable hooks
it will render all heads, and list nodes mutable. This is a prime
place to attack, because being able to manipulate those hooks is a
way to bypass all LSMs easily.

By moving to hlist_head, it keeps a singly-linked, non-circular list.
This can always be marked as read only because we add a mutable "null"
hook. All immutable LSMs should be installed as immutable hooks and
sit before the null hook.

+------+   +-----------+   +-----------+   +-----------+   +------------------+
|      |   |           |   |           |   |           |   |                  |
| HEAD +---> Immutable +---> Immutable +---> Null hook +--->   Mutable Hook   |
|      |   |  Hook 1   |   |  Hook 2   |   |           |   |                  |
+------+   +-----------+   +-----------+   +-----------+   +------------------+
                 |               |                                  |
                 v               v                                  v
             Callback        Callback                           Callback

If LSMs have a model to be unloaded, or are compled as modules,
they should mark themselves mutable at runtime.

In order to provide safe code-unloading, there is a shared SRCU between
all security hooks. This SRCU is very cheap for runtime overhead on
reads, but there is synchronization around it for unloads. There is
only a cost to pay at unload time, which is based on the execution time
of longest chain of callbacks after synchronization begins.

Because of all of this, we can now load LSMs at runtime, so those APIs
are exposed. It is up to the module author to check if
CONFIG_SECURITY_WRITABLE_HOOKS is enabled prior to trying to load.


Changes since:
v2:
	* Split out hlist_head patch
	* Apply Tetsuo's changes to clean up functions which are not
	  covered by call_int_hook / call_void_hook
	* Disable NULL hook checking when uneeded
v1:
	* Add SRCU to allow for code-unloading
	* Add concurrency control around hook mutation

Sargun Dhillon (1):
  security: Add mechanism to safely (un)load LSMs after boot time

 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(-)

-- 
2.14.1

[PATCH v3 1/1] security: Add mechanism to safely (un)load LSMs after boot time

[Sargun Dhillon]

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 <sargun@sargun.me>
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)
+
+static inline int lock_lsm(void)
+{
+	return srcu_read_lock(&security_hook_srcu);
+}
+
+static inline void unlock_lsm(int idx)
+{
+	srcu_read_unlock(&security_hook_srcu, idx);
+}
+
+/*
+ * This has to look for the null hook in the hook list by looking for
+ * a security_hook_list with a NULL callback on the callback chain.
+ *
+ * If is_mutable is set, the hook must be installed after the
+ * null hook. It is the caller's responsibility to ensure that
+ * they only load immutable hooks at boot time, and to not
+ * attempt to unload them.
+ */
+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)
+			continue;
+
+		if (is_mutable)
+			hlist_add_behind_rcu(&hook->list, &mutable_hook->list);
+		else
+			hlist_add_before_rcu(&hook->list, &mutable_hook->list);
+		return;
+	}
+
+	panic("Unable to install hook, cannot find mutable hook\n");
+}
+
+/*
+ * The mutable hooks exist as a mechanism to transition from the read-only
+ * set of hooks to the mutable set of hooks.
+ */
+static void __init setup_mutable_hooks(void)
+{
+	struct hlist_head *list = (struct hlist_head *) &security_hook_heads;
+	struct security_hook_list *shl;
+	int i;
+
+	for (i = 0; i < SECURITY_HOOK_COUNT; i++) {
+		shl = &null_hooks[i];
+		shl->head = &list[i];
+		hlist_add_head_rcu(&shl->list, shl->head);
+	}
+}
+
+void security_delete_hooks(struct security_hook_list *hooks, int count)
+{
+	int i;
+
+	mutex_lock(&security_hook_mutex);
+	for (i = 0; i < count; i++)
+		hlist_del_rcu(&hooks[i].list);
+	mutex_unlock(&security_hook_mutex);
+
+	synchronize_srcu(&security_hook_srcu);
+}
+EXPORT_SYMBOL_GPL(security_delete_hooks);
+
+#else
+#define HAS_FUNC(SHL, FUNC)	true
+
+static inline int lock_lsm(void)
+{
+	return 0;
+}
+
+static inline void unlock_lsm(int idx) {}
+
+static void security_add_hook(struct security_hook_list *hook, bool is_mutable)
+{
+	WARN_ONCE(is_mutable,
+			"Mutable hook loaded with writable hooks disabled");
+	hlist_add_tail_rcu(&hook->list, hook->head);
+}
+
+static void __init setup_mutable_hooks(void) {}
+#endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
+
 /**
  * security_init - initializes the security framework
  *
@@ -60,14 +165,9 @@ static void __init do_security_initcalls(void)
  */
 int __init security_init(void)
 {
-	int i;
-	struct hlist_head *list = (struct hlist_head *) &security_hook_heads;
-
-	for (i = 0; i < sizeof(security_hook_heads) / sizeof(struct hlist_head);
-	     i++)
-		INIT_HLIST_HEAD(&list[i]);
 	pr_info("Security Framework initialized\n");
 
+	setup_mutable_hooks();
 	/*
 	 * Load minor LSMs, with the capability module always first.
 	 */
@@ -153,21 +253,26 @@ int __init security_module_enable(const char *module)
  * @hooks: the hooks to add
  * @count: the number of hooks to add
  * @lsm: the name of the security module
+ * @is_mutable: is this hook mutable after kernel init
  *
  * Each LSM has to register its hooks with the infrastructure.
  */
-void __init security_add_hooks(struct security_hook_list *hooks, int count,
-				char *lsm)
+void security_add_hooks(struct security_hook_list *hooks, int count,
+				char *lsm, bool is_mutable)
 {
 	int i;
 
+	mutex_lock(&security_hook_mutex);
 	for (i = 0; i < count; i++) {
 		hooks[i].lsm = lsm;
-		hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
+		security_add_hook(&hooks[i], is_mutable);
 	}
+	mutex_unlock(&security_hook_mutex);
+
 	if (lsm_append(lsm, &lsm_names) < 0)
 		panic("%s - Cannot get early memory.\n", __func__);
 }
+EXPORT_SYMBOL_GPL(security_add_hooks);
 
 int call_lsm_notifier(enum lsm_event event, void *data)
 {
@@ -197,25 +302,34 @@ EXPORT_SYMBOL(unregister_lsm_notifier);
  *	This is a hook that returns a value.
  */
 
-#define call_void_hook(FUNC, ...)				\
-	do {							\
-		struct security_hook_list *P;			\
-								\
+#define call_void_hook(FUNC, ...)					\
+	do {								\
+		struct security_hook_list *P;				\
+		int srcu_idx;						\
+									\
+		srcu_idx = lock_lsm();					\
 		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
-			P->hook.FUNC(__VA_ARGS__);		\
+			if (HAS_FUNC(P, FUNC))				\
+				P->hook.FUNC(__VA_ARGS__);		\
+		unlock_lsm(srcu_idx);					\
 	} while (0)
 
 #define call_int_hook(FUNC, IRC, ...) ({			\
-	int RC = IRC;						\
+	int srcu_idx, RC = IRC;					\
+								\
+	srcu_idx = lock_lsm();					\
 	do {							\
 		struct security_hook_list *P;			\
 								\
 		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
-			RC = P->hook.FUNC(__VA_ARGS__);		\
-			if (RC != 0)				\
-				break;				\
+			if (HAS_FUNC(P, FUNC)) {		\
+				RC = P->hook.FUNC(__VA_ARGS__);	\
+				if (RC != 0)			\
+					break;			\
+			}					\
 		}						\
 	} while (0);						\
+	unlock_lsm(srcu_idx);					\
 	RC;							\
 })
 
@@ -309,6 +423,7 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 	struct security_hook_list *hp;
 	int cap_sys_admin = 1;
 	int rc;
+	int srcu_idx;
 
 	/*
 	 * The module will respond with a positive value if
@@ -317,13 +432,17 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 	 * agree that it should be set it will. If any module
 	 * thinks it should not be set it won't.
 	 */
+	srcu_idx = lock_lsm();
 	hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) {
+		if (!HAS_FUNC(hp, vm_enough_memory))
+			continue;
 		rc = hp->hook.vm_enough_memory(mm, pages);
 		if (rc <= 0) {
 			cap_sys_admin = 0;
 			break;
 		}
 	}
+	unlock_lsm(srcu_idx);
 	return __vm_enough_memory(mm, pages, cap_sys_admin);
 }
 
@@ -795,43 +914,58 @@ int security_inode_killpriv(struct dentry *dentry)
 	return call_int_hook(inode_killpriv, 0, dentry);
 }
 
-int security_inode_getsecurity(struct inode *inode, const char *name, void **buffer, bool alloc)
+int security_inode_getsecurity(struct inode *inode, const char *name,
+					void **buffer, bool alloc)
 {
 	struct security_hook_list *hp;
-	int rc;
+	int rc = -EOPNOTSUPP;
+	int srcu_idx;
 
 	if (unlikely(IS_PRIVATE(inode)))
 		return -EOPNOTSUPP;
 	/*
 	 * Only one module will provide an attribute with a given name.
 	 */
+	srcu_idx = lock_lsm();
 	hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
+		if (!HAS_FUNC(hp, inode_getsecurity))
+			continue;
 		rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
 		if (rc != -EOPNOTSUPP)
-			return rc;
+			break;
 	}
-	return -EOPNOTSUPP;
+	unlock_lsm(srcu_idx);
+	return rc;
 }
 
-int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags)
+int security_inode_setsecurity(struct inode *inode, const char *name,
+					const void *value, size_t size,
+					int flags)
 {
 	struct security_hook_list *hp;
-	int rc;
+	int rc = -EOPNOTSUPP;
+	int srcu_idx;
 
 	if (unlikely(IS_PRIVATE(inode)))
 		return -EOPNOTSUPP;
 	/*
 	 * Only one module will provide an attribute with a given name.
 	 */
+	srcu_idx = lock_lsm();
 	hlist_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) {
+		if (!HAS_FUNC(hp, inode_setsecurity))
+			continue;
 		rc = hp->hook.inode_setsecurity(inode, name, value, size,
-								flags);
+						flags);
 		if (rc != -EOPNOTSUPP)
-			return rc;
+			break;
 	}
-	return -EOPNOTSUPP;
+	unlock_lsm(srcu_idx);
+	return rc;
 }
 
+
+
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size)
 {
 	if (unlikely(IS_PRIVATE(inode)))
@@ -1119,14 +1253,19 @@ int security_task_kill(struct task_struct *p, struct siginfo *info,
 	return call_int_hook(task_kill, 0, p, info, sig, secid);
 }
 
-int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
-			 unsigned long arg4, unsigned long arg5)
+int security_task_prctl(int option, unsigned long arg2,
+				 unsigned long arg3, unsigned long arg4,
+				 unsigned long arg5)
 {
 	int thisrc;
 	int rc = -ENOSYS;
 	struct security_hook_list *hp;
+	int srcu_idx;
 
+	srcu_idx = lock_lsm();
 	hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
+		if (!HAS_FUNC(hp, task_prctl))
+			continue;
 		thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
 		if (thisrc != -ENOSYS) {
 			rc = thisrc;
@@ -1134,6 +1273,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 				break;
 		}
 	}
+	unlock_lsm(srcu_idx);
 	return rc;
 }
 
@@ -1614,11 +1754,12 @@ int security_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir)
 }
 
 int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
-				       struct xfrm_policy *xp,
-				       const struct flowi *fl)
+						struct xfrm_policy *xp,
+						const struct flowi *fl)
 {
 	struct security_hook_list *hp;
 	int rc = 1;
+	int srcu_idx;
 
 	/*
 	 * Since this function is expected to return 0 or 1, the judgment
@@ -1629,11 +1770,16 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
 	 * For speed optimization, we explicitly break the loop rather than
 	 * using the macro
 	 */
-	hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match,
+	srcu_idx = lock_lsm();
+	hlist_for_each_entry(hp,
+				&security_hook_heads.xfrm_state_pol_flow_match,
 				list) {
+		if (!HAS_FUNC(hp, xfrm_state_pol_flow_match))
+			continue;
 		rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
 		break;
 	}
+	unlock_lsm(srcu_idx);
 	return rc;
 }
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8644d864e3c1..f05184a3a7a6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6393,7 +6393,7 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
 }
 #endif
 
-static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
+static struct security_hook_list selinux_hooks[] __lsm_mutable_after_init = {
 	LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
 	LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),
 	LSM_HOOK_INIT(binder_transfer_binder, selinux_binder_transfer_binder),
@@ -6651,7 +6651,8 @@ static __init int selinux_init(void)
 					    0, SLAB_PANIC, NULL);
 	avc_init();
 
-	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
+	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux",
+				IS_ENABLED(CONFIG_SECURITY_SELINUX_DISABLE));
 
 	if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
 		panic("SELinux: Unable to register AVC netcache callback\n");
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 03fdecba93bb..7a9f1bb06c8e 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4902,7 +4902,8 @@ static __init int smack_init(void)
 	/*
 	 * Register with LSM
 	 */
-	security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
+	security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack",
+				false);
 
 	return 0;
 }
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 213b8c593668..ba74fab0e9a5 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -543,7 +543,8 @@ static int __init tomoyo_init(void)
 	if (!security_module_enable("tomoyo"))
 		return 0;
 	/* register ourselves with the security framework */
-	security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
+	security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo",
+				false);
 	printk(KERN_INFO "TOMOYO Linux initialized\n");
 	cred->security = &tomoyo_kernel_domain;
 	tomoyo_mm_init();
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index ffda91a4a1aa..04c9aed9e951 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -480,6 +480,6 @@ static inline void yama_init_sysctl(void) { }
 void __init yama_add_hooks(void)
 {
 	pr_info("Yama: becoming mindful.\n");
-	security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama");
+	security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama", false);
 	yama_init_sysctl();
 }
-- 
2.14.1

Re: [PATCH v3 1/1] security: Add mechanism to safely (un)load LSMs after boot time

[Casey Schaufler]

On 3/29/2018 2:14 PM, Sargun Dhillon wrote:
> 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 <sargun@sargun.me>
> 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.

Re: [PATCH v3 1/1] security: Add mechanism to safely (un)load LSMs after boot time

[Sargun Dhillon]

On Thu, Mar 29, 2018 at 02:37:10PM -0700, Casey Schaufler wrote:
> On 3/29/2018 2:14 PM, Sargun Dhillon wrote:
> > 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 <sargun@sargun.me>
> > 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;

Re: [PATCH v3 1/1] security: Add mechanism to safely (un)load LSMs after boot time

[Casey Schaufler]

On 3/29/2018 7:33 PM, Sargun Dhillon wrote:
> On Thu, Mar 29, 2018 at 02:37:10PM -0700, Casey Schaufler wrote:
>> On 3/29/2018 2:14 PM, Sargun Dhillon wrote:
>>> 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 <sargun@sargun.me>
>>> 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.
> + */

I think a comment like this is helpful.

Why not have two hook list heads, one for regular hooks and
one for mutable hooks? You can dispense with the "null hook"
handling.

>  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;
> +}

I like the HAS_FUNC() approach better.

What I think would work best is to have a separate list head
for the mutable hooks. 

Re: [PATCH v3 1/1] security: Add mechanism to safely (un)load LSMs after boot time

[Sargun Dhillon]

On Fri, Mar 30, 2018 at 2:39 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 3/29/2018 7:33 PM, Sargun Dhillon wrote:
>> On Thu, Mar 29, 2018 at 02:37:10PM -0700, Casey Schaufler wrote:
>>> On 3/29/2018 2:14 PM, Sargun Dhillon wrote:
>>>> 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 <sargun@sargun.me>
>>>> 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.
>> + */
>
> I think a comment like this is helpful.
>
> Why not have two hook list heads, one for regular hooks and
> one for mutable hooks? You can dispense with the "null hook"
> handling.
>
The iterations gets really messy. The patch earlier had a lot of
awkward gunk in it. The issue primarily came with deciding to iterate
over the second set of hooks iff the first set of hooks failed. With
the way the calls are setup, it's messy.

What's your primary issue with the "null hooks"? If it's the null hook
checks, I think I can actually remove those.

>>  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;
>> +}
>
> I like the HAS_FUNC() approach better.
 Just curious, why? I personally prefer small static inline functions
over macros, if possible.

>
> What I think would work best is to have a separate list head
> for the mutable hooks.
>
>
>

Re: [PATCH v3 1/1] security: Add mechanism to safely (un)load LSMs after boot time

[Kees Cook]

On Fri, Mar 30, 2018 at 11:16 PM, Sargun Dhillon <sargun@sargun.me> wrote:
> On Fri, Mar 30, 2018 at 2:39 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>  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;
>>> +}
>>
>> I like the HAS_FUNC() approach better.
>
>  Just curious, why? I personally prefer small static inline functions
> over macros, if possible.

Generally speaking, small static inline functions are better since
they provide type-checking. In this case, though, it looks like you're
just doing a cast, but with a union. Why isn't this just:

return !!((uintptr_t)shl->hook)

?

Though the security_list_options union exists for callback type
checking, so really, having HAS_FUNC() with the explicit function
you're interested in creates a bit of self-documenting code (even if
it always resolves to the above test).

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH v3 1/1] security: Add mechanism to safely (un)load LSMs after boot time

[Casey Schaufler]

On 3/30/2018 11:16 PM, Sargun Dhillon wrote:
> On Fri, Mar 30, 2018 at 2:39 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 3/29/2018 7:33 PM, Sargun Dhillon wrote:
>>> On Thu, Mar 29, 2018 at 02:37:10PM -0700, Casey Schaufler wrote:
>>>> On 3/29/2018 2:14 PM, Sargun Dhillon wrote:
>>>>> 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 <sargun@sargun.me>
>>>>> 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.
>>> + */
>> I think a comment like this is helpful.
>>
>> Why not have two hook list heads, one for regular hooks and
>> one for mutable hooks? You can dispense with the "null hook"
>> handling.
>>
> The iterations gets really messy. The patch earlier had a lot of
> awkward gunk in it. The issue primarily came with deciding to iterate
> over the second set of hooks iff the first set of hooks failed. With
> the way the calls are setup, it's messy.

Hang on. I would think you'd only call the 2nd list if the first list
was completed successfully. And of course the void hooks always get
called regardless. 

> What's your primary issue with the "null hooks"? If it's the null hook
> checks, I think I can actually remove those.

In the common case of one LSM and no mutable modules you
are making a check on every hook that will always have the
same answer. Performance impact *of any amount* that does
not add value in the common case has to be avoided.

This is a clever implementation. But it does not generalize.
If I wanted to add a third kind of hooks the mechanism would
not support it. If you add a 2nd list it's obvious what you'd
want to do to add a 3rd.

>
>>>  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;
>>> +}
>> I like the HAS_FUNC() approach better.
>  Just curious, why? I personally prefer small static inline functions
> over macros, if possible.

Minimize the complexity, even inside a macro/function. Your
is_null_hook() function is massively more complicated than the
HAS_FUNC() macro. I'm still not 100% sure I understand why you
have the wacky union, and how you can know that it will get you
the right result on all architectures.

>
>> What I think would work best is to have a separate list head
>> for the mutable hooks.
>>
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v3 1/1] security: Add mechanism to safely (un)load LSMs after boot time

[Tetsuo Handa]

> +/*
> + * 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.
> + */

Do we need to use null hook as hook == NULL?
Why not overwrite null hook's hook field?

#define call_void_hook(FUNC, ...)                               		\
	do {                                                    		\
		struct security_hook_list *P;                   		\
		int srcu_idx = lock_lsm();					\
		for (P = &security_hook_heads.FUNC; P->hook.FUNC; P = P->next)	\
			P->hook.FUNC(__VA_ARGS__);				\
		unlock_lsm(srcu_idx);
	} while (0)

"struct hlist_head security_hook_heads[SECURITY_HOOK_COUNT]" is marked as __ro_after_init.
Built-in LSM module's "struct security_hook_list[]" is also marked as __ro_after_init.
Dynamic LSM module's "struct security_hook_list[]" is not marked as __initdata.

Hook registration function appends to tail of security_hook_heads.FUNC.
But, before __ro_after_init is applied, initial
"struct security_hook_list dynamic_hook_list[SECURITY_HOOK_COUNT]" is appended to
tail of security_hook_heads. That is, only "struct security_hook_list" at
initial dynamic_hook_list[] and later are writable.

Dynamic hook registration function overwrites current dynamic_hook_list[] with
supplied dynamic module's "struct security_hook_list[]". Then, dynamic hook
registration function allocates memory for next dynamic_hook_list[] and
appends to tail of security_hook_heads.FUNC (note that the tail element is
writable because it is guaranteed to be initial dynamic_hook_list[] or later.



Before registering first built-in immutable LSM module.

    r/w
 * +------+
 * |      |
 * | HEAD +
 * |      |
 * +------+
 *
 *
 *

Before registering second built-in immutable LSM module.

    r/w        r/w
 * +------+   +-----------+
 * |      |   |           |
 * | HEAD +---> Immutable +
 * |      |   |  Hook 1   |
 * +------+   +-----------+
 *                  |
 *                  v
 *              Callback

Before registering initial dynamic_hook_list[].

    r/w        r/w             r/w
 * +------+   +-----------+   +-----------+
 * |      |   |           |   |           |
 * | HEAD +---> Immutable +---> Immutable +
 * |      |   |  Hook 1   |   |  Hook 2   |
 * +------+   +-----------+   +-----------+
 *                  |               |
 *                  v               v
 *              Callback        Callback

After registering initial dynamic_hook_list[] and applying __ro_after_init.

    r/o        r/o             r/o             r/w
 * +------+   +-----------+   +-----------+   +----------------+
 * |      |   |           |   |           |   |                |
 * | HEAD +---> Immutable +---> Immutable +---> Hook for first +
 * |      |   |  Hook 1   |   |  Hook 2   |   | Mutable Module |
 * +------+   +-----------+   +-----------+   +----------------+
 *                  |               |
 *                  v               v
 *              Callback        Callback

After registering first mutable LSM module.

    r/o        r/o             r/o             r/w             r/w
 * +------+   +-----------+   +-----------+   +-----------+   +-----------------+
 * |      |   |           |   |           |   |           |   |                 |
 * | HEAD +---> Immutable +---> Immutable +---> Mutable   +---> Hook for second +
 * |      |   |  Hook 1   |   |  Hook 2   |   |  Hook 1   |   | Mutable Module  |
 * +------+   +-----------+   +-----------+   +-----------+   +-----------------+
 *                  |               |             |
 *                  v               v             v
 *              Callback        Callback      Callback

After registering second mutable LSM module.

    r/o        r/o             r/o             r/w             r/w             r/w
 * +------+   +-----------+   +-----------+   +-----------+   +-----------+   +-----------------+
 * |      |   |           |   |           |   |           |   |           |   |                 |
 * | HEAD +---> Immutable +---> Immutable +---> Mutable   +---> Mutable   +---> Hook for third  +
 * |      |   |  Hook 1   |   |  Hook 2   |   |  Hook 1   |   |  Hook 2   |   | Mutable Module  |
 * +------+   +-----------+   +-----------+   +-----------+   +-----------+   +-----------------+
 *                  |               |             |
 *                  v               v             v
 *              Callback        Callback      Callback

After protectable memory is accepted, all r/w above except the last one will be
marked as r/o by allocating "Hook for X'th Mutable Module" using that allocator.