[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Sargun Dhillon]

The primary security benefit of this patchset is the introduction of
read-only hooks, even if some security modules have mutable hooks.
Currently, if you have any LSMs with mutable hooks it will render all
heads, and list nodes mutable. These are a prime place to attack because
being able to manipulate those hooks is a way to bypass all LSMs easily
and to create a persistent, covert channel to intercept nearly all
calls.

There is a shares SRCU between all security hooks to facilitate safe
LSM- unloading. 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.

If CONFIG_SECURITY_WRITABLE_HOOKS is enabled, then hooks can be loaded
at runtime. The module can check the return code of security_add_hooks
to determine whether or not they can install their hooks, independently
of checking for the Kconfig value. In the future, we may make it so that
runtime hook loading is also disabled, after boot. We can do that in a
follow-up patch.

If CONFIG_SECURITY_UNREGISTRABLE_HOOKS is enabled, then hooks can be
unloaded at runtime. This behaviour can be disabled by setting
security.allow_unregister_hooks to 0. Once set, it requires a reboot
to be reset.

SELinux is exempt from the CONFIG_SECURITY_UNREGISTRABLE_HOOKS
rule, and can unregister at any time. SELinux exposes a mechanism
to disable itself, prior to policy loading. This can be disabled
at compilation time, and it depends on CONFIG_SECURITY_WRITABLE_HOOKS.
Changing this behaviour would require breaking the uapi.

This patch can allow for a new style of LSMs. There are many cases
where LSM "policies" would be better defined in some formal
programming language, like C. This is either due to flexibility,
or performance, for functions in the hot path.

It also unlocks development usecases, but those come as a secondary
benefit to the earlier feature. There has been an appetite for
out-of-tree LSMs:
http://kernsec.org/pipermail/linux-security-module-archive/2017-March/000119.html
Casey's work has been great enabling minor LSM stacking, and a variety
of LSMs have been proposed on list that haven't made it into the kernel
but fit into the minor LSM stacking model.

One of the other big changes is the introduction of the lsm_info
structure. This encapsulates various information about the LSM
itself, and can be used at a later time.

This patch was tested with CONFIG_PROVE_LOCKING to prove the correctness
of the locking in lock_existing_hooks. It grabs the module lock
to prevent new hooks from being loaded, or current hooks from being
unloaded. It then grabs the lsm_info_lock, which is only grabbed
on hook modification, or inspection of the loaded LSMs.

The other thing that this introduces is monitoring of LKM
unloading. If an LSM's LKM is unloaded when it is not
supposed to be, it will crash the system.

Thanks to Tetsuo for their review, and feedback.

Changes since:
v6:
  * introduce LSM info
  * Crash by default instead of leaving it up to module authors
    to deal with things going wrong
  * Monitor module unloading, to ensure that attempts to
    unload the LKM for an LSM crashes the system.
v5:
  * Rename "CONFIG_SECURITY_UNLOADABLE_MODULES" to
    "CONFIG_SECURITY_UNREGISTRABLE_HOOKS" which enables arbitrary hook
    deregistration. If disabled, hooks are not allowed to deregister.
    SELinux is exempt.
  * Rename security.allow_unload_hooks to
    security.allow_unregister_hooks
  * Make it so that both security_add_hooks, and security_delete_hooks
    return errors upon being unsuccessful, allowing module authors to
    make the best decision for their use case.
  * Get rid of LSM_HOOK_INIT_MUTABLE
v4:
  * Introduce the configuration flag "CONFIG_SECURITY_UNLOADABLE_MODULES"
    to disable module unloading
  * Introduce the kernel parameter security.allow_unload_hooks
v3:
  * Instead of taking the approach of a "null hook", using the approach of
    a second set of hooks -- this was mostly done through the
    FOR_EACH_SECURITY_HOOK_MUTABLE macro, which gets compiled out if
    CONFIG_SECURITY_WRITABLE_HOOKS is disabled.
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 (6):
  security: Move LSM registration arguments to struct lsm_info
  security: Make security_hook_heads private
  security: Introduce mutable (RW) hooks
  security: Expose security_add_hooks externally and add error handling
  security: Panic on forced unloading of security module
  security: Add SECURITY_UNREGISTRABLE_HOOKS to allow for hook removal

 include/linux/lsm_hooks.h                     |  67 ++--
 scripts/gcc-plugins/randomize_layout_plugin.c |   2 -
 security/Kconfig                              |  12 +
 security/apparmor/lsm.c                       |   8 +-
 security/commoncap.c                          |   8 +-
 security/inode.c                              |  56 +++-
 security/loadpin/loadpin.c                    |   6 +-
 security/security.c                           | 421 +++++++++++++++++++++-----
 security/security.h                           |  11 +
 security/selinux/hooks.c                      |  16 +-
 security/smack/smack_lsm.c                    |   5 +-
 security/tomoyo/tomoyo.c                      |   6 +-
 security/yama/yama_lsm.c                      |   6 +-
 13 files changed, 491 insertions(+), 133 deletions(-)
 create mode 100644 security/security.h

-- 
2.14.1

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

[PATCH v7 1/6] security: Move LSM registration arguments to struct lsm_info

[Sargun Dhillon]

Previously, when LSMs registered, they independently passed their name
and hook count. This had two implications:

1) Is required us to clone the name, so we could present it in
   security FS. This required memory allocation at start time.
2) Every time we wanted to tie more information back from
   the security hooks, to the LSM, we would have to add
   duplicated fields in struct security_hook_list.

It also introduces a new file -- security/security.h, which is meant
to be private headers to be shared only between pieces of security
"infrastructure".

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 include/linux/lsm_hooks.h  | 44 ++++++++++-------------
 security/apparmor/lsm.c    |  6 ++--
 security/commoncap.c       |  8 +++--
 security/inode.c           | 56 +++++++++++++++++++++++++----
 security/loadpin/loadpin.c |  4 ++-
 security/security.c        | 89 +++++++++++++++++++++++-----------------------
 security/security.h        | 10 ++++++
 security/selinux/hooks.c   |  6 ++--
 security/smack/smack_lsm.c |  3 +-
 security/tomoyo/tomoyo.c   |  4 ++-
 security/yama/yama_lsm.c   |  4 ++-
 11 files changed, 147 insertions(+), 87 deletions(-)
 create mode 100644 security/security.h

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 9d0b286f3dba..65f346cb6639 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2004,11 +2004,20 @@ struct security_hook_heads {
  * Security module hook list structure.
  * For use with generic list macros for common operations.
  */
+struct security_hook_list;
+struct lsm_info {
+	struct hlist_node		list;
+	const char			*name;
+	const unsigned int		count;
+	struct security_hook_list	*hooks;
+} __randomize_layout;
+
 struct security_hook_list {
 	struct hlist_node		list;
 	struct hlist_head		*head;
 	union security_list_options	hook;
-	char				*lsm;
+	/* This field is not currently in use */
+	struct lsm_info			*info;
 } __randomize_layout;
 
 /*
@@ -2020,33 +2029,18 @@ struct security_hook_list {
 #define LSM_HOOK_INIT(HEAD, HOOK) \
 	{ .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } }
 
-extern struct security_hook_heads security_hook_heads;
-extern char *lsm_names;
+#define LSM_MODULE_INIT(NAME, HOOKS)		\
+	{					\
+		.name	= NAME,			\
+		.hooks	= HOOKS,		\
+		.count	= ARRAY_SIZE(HOOKS),	\
+	}
 
-extern void security_add_hooks(struct security_hook_list *hooks, int count,
-				char *lsm);
+extern struct security_hook_heads security_hook_heads;
+extern void security_add_hooks(struct lsm_info *lsm);
 
 #ifdef CONFIG_SECURITY_SELINUX_DISABLE
-/*
- * Assuring the safety of deleting a security module is up to
- * the security module involved. This may entail ordering the
- * module's hook list in a particular way, refusing to disable
- * the module once a policy is loaded or any number of other
- * actions better imagined than described.
- *
- * The name of the configuration option reflects the only module
- * that currently uses the mechanism. Any developer who thinks
- * 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);
-}
+void security_delete_hooks(struct lsm_info *lsm);
 #endif /* CONFIG_SECURITY_SELINUX_DISABLE */
 
 /* Currently required to handle SELinux runtime hook disable. */
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index ce2b89e9ad94..d0b3f9a6d488 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1190,6 +1190,9 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(task_kill, apparmor_task_kill),
 };
 
+static struct lsm_info apparmor_info =
+	LSM_MODULE_INIT("apparmor", apparmor_hooks);
+
 /*
  * AppArmor sysfs module parameters
  */
@@ -1561,8 +1564,7 @@ static int __init apparmor_init(void)
 		aa_free_root_ns();
 		goto buffers_out;
 	}
-	security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
-				"apparmor");
+	security_add_hooks(&apparmor_info);
 
 	/* Report that AppArmor successfully initialized */
 	apparmor_initialized = 1;
diff --git a/security/commoncap.c b/security/commoncap.c
index 48620c93d697..d25891adda81 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1339,7 +1339,7 @@ int cap_mmap_file(struct file *file, unsigned long reqprot,
 
 #ifdef CONFIG_SECURITY
 
-struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
+static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(capable, cap_capable),
 	LSM_HOOK_INIT(settime, cap_settime),
 	LSM_HOOK_INIT(ptrace_access_check, cap_ptrace_access_check),
@@ -1360,10 +1360,12 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(vm_enough_memory, cap_vm_enough_memory),
 };
 
+static struct lsm_info capability_info =
+	LSM_MODULE_INIT("capability", capability_hooks);
+
 void __init capability_add_hooks(void)
 {
-	security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
-				"capability");
+	security_add_hooks(&capability_info);
 }
 
 #endif /* CONFIG_SECURITY */
diff --git a/security/inode.c b/security/inode.c
index 8dd9ca8848e4..554258be2949 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -22,6 +22,8 @@
 #include <linux/security.h>
 #include <linux/lsm_hooks.h>
 #include <linux/magic.h>
+#include <linux/seq_file.h>
+#include "security.h"
 
 static struct vfsmount *mount;
 static int mount_count;
@@ -309,16 +311,58 @@ EXPORT_SYMBOL_GPL(securityfs_remove);
 
 #ifdef CONFIG_SECURITY
 static struct dentry *lsm_dentry;
-static ssize_t lsm_read(struct file *filp, char __user *buf, size_t count,
-			loff_t *ppos)
+
+static void *lsm_seq_start(struct seq_file *s, loff_t *pos)
+{
+	int ret;
+
+	ret = mutex_lock_killable(&lsm_info_lock);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return seq_hlist_start(&lsm_info_head, *pos);
+}
+
+static int lsm_seq_show(struct seq_file *s, void *v)
+{
+	struct hlist_node *node = (struct hlist_node *)v;
+	struct lsm_info *info;
+
+	info = hlist_entry(node, struct lsm_info, list);
+	if (node->next)
+		seq_printf(s, "%s,", info->name);
+	else
+		seq_printf(s, "%s", info->name);
+	return 0;
+}
+
+static void *lsm_seq_next(struct seq_file *s, void *v, loff_t *pos)
+{
+	return seq_hlist_next(v, &lsm_info_head, pos);
+}
+
+static void lsm_seq_stop(struct seq_file *s, void *v)
+{
+	mutex_unlock(&lsm_info_lock);
+}
+
+static const struct seq_operations lsm_seq_ops = {
+	.start		= lsm_seq_start,
+	.next		= lsm_seq_next,
+	.stop		= lsm_seq_stop,
+	.show		= lsm_seq_show,
+};
+
+static int lsm_ops_open(struct inode *inode, struct file *file)
 {
-	return simple_read_from_buffer(buf, count, ppos, lsm_names,
-		strlen(lsm_names));
+	return seq_open(file, &lsm_seq_ops);
 }
 
 static const struct file_operations lsm_ops = {
-	.read = lsm_read,
-	.llseek = generic_file_llseek,
+	.open		= lsm_ops_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= seq_release,
 };
 #endif
 
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index 5fa191252c8f..dabc49b855f7 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -178,10 +178,12 @@ static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
 };
 
+static struct lsm_info loadpin_info = LSM_MODULE_INIT("loadpin", loadpin_hooks);
+
 void __init loadpin_add_hooks(void)
 {
 	pr_info("ready to pin (currently %sabled)", enabled ? "en" : "dis");
-	security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
+	security_add_hooks(&loadpin_info);
 }
 
 /* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
diff --git a/security/security.c b/security/security.c
index 7bc2fde023a7..36b9d2b0a135 100644
--- a/security/security.c
+++ b/security/security.c
@@ -28,7 +28,9 @@
 #include <linux/personality.h>
 #include <linux/backing-dev.h>
 #include <linux/string.h>
+#include <linux/mutex.h>
 #include <net/flow.h>
+#include "security.h"
 
 #include <trace/events/initcall.h>
 
@@ -37,10 +39,12 @@
 /* Maximum number of letters for an LSM name string */
 #define SECURITY_NAME_MAX	10
 
+DEFINE_MUTEX(lsm_info_lock);
+struct hlist_head lsm_info_head __lsm_ro_after_init = HLIST_HEAD_INIT;
 struct security_hook_heads security_hook_heads __lsm_ro_after_init;
 static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
 
-char *lsm_names;
+
 /* Boot-time LSM user choice */
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
 	CONFIG_DEFAULT_SECURITY;
@@ -97,40 +101,6 @@ static int __init choose_lsm(char *str)
 }
 __setup("security=", choose_lsm);
 
-static bool match_last_lsm(const char *list, const char *lsm)
-{
-	const char *last;
-
-	if (WARN_ON(!list || !lsm))
-		return false;
-	last = strrchr(list, ',');
-	if (last)
-		/* Pass the comma, strcmp() will check for '\0' */
-		last++;
-	else
-		last = list;
-	return !strcmp(last, lsm);
-}
-
-static int lsm_append(char *new, char **result)
-{
-	char *cp;
-
-	if (*result == NULL) {
-		*result = kstrdup(new, GFP_KERNEL);
-	} else {
-		/* Check if it is the last registered name */
-		if (match_last_lsm(*result, new))
-			return 0;
-		cp = kasprintf(GFP_KERNEL, "%s,%s", *result, new);
-		if (cp == NULL)
-			return -ENOMEM;
-		kfree(*result);
-		*result = cp;
-	}
-	return 0;
-}
-
 /**
  * security_module_enable - Load given security module on boot ?
  * @module: the name of the module
@@ -154,25 +124,54 @@ int __init security_module_enable(const char *module)
 	return !strcmp(module, chosen_lsm);
 }
 
+#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+/*
+ * Assuring the safety of deleting a security module is up to
+ * the security module involved. This may entail ordering the
+ * module's hook list in a particular way, refusing to disable
+ * the module once a policy is loaded or any number of other
+ * actions better imagined than described.
+ *
+ * The name of the configuration option reflects the only module
+ * that currently uses the mechanism. Any developer who thinks
+ * disabling their module is a good idea needs to be at least as
+ * careful as the SELinux team.
+ */
+void security_delete_hooks(struct lsm_info *info)
+{
+	struct security_hook_list *hook;
+	int i;
+
+	for (i = 0; i < info->count; i++) {
+		hook = &info->hooks[i];
+		hlist_del_rcu(&hook->list);
+	}
+
+	mutex_lock(&lsm_info_lock);
+	hlist_del(&info->list);
+	mutex_unlock(&lsm_info_lock);
+}
+#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
 /**
  * security_add_hooks - Add a modules hooks to the hook lists.
- * @hooks: the hooks to add
- * @count: the number of hooks to add
- * @lsm: the name of the security module
+ * @lsm_info: The lsm_info struct for this security module
  *
  * 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 __init security_add_hooks(struct lsm_info *info)
 {
+	struct security_hook_list *hook;
 	int i;
 
-	for (i = 0; i < count; i++) {
-		hooks[i].lsm = lsm;
-		hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
+	for (i = 0; i < info->count; i++) {
+		hook = &info->hooks[i];
+		hook->info = info;
+		hlist_add_tail_rcu(&hook->list, hook->head);
 	}
-	if (lsm_append(lsm, &lsm_names) < 0)
-		panic("%s - Cannot get early memory.\n", __func__);
+
+	mutex_lock(&lsm_info_lock);
+	hlist_add_tail_rcu(&info->list, &lsm_info_head);
+	mutex_unlock(&lsm_info_lock);
 }
 
 int call_lsm_notifier(enum lsm_event event, void *data)
diff --git a/security/security.h b/security/security.h
new file mode 100644
index 000000000000..79d1388fb038
--- /dev/null
+++ b/security/security.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include <linux/mutex.h>
+#include <linux/list.h>
+#include <linux/lsm_hooks.h>
+
+#ifndef __SECURITY_SECURITY_H
+#define __SECURITY_SECURITY_H
+extern struct hlist_head lsm_info_head;
+extern struct mutex lsm_info_lock;
+#endif
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4cafe6a19167..a6d1bc76340e 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -7083,6 +7083,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 #endif
 };
 
+static struct lsm_info selinux_info = LSM_MODULE_INIT("selinux", selinux_hooks);
+
 static __init int selinux_init(void)
 {
 	if (!security_module_enable("selinux")) {
@@ -7122,7 +7124,7 @@ static __init int selinux_init(void)
 
 	hashtab_cache_init();
 
-	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
+	security_add_hooks(&selinux_info);
 
 	if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
 		panic("SELinux: Unable to register AVC netcache callback\n");
@@ -7269,7 +7271,7 @@ int selinux_disable(struct selinux_state *state)
 
 	selinux_enabled = 0;
 
-	security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
+	security_delete_hooks(&selinux_info);
 
 	/* Try to destroy the avc node cache */
 	avc_disable();
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 0b414836bebd..a8959cc29bde 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4764,6 +4764,7 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(dentry_create_files_as, smack_dentry_create_files_as),
 };
 
+static struct lsm_info smack_info = LSM_MODULE_INIT("smack", smack_hooks);
 
 static __init void init_smack_known_list(void)
 {
@@ -4842,7 +4843,7 @@ static __init int smack_init(void)
 	/*
 	 * Register with LSM
 	 */
-	security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
+	security_add_hooks(&smack_info);
 
 	return 0;
 }
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 213b8c593668..e8f08c10e339 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -528,6 +528,8 @@ static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(socket_sendmsg, tomoyo_socket_sendmsg),
 };
 
+static struct lsm_info tomoyo_info = LSM_MODULE_INIT("tomoyo", tomoyo_hooks);
+
 /* Lock for GC. */
 DEFINE_SRCU(tomoyo_ss);
 
@@ -543,7 +545,7 @@ 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_info);
 	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..4f80b7031e20 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -430,6 +430,8 @@ static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(task_free, yama_task_free),
 };
 
+static struct lsm_info yama_info = LSM_MODULE_INIT("yama", yama_hooks);
+
 #ifdef CONFIG_SYSCTL
 static int yama_dointvec_minmax(struct ctl_table *table, int write,
 				void __user *buffer, size_t *lenp, loff_t *ppos)
@@ -480,6 +482,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_info);
 	yama_init_sysctl();
 }
-- 
2.14.1

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

[PATCH v7 2/6] security: Make security_hook_heads private

[Sargun Dhillon]

This is in preparation for future patches. The pointer to the hook
head is no longer embedded in the security_hook_list structure,
and instead, now there is an offset.

In addition, since struct security_hook_heads is a static defined
in security.c, and it's full of hlist_heads, who's initialization
involves setting them to null, there's no more reason for explicit
initialization.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 include/linux/lsm_hooks.h                     | 10 +++++---
 scripts/gcc-plugins/randomize_layout_plugin.c |  2 --
 security/security.c                           | 34 +++++++++++++++++----------
 security/security.h                           |  3 ++-
 4 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 65f346cb6639..93bb0f8a597f 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2014,7 +2014,7 @@ struct lsm_info {
 
 struct security_hook_list {
 	struct hlist_node		list;
-	struct hlist_head		*head;
+	const unsigned int		offset;
 	union security_list_options	hook;
 	/* This field is not currently in use */
 	struct lsm_info			*info;
@@ -2026,8 +2026,13 @@ struct security_hook_list {
  * care of the common case and reduces the amount of
  * text involved.
  */
+#define HOOK_OFFSET(HEAD)	offsetof(struct security_hook_heads, HEAD)
+
 #define LSM_HOOK_INIT(HEAD, HOOK) \
-	{ .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } }
+	{					\
+		.offset	= HOOK_OFFSET(HEAD),	\
+		.hook	= { .HEAD = HOOK }	\
+	}
 
 #define LSM_MODULE_INIT(NAME, HOOKS)		\
 	{					\
@@ -2036,7 +2041,6 @@ struct security_hook_list {
 		.count	= ARRAY_SIZE(HOOKS),	\
 	}
 
-extern struct security_hook_heads security_hook_heads;
 extern void security_add_hooks(struct lsm_info *lsm);
 
 #ifdef CONFIG_SECURITY_SELINUX_DISABLE
diff --git a/scripts/gcc-plugins/randomize_layout_plugin.c b/scripts/gcc-plugins/randomize_layout_plugin.c
index 6d5bbd31db7f..d94138999427 100644
--- a/scripts/gcc-plugins/randomize_layout_plugin.c
+++ b/scripts/gcc-plugins/randomize_layout_plugin.c
@@ -52,8 +52,6 @@ static const struct whitelist_entry whitelist[] = {
 	{ "net/unix/af_unix.c", "unix_skb_parms", "char" },
 	/* big_key payload.data struct splashing */
 	{ "security/keys/big_key.c", "path", "void *" },
-	/* walk struct security_hook_heads as an array of struct hlist_head */
-	{ "security/security.c", "hlist_head", "security_hook_heads" },
 	{ }
 };
 
diff --git a/security/security.c b/security/security.c
index 36b9d2b0a135..acdbf65bd752 100644
--- a/security/security.c
+++ b/security/security.c
@@ -41,9 +41,13 @@
 
 DEFINE_MUTEX(lsm_info_lock);
 struct hlist_head lsm_info_head __lsm_ro_after_init = HLIST_HEAD_INIT;
-struct security_hook_heads security_hook_heads __lsm_ro_after_init;
+static struct security_hook_heads security_hook_heads __lsm_ro_after_init;
 static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
 
+struct security_hook_heads *get_security_hook_heads(void)
+{
+	return &security_hook_heads;
+}
 
 /* Boot-time LSM user choice */
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
@@ -70,12 +74,6 @@ 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");
 
 	/*
@@ -152,6 +150,20 @@ void security_delete_hooks(struct lsm_info *info)
 	mutex_unlock(&lsm_info_lock);
 }
 #endif /* CONFIG_SECURITY_SELINUX_DISABLE */
+
+static void __init security_add_hook(struct security_hook_list *hook,
+					struct lsm_info *info)
+{
+	const unsigned int offset = hook->offset;
+	const unsigned int idx = offset / sizeof(struct hlist_head);
+	struct hlist_head *head;
+
+	WARN_ON(offset % sizeof(struct hlist_head));
+
+	hook->info = info;
+	head = (struct hlist_head *)(&security_hook_heads) + idx;
+	hlist_add_tail_rcu(&hook->list, head);
+}
 /**
  * security_add_hooks - Add a modules hooks to the hook lists.
  * @lsm_info: The lsm_info struct for this security module
@@ -160,14 +172,10 @@ void security_delete_hooks(struct lsm_info *info)
  */
 void __init security_add_hooks(struct lsm_info *info)
 {
-	struct security_hook_list *hook;
 	int i;
 
-	for (i = 0; i < info->count; i++) {
-		hook = &info->hooks[i];
-		hook->info = info;
-		hlist_add_tail_rcu(&hook->list, hook->head);
-	}
+	for (i = 0; i < info->count; i++)
+		security_add_hook(&info->hooks[i], info);
 
 	mutex_lock(&lsm_info_lock);
 	hlist_add_tail_rcu(&info->list, &lsm_info_head);
diff --git a/security/security.h b/security/security.h
index 79d1388fb038..b4d1a60862c3 100644
--- a/security/security.h
+++ b/security/security.h
@@ -5,6 +5,7 @@
 
 #ifndef __SECURITY_SECURITY_H
 #define __SECURITY_SECURITY_H
-extern struct hlist_head lsm_info_head;
 extern struct mutex lsm_info_lock;
+extern struct hlist_head lsm_info_head;
+extern struct security_hook_heads *get_security_hook_heads(void);
 #endif
-- 
2.14.1

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

[PATCH v7 3/6] security: Introduce mutable (RW) hooks

[Sargun Dhillon]

This PR introduces a separate set of immutable, versus mutable security
hooks. This means that modules (like SELinux) can safely unload themselves
after boot time, while not forcing the entire rest of the LSMs to suffer
from the same security issues.

Mutable (RW) hooks are only evaluated after all Immutable (RO) hooks
are successfully run.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 include/linux/lsm_hooks.h  |   9 +--
 security/apparmor/lsm.c    |   4 +-
 security/commoncap.c       |   4 +-
 security/loadpin/loadpin.c |   4 +-
 security/security.c        | 137 +++++++++++++++++++++++++++++++++++++--------
 security/security.h        |   2 +-
 security/selinux/hooks.c   |  12 +++-
 security/smack/smack_lsm.c |   4 +-
 security/tomoyo/tomoyo.c   |   4 +-
 security/yama/yama_lsm.c   |   4 +-
 10 files changed, 137 insertions(+), 47 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 93bb0f8a597f..5846b010de0d 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2041,19 +2041,12 @@ struct security_hook_list {
 		.count	= ARRAY_SIZE(HOOKS),	\
 	}
 
-extern void security_add_hooks(struct lsm_info *lsm);
+extern void security_add_hooks(struct lsm_info *lsm, bool is_mutable);
 
 #ifdef CONFIG_SECURITY_SELINUX_DISABLE
 void security_delete_hooks(struct lsm_info *lsm);
 #endif /* CONFIG_SECURITY_SELINUX_DISABLE */
 
-/* Currently required to handle SELinux runtime hook disable. */
-#ifdef CONFIG_SECURITY_WRITABLE_HOOKS
-#define __lsm_ro_after_init
-#else
-#define __lsm_ro_after_init	__ro_after_init
-#endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
-
 extern int __init security_module_enable(const char *module);
 extern void __init capability_add_hooks(void);
 #ifdef CONFIG_SECURITY_YAMA
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index d0b3f9a6d488..85ad007bcb92 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1117,7 +1117,7 @@ static void apparmor_sock_graft(struct sock *sk, struct socket *parent)
 		ctx->label = aa_get_current_label();
 }
 
-static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
+static struct security_hook_list apparmor_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(ptrace_access_check, apparmor_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, apparmor_ptrace_traceme),
 	LSM_HOOK_INIT(capget, apparmor_capget),
@@ -1564,7 +1564,7 @@ static int __init apparmor_init(void)
 		aa_free_root_ns();
 		goto buffers_out;
 	}
-	security_add_hooks(&apparmor_info);
+	security_add_hooks(&apparmor_info, false);
 
 	/* Report that AppArmor successfully initialized */
 	apparmor_initialized = 1;
diff --git a/security/commoncap.c b/security/commoncap.c
index d25891adda81..6ce5703075d7 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1339,7 +1339,7 @@ int cap_mmap_file(struct file *file, unsigned long reqprot,
 
 #ifdef CONFIG_SECURITY
 
-static struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
+static struct security_hook_list capability_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(capable, cap_capable),
 	LSM_HOOK_INIT(settime, cap_settime),
 	LSM_HOOK_INIT(ptrace_access_check, cap_ptrace_access_check),
@@ -1365,7 +1365,7 @@ static struct lsm_info capability_info =
 
 void __init capability_add_hooks(void)
 {
-	security_add_hooks(&capability_info);
+	security_add_hooks(&capability_info, false);
 }
 
 #endif /* CONFIG_SECURITY */
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index dabc49b855f7..c31907ab9724 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -173,7 +173,7 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
 	return 0;
 }
 
-static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
+static struct security_hook_list loadpin_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
 	LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
 };
@@ -183,7 +183,7 @@ static struct lsm_info loadpin_info = LSM_MODULE_INIT("loadpin", loadpin_hooks);
 void __init loadpin_add_hooks(void)
 {
 	pr_info("ready to pin (currently %sabled)", enabled ? "en" : "dis");
-	security_add_hooks(&loadpin_info);
+	security_add_hooks(&loadpin_info, false);
 }
 
 /* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
diff --git a/security/security.c b/security/security.c
index acdbf65bd752..6b090d53bf9d 100644
--- a/security/security.c
+++ b/security/security.c
@@ -40,14 +40,36 @@
 #define SECURITY_NAME_MAX	10
 
 DEFINE_MUTEX(lsm_info_lock);
-struct hlist_head lsm_info_head __lsm_ro_after_init = HLIST_HEAD_INIT;
-static struct security_hook_heads security_hook_heads __lsm_ro_after_init;
+struct hlist_head lsm_info_head = HLIST_HEAD_INIT;
+static struct security_hook_heads security_hook_heads_ro __ro_after_init;
 static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
 
-struct security_hook_heads *get_security_hook_heads(void)
+#define FOR_EACH_SECURITY_HOOK_RO(ITERATOR, HEAD) \
+	hlist_for_each_entry_rcu(ITERATOR, &security_hook_heads_ro.HEAD, list)
+
+
+#ifdef CONFIG_SECURITY_WRITABLE_HOOKS
+static struct security_hook_heads security_hook_heads_rw;
+
+#define FOR_EACH_SECURITY_HOOK_RW(ITERATOR, HEAD) \
+	hlist_for_each_entry_rcu(ITERATOR, &security_hook_heads_rw.HEAD, list)
+
+struct security_hook_heads *get_security_hook_heads(bool is_mutable)
+{
+	if (is_mutable)
+		return &security_hook_heads_rw;
+	return &security_hook_heads_ro;
+}
+#else
+#define FOR_EACH_SECURITY_HOOK_RW(ITERATOR, HEAD)	while (0)
+
+struct security_hook_heads *get_security_hook_heads(bool is_mutable)
 {
-	return &security_hook_heads;
+	if (is_mutable)
+		return ERR_PTR(-ENOTSUPP);
+	return &security_hook_heads_ro;
 }
+#endif
 
 /* Boot-time LSM user choice */
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
@@ -74,7 +96,8 @@ static void __init do_security_initcalls(void)
  */
 int __init security_init(void)
 {
-	pr_info("Security Framework initialized\n");
+	pr_info("Security Framework initialized with%s writable hooks\n",
+		IS_ENABLED(CONFIG_SECURITY_WRITABLE_HOOKS) ? "" : "out");
 
 	/*
 	 * Load minor LSMs, with the capability module always first.
@@ -152,7 +175,8 @@ void security_delete_hooks(struct lsm_info *info)
 #endif /* CONFIG_SECURITY_SELINUX_DISABLE */
 
 static void __init security_add_hook(struct security_hook_list *hook,
-					struct lsm_info *info)
+					struct lsm_info *info,
+					struct security_hook_heads *heads)
 {
 	const unsigned int offset = hook->offset;
 	const unsigned int idx = offset / sizeof(struct hlist_head);
@@ -161,21 +185,27 @@ static void __init security_add_hook(struct security_hook_list *hook,
 	WARN_ON(offset % sizeof(struct hlist_head));
 
 	hook->info = info;
-	head = (struct hlist_head *)(&security_hook_heads) + idx;
+	head = (struct hlist_head *)(heads) + idx;
 	hlist_add_tail_rcu(&hook->list, head);
 }
 /**
  * security_add_hooks - Add a modules hooks to the hook lists.
  * @lsm_info: The lsm_info struct for this security module
+ * @is_mutable: Can these hooks be loaded or unloaded after boot time
  *
  * Each LSM has to register its hooks with the infrastructure.
  */
-void __init security_add_hooks(struct lsm_info *info)
+void __init security_add_hooks(struct lsm_info *info, bool is_mutable)
 {
+	struct security_hook_heads *heads;
 	int i;
 
+	heads = get_security_hook_heads(is_mutable);
+	if (IS_ERR(heads))
+		panic("Failed to get security heads: %ld\n", PTR_ERR(heads));
+
 	for (i = 0; i < info->count; i++)
-		security_add_hook(&info->hooks[i], info);
+		security_add_hook(&info->hooks[i], info, heads);
 
 	mutex_lock(&lsm_info_lock);
 	hlist_add_tail_rcu(&info->list, &lsm_info_head);
@@ -214,24 +244,41 @@ EXPORT_SYMBOL(unregister_lsm_notifier);
 	do {							\
 		struct security_hook_list *P;			\
 								\
-		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
+		FOR_EACH_SECURITY_HOOK_RO(P, FUNC)		\
+			P->hook.FUNC(__VA_ARGS__);		\
+		FOR_EACH_SECURITY_HOOK_RW(P, FUNC)		\
 			P->hook.FUNC(__VA_ARGS__);		\
 	} while (0)
 
-#define call_int_hook(FUNC, IRC, ...) ({			\
+/*
+ * In certain functions, call_int_hook is invoked multiple times, therefore
+ * the flow-control labels for goto have to be unique with that namespace.
+ * In order to work around this, we wrap call_int_hook2 with a unique-label
+ * defined below in call_int_hook.
+ */
+#define call_int_hook2(LABEL, FUNC, IRC, ...) ({		\
 	int RC = IRC;						\
 	do {							\
 		struct security_hook_list *P;			\
 								\
-		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
+		FOR_EACH_SECURITY_HOOK_RO(P, FUNC) {		\
+			RC = P->hook.FUNC(__VA_ARGS__);		\
+			if (RC != 0)				\
+				goto LABEL;			\
+		}						\
+		FOR_EACH_SECURITY_HOOK_RW(P, FUNC) {		\
 			RC = P->hook.FUNC(__VA_ARGS__);		\
 			if (RC != 0)				\
 				break;				\
 		}						\
 	} while (0);						\
+LABEL:								\
 	RC;							\
 })
 
+#define call_int_hook(FUNC, IRC, ...)	\
+	call_int_hook2(__UNIQUE_ID(FUNC), FUNC, IRC, __VA_ARGS__)
+
 /* Security operations */
 
 int security_binder_set_context_mgr(struct task_struct *mgr)
@@ -330,13 +377,22 @@ 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.
 	 */
-	hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) {
+	FOR_EACH_SECURITY_HOOK_RO(hp, vm_enough_memory) {
+		rc = hp->hook.vm_enough_memory(mm, pages);
+		if (rc <= 0) {
+			cap_sys_admin = 0;
+			goto out;
+		}
+	}
+
+	FOR_EACH_SECURITY_HOOK_RW(hp, vm_enough_memory) {
 		rc = hp->hook.vm_enough_memory(mm, pages);
 		if (rc <= 0) {
 			cap_sys_admin = 0;
 			break;
 		}
 	}
+out:
 	return __vm_enough_memory(mm, pages, cap_sys_admin);
 }
 
@@ -811,38 +867,55 @@ int security_inode_killpriv(struct dentry *dentry)
 int security_inode_getsecurity(struct inode *inode, const char *name, void **buffer, bool alloc)
 {
 	struct security_hook_list *hp;
-	int rc;
+	int rc = -EOPNOTSUPP;
 
 	if (unlikely(IS_PRIVATE(inode)))
 		return -EOPNOTSUPP;
 	/*
 	 * Only one module will provide an attribute with a given name.
 	 */
-	hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
+	FOR_EACH_SECURITY_HOOK_RO(hp, inode_getsecurity) {
+		rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
+		if (rc != -EOPNOTSUPP)
+			goto out;
+	}
+
+	FOR_EACH_SECURITY_HOOK_RW(hp, inode_getsecurity) {
 		rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
 		if (rc != -EOPNOTSUPP)
-			return rc;
+			break;
 	}
-	return -EOPNOTSUPP;
+
+out:
+	return rc;
 }
 
 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;
 
 	if (unlikely(IS_PRIVATE(inode)))
 		return -EOPNOTSUPP;
 	/*
 	 * Only one module will provide an attribute with a given name.
 	 */
-	hlist_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) {
+	FOR_EACH_SECURITY_HOOK_RO(hp, inode_setsecurity) {
+		rc = hp->hook.inode_setsecurity(inode, name, value, size,
+								flags);
+		if (rc != -EOPNOTSUPP)
+			goto out;
+	}
+
+	FOR_EACH_SECURITY_HOOK_RW(hp, inode_setsecurity) {
 		rc = hp->hook.inode_setsecurity(inode, name, value, size,
 								flags);
 		if (rc != -EOPNOTSUPP)
-			return rc;
+			break;
 	}
-	return -EOPNOTSUPP;
+
+out:
+	return rc;
 }
 
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size)
@@ -1146,7 +1219,16 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 	int rc = -ENOSYS;
 	struct security_hook_list *hp;
 
-	hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
+	FOR_EACH_SECURITY_HOOK_RO(hp, task_prctl) {
+		thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
+		if (thisrc != -ENOSYS) {
+			rc = thisrc;
+			if (thisrc != 0)
+				goto out;
+		}
+	}
+
+	FOR_EACH_SECURITY_HOOK_RW(hp, task_prctl) {
 		thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
 		if (thisrc != -ENOSYS) {
 			rc = thisrc;
@@ -1154,6 +1236,8 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 				break;
 		}
 	}
+
+out:
 	return rc;
 }
 
@@ -1671,11 +1755,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,
-				list) {
+	FOR_EACH_SECURITY_HOOK_RO(hp, xfrm_state_pol_flow_match) {
+		rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
+		goto out;
+	}
+
+	FOR_EACH_SECURITY_HOOK_RW(hp, xfrm_state_pol_flow_match) {
 		rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
 		break;
 	}
+out:
 	return rc;
 }
 
diff --git a/security/security.h b/security/security.h
index b4d1a60862c3..3e757f55cc1d 100644
--- a/security/security.h
+++ b/security/security.h
@@ -7,5 +7,5 @@
 #define __SECURITY_SECURITY_H
 extern struct mutex lsm_info_lock;
 extern struct hlist_head lsm_info_head;
-extern struct security_hook_heads *get_security_hook_heads(void);
+extern struct security_hook_heads *get_security_hook_heads(bool is_mutable);
 #endif
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index a6d1bc76340e..b324f03ae723 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6848,7 +6848,13 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
 }
 #endif
 
-static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
+#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+#define __selinux_ro_after_init
+#else
+#define __selinux_ro_after_init	__ro_after_init
+#endif
+
+static struct security_hook_list selinux_hooks[] __selinux_ro_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),
@@ -7087,6 +7093,8 @@ static struct lsm_info selinux_info = LSM_MODULE_INIT("selinux", selinux_hooks);
 
 static __init int selinux_init(void)
 {
+	const bool is_mutable = IS_ENABLED(CONFIG_SECURITY_SELINUX_DISABLE);
+
 	if (!security_module_enable("selinux")) {
 		selinux_enabled = 0;
 		return 0;
@@ -7124,7 +7132,7 @@ static __init int selinux_init(void)
 
 	hashtab_cache_init();
 
-	security_add_hooks(&selinux_info);
+	security_add_hooks(&selinux_info, is_mutable);
 
 	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 a8959cc29bde..86fe6467e0fb 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4623,7 +4623,7 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
 	return 0;
 }
 
-static struct security_hook_list smack_hooks[] __lsm_ro_after_init = {
+static struct security_hook_list smack_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(ptrace_access_check, smack_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, smack_ptrace_traceme),
 	LSM_HOOK_INIT(syslog, smack_syslog),
@@ -4843,7 +4843,7 @@ static __init int smack_init(void)
 	/*
 	 * Register with LSM
 	 */
-	security_add_hooks(&smack_info);
+	security_add_hooks(&smack_info, false);
 
 	return 0;
 }
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index e8f08c10e339..879d358198b8 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -497,7 +497,7 @@ static int tomoyo_socket_sendmsg(struct socket *sock, struct msghdr *msg,
  * tomoyo_security_ops is a "struct security_operations" which is used for
  * registering TOMOYO.
  */
-static struct security_hook_list tomoyo_hooks[] __lsm_ro_after_init = {
+static struct security_hook_list tomoyo_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(cred_alloc_blank, tomoyo_cred_alloc_blank),
 	LSM_HOOK_INIT(cred_prepare, tomoyo_cred_prepare),
 	LSM_HOOK_INIT(cred_transfer, tomoyo_cred_transfer),
@@ -545,7 +545,7 @@ static int __init tomoyo_init(void)
 	if (!security_module_enable("tomoyo"))
 		return 0;
 	/* register ourselves with the security framework */
-	security_add_hooks(&tomoyo_info);
+	security_add_hooks(&tomoyo_info, 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 4f80b7031e20..7f2ce8f7c11f 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -423,7 +423,7 @@ int yama_ptrace_traceme(struct task_struct *parent)
 	return rc;
 }
 
-static struct security_hook_list yama_hooks[] __lsm_ro_after_init = {
+static struct security_hook_list yama_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(ptrace_access_check, yama_ptrace_access_check),
 	LSM_HOOK_INIT(ptrace_traceme, yama_ptrace_traceme),
 	LSM_HOOK_INIT(task_prctl, yama_task_prctl),
@@ -482,6 +482,6 @@ static inline void yama_init_sysctl(void) { }
 void __init yama_add_hooks(void)
 {
 	pr_info("Yama: becoming mindful.\n");
-	security_add_hooks(&yama_info);
+	security_add_hooks(&yama_info, false);
 	yama_init_sysctl();
 }
-- 
2.14.1

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

[PATCH v7 4/6] security: Expose security_add_hooks externally and add error handling

[Sargun Dhillon]

This exports security_add_hooks. In order to safely export it, we need
have proper error handling for when mutable hooks are not available,
rather than crashing the kernel.

It now returns an error if it's unable to get a lock to install
hooks. For built-in modules, this should never happen, so by
default, they crash the kernel. It is up to 3rd party module
authors to handle this case for their own modules.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 include/linux/lsm_hooks.h  |  7 +++++--
 security/apparmor/lsm.c    |  2 +-
 security/commoncap.c       |  2 +-
 security/loadpin/loadpin.c |  2 +-
 security/security.c        | 23 ++++++++++++++++-------
 security/selinux/hooks.c   |  2 +-
 security/smack/smack_lsm.c |  2 +-
 security/tomoyo/tomoyo.c   |  2 +-
 security/yama/yama_lsm.c   |  2 +-
 9 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 5846b010de0d..c794c8f2f8b9 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -28,7 +28,7 @@
 #include <linux/security.h>
 #include <linux/init.h>
 #include <linux/rculist.h>
-
+#include <linux/module.h>
 /**
  * union security_list_options - Linux Security Module hook function list
  *
@@ -2010,6 +2010,7 @@ struct lsm_info {
 	const char			*name;
 	const unsigned int		count;
 	struct security_hook_list	*hooks;
+	struct module			*owner;
 } __randomize_layout;
 
 struct security_hook_list {
@@ -2039,9 +2040,11 @@ struct security_hook_list {
 		.name	= NAME,			\
 		.hooks	= HOOKS,		\
 		.count	= ARRAY_SIZE(HOOKS),	\
+		.owner	= THIS_MODULE,		\
 	}
 
-extern void security_add_hooks(struct lsm_info *lsm, bool is_mutable);
+extern int __must_check security_add_hooks(struct lsm_info *lsm,
+						bool is_mutable);
 
 #ifdef CONFIG_SECURITY_SELINUX_DISABLE
 void security_delete_hooks(struct lsm_info *lsm);
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 85ad007bcb92..6477e9b088b1 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1564,7 +1564,7 @@ static int __init apparmor_init(void)
 		aa_free_root_ns();
 		goto buffers_out;
 	}
-	security_add_hooks(&apparmor_info, false);
+	BUG_ON(security_add_hooks(&apparmor_info, false));
 
 	/* Report that AppArmor successfully initialized */
 	apparmor_initialized = 1;
diff --git a/security/commoncap.c b/security/commoncap.c
index 6ce5703075d7..3739e11d02e9 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1365,7 +1365,7 @@ static struct lsm_info capability_info =
 
 void __init capability_add_hooks(void)
 {
-	security_add_hooks(&capability_info, false);
+	BUG_ON(security_add_hooks(&capability_info, false));
 }
 
 #endif /* CONFIG_SECURITY */
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index c31907ab9724..b0a357034736 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -183,7 +183,7 @@ static struct lsm_info loadpin_info = LSM_MODULE_INIT("loadpin", loadpin_hooks);
 void __init loadpin_add_hooks(void)
 {
 	pr_info("ready to pin (currently %sabled)", enabled ? "en" : "dis");
-	security_add_hooks(&loadpin_info, false);
+	BUG_ON(security_add_hooks(&loadpin_info, false));
 }
 
 /* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
diff --git a/security/security.c b/security/security.c
index 6b090d53bf9d..8d8a227eeeea 100644
--- a/security/security.c
+++ b/security/security.c
@@ -174,9 +174,9 @@ void security_delete_hooks(struct lsm_info *info)
 }
 #endif /* CONFIG_SECURITY_SELINUX_DISABLE */
 
-static void __init security_add_hook(struct security_hook_list *hook,
-					struct lsm_info *info,
-					struct security_hook_heads *heads)
+static void security_add_hook(struct security_hook_list *hook,
+				struct lsm_info *info,
+				struct security_hook_heads *heads)
 {
 	const unsigned int offset = hook->offset;
 	const unsigned int idx = offset / sizeof(struct hlist_head);
@@ -194,23 +194,32 @@ static void __init security_add_hook(struct security_hook_list *hook,
  * @is_mutable: Can these hooks be loaded or unloaded after boot time
  *
  * Each LSM has to register its hooks with the infrastructure.
+ * Return 0 on success
  */
-void __init security_add_hooks(struct lsm_info *info, bool is_mutable)
+int __must_check security_add_hooks(struct lsm_info *info, bool is_mutable)
 {
 	struct security_hook_heads *heads;
 	int i;
 
 	heads = get_security_hook_heads(is_mutable);
 	if (IS_ERR(heads))
-		panic("Failed to get security heads: %ld\n", PTR_ERR(heads));
+		return PTR_ERR(heads);
+	if (!try_module_get(info->owner))
+		return -EINVAL;
+
+	if (mutex_lock_killable(&lsm_info_lock)) {
+		module_put(info->owner);
+		return -EINTR;
+	}
 
 	for (i = 0; i < info->count; i++)
 		security_add_hook(&info->hooks[i], info, heads);
-
-	mutex_lock(&lsm_info_lock);
 	hlist_add_tail_rcu(&info->list, &lsm_info_head);
 	mutex_unlock(&lsm_info_lock);
+
+	return 0;
 }
+EXPORT_SYMBOL_GPL(security_add_hooks);
 
 int call_lsm_notifier(enum lsm_event event, void *data)
 {
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index b324f03ae723..fb830dc80e15 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -7132,7 +7132,7 @@ static __init int selinux_init(void)
 
 	hashtab_cache_init();
 
-	security_add_hooks(&selinux_info, is_mutable);
+	BUG_ON(security_add_hooks(&selinux_info, is_mutable));
 
 	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 86fe6467e0fb..0844776c4d18 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4843,7 +4843,7 @@ static __init int smack_init(void)
 	/*
 	 * Register with LSM
 	 */
-	security_add_hooks(&smack_info, false);
+	BUG_ON(security_add_hooks(&smack_info, false));
 
 	return 0;
 }
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 879d358198b8..4b08021338dc 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -545,7 +545,7 @@ static int __init tomoyo_init(void)
 	if (!security_module_enable("tomoyo"))
 		return 0;
 	/* register ourselves with the security framework */
-	security_add_hooks(&tomoyo_info, false);
+	BUG_ON(security_add_hooks(&tomoyo_info, 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 7f2ce8f7c11f..06dd2eb5d0ca 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -482,6 +482,6 @@ static inline void yama_init_sysctl(void) { }
 void __init yama_add_hooks(void)
 {
 	pr_info("Yama: becoming mindful.\n");
-	security_add_hooks(&yama_info, false);
+	BUG_ON(security_add_hooks(&yama_info, false));
 	yama_init_sysctl();
 }
-- 
2.14.1

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

[PATCH v7 5/6] security: Panic on forced unloading of security module

[Sargun Dhillon]

Although, when LSMs are loaded, the kernel takes a refcount on them, the
administrator can force unload the module if the
CONFIG_MODULE_FORCE_UNLOAD is set. Although this may be fine for some
cases, in the case of security modules, this is problematic, as it
may leave the system unsecure, or unaudited.

Although, a kernel panic will occur on the next attempt to make a
callback for that hook, new code could be loaded, which would not
trigger a panic, allowing for silent failure. Therefore, we must
panic on an attempt to forcefully unload an LSM.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 security/security.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/security/security.c b/security/security.c
index 8d8a227eeeea..204b9978ba24 100644
--- a/security/security.c
+++ b/security/security.c
@@ -89,6 +89,34 @@ static void __init do_security_initcalls(void)
 	}
 }
 
+/*
+ * Check if one of our modules is being unloaded. This can happen if
+ * CONFIG_MODULE_FORCE_UNLOAD is enabled.
+ * If it is being unloaded, panic and let the user know what's going on
+ */
+static int security_module_cb(struct notifier_block *nb, unsigned long val,
+				void *data)
+{
+	struct module *mod = data;
+	struct lsm_info *info;
+
+	if (val != MODULE_STATE_GOING)
+		return NOTIFY_DONE;
+
+	mutex_lock(&lsm_info_lock);
+	hlist_for_each_entry(info, &lsm_info_head, list)
+		if (mod == info->owner)
+			panic("Security module %s is being unloaded forcefully\n",
+				info->name);
+	mutex_unlock(&lsm_info_lock);
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block security_nb = {
+	.notifier_call	= security_module_cb,
+};
+
 /**
  * security_init - initializes the security framework
  *
@@ -99,6 +127,9 @@ int __init security_init(void)
 	pr_info("Security Framework initialized with%s writable hooks\n",
 		IS_ENABLED(CONFIG_SECURITY_WRITABLE_HOOKS) ? "" : "out");
 
+	if (IS_ENABLED(CONFIG_SECURITY_WRITABLE_HOOKS) &&
+		IS_ENABLED(CONFIG_MODULE_FORCE_UNLOAD))
+		register_module_notifier(&security_nb);
 	/*
 	 * Load minor LSMs, with the capability module always first.
 	 */
-- 
2.14.1

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

[PATCH v7 6/6] security: Add SECURITY_UNREGISTRABLE_HOOKS to allow for hook removal

[Sargun Dhillon]

This enables safe security hook module unloading, and long with code
unloading. It also comes with a knob to prevent unloading modules
at runtime, or after boot time, in case the administrator wants
to lock down their system as such.

Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
 include/linux/lsm_hooks.h |   5 +-
 security/Kconfig          |  12 +++
 security/security.c       | 225 +++++++++++++++++++++++++++++++++++++---------
 security/selinux/hooks.c  |   2 +-
 4 files changed, 199 insertions(+), 45 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index c794c8f2f8b9..5c227bbb2883 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2047,8 +2047,11 @@ extern int __must_check security_add_hooks(struct lsm_info *lsm,
 						bool is_mutable);
 
 #ifdef CONFIG_SECURITY_SELINUX_DISABLE
-void security_delete_hooks(struct lsm_info *lsm);
+void __security_delete_hooks(struct lsm_info *lsm);
 #endif /* CONFIG_SECURITY_SELINUX_DISABLE */
+#ifdef CONFIG_SECURITY_UNREGISTRABLE_HOOKS
+void security_delete_hooks(struct lsm_info *lsm);
+#endif /* CONFIG_SECURITY_UNREGISTRABLE_HOOKS */
 
 extern int __init security_module_enable(const char *module);
 extern void __init capability_add_hooks(void);
diff --git a/security/Kconfig b/security/Kconfig
index c4302067a3ad..c7039f625241 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -36,6 +36,18 @@ config SECURITY_WRITABLE_HOOKS
 	bool
 	default n
 
+config SECURITY_UNREGISTRABLE_HOOKS
+	depends on SECURITY_WRITABLE_HOOKS && SRCU
+	default n
+	bool "Enable security module unloading"
+	help
+		This allows arbitrary security modules to be unloaded by the
+		administrator after boot time. After boot time, this behaviour
+		can still be disabled via sysfs, or disabled at boot time via
+		a kernel parameter.
+
+		If you are unsure how to answer this question, answer N.
+
 config SECURITYFS
 	bool "Enable the securityfs filesystem"
 	help
diff --git a/security/security.c b/security/security.c
index 204b9978ba24..36065128c6c5 100644
--- a/security/security.c
+++ b/security/security.c
@@ -29,6 +29,8 @@
 #include <linux/backing-dev.h>
 #include <linux/string.h>
 #include <linux/mutex.h>
+#include <linux/srcu.h>
+#include <linux/atomic.h>
 #include <net/flow.h>
 #include "security.h"
 
@@ -44,10 +46,44 @@ struct hlist_head lsm_info_head = HLIST_HEAD_INIT;
 static struct security_hook_heads security_hook_heads_ro __ro_after_init;
 static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
 
+/*
+ * security_allow_unregister_hooks blocks the delete_module syscall for
+ * hooks that are loaded into the  set of mutable hooks by getting a reference
+ * on those modules. This allows built-in modules to still delete their security
+ * hooks, so SELinux will still be able to deregister.
+ *
+ * If an arbitrary module tries to deregister, it will get a return code
+ * indicating failure.
+ *
+ * When this value turns true -> false -- Once, lock_existing_hooks will be
+ * called.
+ */
+static atomic_t security_allow_unregister_hooks =
+	ATOMIC_INIT(IS_ENABLED(CONFIG_SECURITY_UNREGISTRABLE_HOOKS) ? 1 : 0);
+
 #define FOR_EACH_SECURITY_HOOK_RO(ITERATOR, HEAD) \
 	hlist_for_each_entry_rcu(ITERATOR, &security_hook_heads_ro.HEAD, list)
 
 
+#if IS_ENABLED(CONFIG_SECURITY_SELINUX_DISABLE) || \
+	IS_ENABLED(CONFIG_SECURITY_UNREGISTRABLE_HOOKS)
+/* This should only be used by SELinux directly */
+void __security_delete_hooks(struct lsm_info *info)
+{
+	struct security_hook_list *hook;
+	int i;
+
+	for (i = 0; i < info->count; i++) {
+		hook = &info->hooks[i];
+		hlist_del_rcu(&hook->list);
+	}
+
+	mutex_lock(&lsm_info_lock);
+	hlist_del(&info->list);
+	mutex_unlock(&lsm_info_lock);
+}
+#endif
+
 #ifdef CONFIG_SECURITY_WRITABLE_HOOKS
 static struct security_hook_heads security_hook_heads_rw;
 
@@ -71,6 +107,111 @@ struct security_hook_heads *get_security_hook_heads(bool is_mutable)
 }
 #endif
 
+#ifdef CONFIG_SECURITY_UNREGISTRABLE_HOOKS
+DEFINE_STATIC_SRCU(security_hook_srcu);
+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);
+}
+
+/**
+ * security_delete_hooks - Remove modules hooks from the mutable hooks lists.
+ * @lsm_info: The lsm_info struct for this security module
+ *
+ * If LSM unloading is disabled, this function will panic. It should not
+ * get called because we should have a refcount for the module.
+ */
+void security_delete_hooks(struct lsm_info *info)
+{
+	if (!atomic_read(&security_allow_unregister_hooks))
+		panic("Security module %s is attempting to delete hooks.\n",
+			info->name);
+
+	__security_delete_hooks(info);
+
+	synchronize_srcu(&security_hook_srcu);
+}
+EXPORT_SYMBOL_GPL(security_delete_hooks);
+
+static void lock_existing_hooks(void)
+{
+	struct lsm_info *info;
+
+	/*
+	 * Prevent module unloading while we're doing this
+	 * try_module_get may fail (safely), if the module
+	 * is already unloading -- allow that.
+	 */
+	mutex_lock(&module_mutex);
+	mutex_lock(&lsm_info_lock);
+	pr_info("Locking security hooks\n");
+
+	hlist_for_each_entry(info, &lsm_info_head, list)
+		WARN_ON(!try_module_get(info->owner));
+
+	mutex_unlock(&lsm_info_lock);
+	mutex_unlock(&module_mutex);
+}
+
+static int allow_unregister_hooks_set(const char *val,
+					const struct kernel_param *kp)
+{
+	const int current_val = atomic_read(&security_allow_unregister_hooks);
+	bool new_val;
+	int ret;
+
+	ret = strtobool(val, &new_val);
+	if (ret)
+		return 0;
+
+	/* Noop */
+	if (!!new_val == !!current_val)
+		return 0;
+
+	/* Do not allow for the transition from false -> true */
+	if (new_val)
+		return -EPERM;
+
+	/* The only legal transition true -> false */
+	if (atomic_cmpxchg(&security_allow_unregister_hooks, 1, 0) == 1)
+		lock_existing_hooks();
+
+	return 0;
+}
+
+static int allow_unregister_hooks_get(char *buffer,
+					const struct kernel_param *kp)
+{
+	const int current_val = atomic_read(&security_allow_unregister_hooks);
+	int ret;
+
+	ret = sprintf(buffer, "%c\n", current_val ? '1' : '0');
+
+	return ret;
+}
+
+static struct kernel_param_ops allow_unregister_hooks_param_ops = {
+	.set	= allow_unregister_hooks_set,
+	.get	= allow_unregister_hooks_get,
+};
+
+module_param_cb(allow_unregister_hooks, &allow_unregister_hooks_param_ops, NULL,
+		0644);
+MODULE_PARM_DESC(allow_unregister_hooks, "Allow deregistering security hooks");
+#else
+static inline int lock_lsm(void)
+{
+	return 0;
+}
+
+static inline void unlock_lsm(int idx) {}
+#endif
+
 /* Boot-time LSM user choice */
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
 	CONFIG_DEFAULT_SECURITY;
@@ -90,9 +231,9 @@ static void __init do_security_initcalls(void)
 }
 
 /*
- * Check if one of our modules is being unloaded. This can happen if
- * CONFIG_MODULE_FORCE_UNLOAD is enabled.
- * If it is being unloaded, panic and let the user know what's going on
+ * Check if one of our modules is being unloaded without the hooks being
+ * unregistered. If things have gone correctly, the LSM should already
+ * have been removed from lsm_info_head.
  */
 static int security_module_cb(struct notifier_block *nb, unsigned long val,
 				void *data)
@@ -106,7 +247,7 @@ static int security_module_cb(struct notifier_block *nb, unsigned long val,
 	mutex_lock(&lsm_info_lock);
 	hlist_for_each_entry(info, &lsm_info_head, list)
 		if (mod == info->owner)
-			panic("Security module %s is being unloaded forcefully\n",
+			panic("Security module %s is being unloaded without deregistering hooks",
 				info->name);
 	mutex_unlock(&lsm_info_lock);
 
@@ -117,6 +258,7 @@ static struct notifier_block security_nb = {
 	.notifier_call	= security_module_cb,
 };
 
+
 /**
  * security_init - initializes the security framework
  *
@@ -127,8 +269,7 @@ int __init security_init(void)
 	pr_info("Security Framework initialized with%s writable hooks\n",
 		IS_ENABLED(CONFIG_SECURITY_WRITABLE_HOOKS) ? "" : "out");
 
-	if (IS_ENABLED(CONFIG_SECURITY_WRITABLE_HOOKS) &&
-		IS_ENABLED(CONFIG_MODULE_FORCE_UNLOAD))
+	if (IS_ENABLED(CONFIG_SECURITY_WRITABLE_HOOKS))
 		register_module_notifier(&security_nb);
 	/*
 	 * Load minor LSMs, with the capability module always first.
@@ -176,35 +317,6 @@ int __init security_module_enable(const char *module)
 	return !strcmp(module, chosen_lsm);
 }
 
-#ifdef CONFIG_SECURITY_SELINUX_DISABLE
-/*
- * Assuring the safety of deleting a security module is up to
- * the security module involved. This may entail ordering the
- * module's hook list in a particular way, refusing to disable
- * the module once a policy is loaded or any number of other
- * actions better imagined than described.
- *
- * The name of the configuration option reflects the only module
- * that currently uses the mechanism. Any developer who thinks
- * disabling their module is a good idea needs to be at least as
- * careful as the SELinux team.
- */
-void security_delete_hooks(struct lsm_info *info)
-{
-	struct security_hook_list *hook;
-	int i;
-
-	for (i = 0; i < info->count; i++) {
-		hook = &info->hooks[i];
-		hlist_del_rcu(&hook->list);
-	}
-
-	mutex_lock(&lsm_info_lock);
-	hlist_del(&info->list);
-	mutex_unlock(&lsm_info_lock);
-}
-#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
-
 static void security_add_hook(struct security_hook_list *hook,
 				struct lsm_info *info,
 				struct security_hook_heads *heads)
@@ -230,25 +342,33 @@ static void security_add_hook(struct security_hook_list *hook,
 int __must_check security_add_hooks(struct lsm_info *info, bool is_mutable)
 {
 	struct security_hook_heads *heads;
-	int i;
+	int i, ret = 0;
 
 	heads = get_security_hook_heads(is_mutable);
 	if (IS_ERR(heads))
 		return PTR_ERR(heads);
-	if (!try_module_get(info->owner))
-		return -EINVAL;
 
-	if (mutex_lock_killable(&lsm_info_lock)) {
-		module_put(info->owner);
+	if (mutex_lock_killable(&lsm_info_lock))
 		return -EINTR;
+
+	if (!atomic_read(&security_allow_unregister_hooks)) {
+		/*
+		 * Because hook deregistration is not allowed, we need to try
+		 * to get a refcount on the module owner.
+		 */
+		if (!try_module_get(info->owner)) {
+			ret = -EINVAL;
+			goto out;
+		}
 	}
 
 	for (i = 0; i < info->count; i++)
 		security_add_hook(&info->hooks[i], info, heads);
 	hlist_add_tail_rcu(&info->list, &lsm_info_head);
+out:
 	mutex_unlock(&lsm_info_lock);
 
-	return 0;
+	return ret;
 }
 EXPORT_SYMBOL_GPL(security_add_hooks);
 
@@ -283,11 +403,14 @@ EXPORT_SYMBOL(unregister_lsm_notifier);
 #define call_void_hook(FUNC, ...)				\
 	do {							\
 		struct security_hook_list *P;			\
+		int srcu_idx;					\
 								\
 		FOR_EACH_SECURITY_HOOK_RO(P, FUNC)		\
 			P->hook.FUNC(__VA_ARGS__);		\
+		srcu_idx = lock_lsm();				\
 		FOR_EACH_SECURITY_HOOK_RW(P, FUNC)		\
 			P->hook.FUNC(__VA_ARGS__);		\
+		unlock_lsm(srcu_idx);				\
 	} while (0)
 
 /*
@@ -300,17 +423,20 @@ EXPORT_SYMBOL(unregister_lsm_notifier);
 	int RC = IRC;						\
 	do {							\
 		struct security_hook_list *P;			\
+		int srcu_idx;					\
 								\
 		FOR_EACH_SECURITY_HOOK_RO(P, FUNC) {		\
 			RC = P->hook.FUNC(__VA_ARGS__);		\
 			if (RC != 0)				\
 				goto LABEL;			\
 		}						\
+		srcu_idx = lock_lsm();				\
 		FOR_EACH_SECURITY_HOOK_RW(P, FUNC) {		\
 			RC = P->hook.FUNC(__VA_ARGS__);		\
 			if (RC != 0)				\
 				break;				\
 		}						\
+		unlock_lsm(srcu_idx);				\
 	} while (0);						\
 LABEL:								\
 	RC;							\
@@ -408,7 +534,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, rc;
 
 	/*
 	 * The module will respond with a positive value if
@@ -425,6 +551,7 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 		}
 	}
 
+	srcu_idx = lock_lsm();
 	FOR_EACH_SECURITY_HOOK_RW(hp, vm_enough_memory) {
 		rc = hp->hook.vm_enough_memory(mm, pages);
 		if (rc <= 0) {
@@ -432,6 +559,7 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 			break;
 		}
 	}
+	unlock_lsm(srcu_idx);
 out:
 	return __vm_enough_memory(mm, pages, cap_sys_admin);
 }
@@ -908,6 +1036,7 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf
 {
 	struct security_hook_list *hp;
 	int rc = -EOPNOTSUPP;
+	int srcu_idx;
 
 	if (unlikely(IS_PRIVATE(inode)))
 		return -EOPNOTSUPP;
@@ -920,11 +1049,13 @@ int security_inode_getsecurity(struct inode *inode, const char *name, void **buf
 			goto out;
 	}
 
+	srcu_idx = lock_lsm();
 	FOR_EACH_SECURITY_HOOK_RW(hp, inode_getsecurity) {
 		rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
 		if (rc != -EOPNOTSUPP)
 			break;
 	}
+	unlock_lsm(srcu_idx);
 
 out:
 	return rc;
@@ -934,6 +1065,7 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
 {
 	struct security_hook_list *hp;
 	int rc = -EOPNOTSUPP;
+	int srcu_idx;
 
 	if (unlikely(IS_PRIVATE(inode)))
 		return -EOPNOTSUPP;
@@ -947,12 +1079,14 @@ int security_inode_setsecurity(struct inode *inode, const char *name, const void
 			goto out;
 	}
 
+	srcu_idx = lock_lsm();
 	FOR_EACH_SECURITY_HOOK_RW(hp, inode_setsecurity) {
 		rc = hp->hook.inode_setsecurity(inode, name, value, size,
 								flags);
 		if (rc != -EOPNOTSUPP)
 			break;
 	}
+	unlock_lsm(srcu_idx);
 
 out:
 	return rc;
@@ -1256,6 +1390,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 			 unsigned long arg4, unsigned long arg5)
 {
 	int thisrc;
+	int srcu_idx;
 	int rc = -ENOSYS;
 	struct security_hook_list *hp;
 
@@ -1268,6 +1403,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 		}
 	}
 
+	srcu_idx = lock_lsm();
 	FOR_EACH_SECURITY_HOOK_RW(hp, task_prctl) {
 		thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
 		if (thisrc != -ENOSYS) {
@@ -1276,6 +1412,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 				break;
 		}
 	}
+	unlock_lsm(srcu_idx);
 
 out:
 	return rc;
@@ -1784,7 +1921,7 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
 				       const struct flowi *fl)
 {
 	struct security_hook_list *hp;
-	int rc = 1;
+	int srcu_idx, rc = 1;
 
 	/*
 	 * Since this function is expected to return 0 or 1, the judgment
@@ -1800,10 +1937,12 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
 		goto out;
 	}
 
+	srcu_idx = lock_lsm();
 	FOR_EACH_SECURITY_HOOK_RW(hp, xfrm_state_pol_flow_match) {
 		rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
 		break;
 	}
+	unlock_lsm(srcu_idx);
 out:
 	return rc;
 }
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index fb830dc80e15..a6a4c7218282 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -7279,7 +7279,7 @@ int selinux_disable(struct selinux_state *state)
 
 	selinux_enabled = 0;
 
-	security_delete_hooks(&selinux_info);
+	__security_delete_hooks(&selinux_info);
 
 	/* Try to destroy the avc node cache */
 	avc_disable();
-- 
2.14.1

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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Tetsuo Handa]

Suggested changes on top of your series.

But I think that your series still lacks critical code which is also not yet
implemented in Casey's work. The reason call_int_hook() can work for hooks
which change state (e.g. security_inode_alloc()) is that there is no need to
call undo function because a hook which might change state (so-called Major LSM
modules) is always the last entry of the list. If we allow runtime registration
of LSM modules, there is a possibility that call_int_hook() returns an error
after some LSM module allocated memory. You need to implement safe transaction
in order to allow FOR_EACH_SECURITY_HOOK_RW hooks to return an error.

---
 include/linux/lsm_hooks.h |  4 +--
 security/security.c       | 86 +++++++++++++----------------------------------
 security/security.h       |  5 ---
 3 files changed, 25 insertions(+), 70 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 5c227bb..98809d6 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2010,7 +2010,7 @@ struct lsm_info {
 	const char			*name;
 	const unsigned int		count;
 	struct security_hook_list	*hooks;
-	struct module			*owner;
+	const struct module		*owner;
 } __randomize_layout;
 
 struct security_hook_list {
@@ -2044,7 +2044,7 @@ struct security_hook_list {
 	}
 
 extern int __must_check security_add_hooks(struct lsm_info *lsm,
-						bool is_mutable);
+					   bool is_writable);
 
 #ifdef CONFIG_SECURITY_SELINUX_DISABLE
 void __security_delete_hooks(struct lsm_info *lsm);
diff --git a/security/security.c b/security/security.c
index 3606512..a4bc9cd 100644
--- a/security/security.c
+++ b/security/security.c
@@ -48,18 +48,15 @@
 
 /*
  * security_allow_unregister_hooks blocks the delete_module syscall for
- * hooks that are loaded into the  set of mutable hooks by getting a reference
+ * hooks that are loaded into the set of writable hooks by getting a reference
  * on those modules. This allows built-in modules to still delete their security
  * hooks, so SELinux will still be able to deregister.
  *
  * If an arbitrary module tries to deregister, it will get a return code
  * indicating failure.
- *
- * When this value turns true -> false -- Once, lock_existing_hooks will be
- * called.
  */
-static atomic_t security_allow_unregister_hooks =
-	ATOMIC_INIT(IS_ENABLED(CONFIG_SECURITY_UNREGISTRABLE_HOOKS) ? 1 : 0);
+static bool security_allow_unregister_hooks =
+	IS_ENABLED(CONFIG_SECURITY_UNREGISTRABLE_HOOKS) ? 1 : 0;
 
 #define FOR_EACH_SECURITY_HOOK_RO(ITERATOR, HEAD) \
 	hlist_for_each_entry_rcu(ITERATOR, &security_hook_heads_ro.HEAD, list)
@@ -90,18 +87,18 @@ void __security_delete_hooks(struct lsm_info *info)
 #define FOR_EACH_SECURITY_HOOK_RW(ITERATOR, HEAD) \
 	hlist_for_each_entry_rcu(ITERATOR, &security_hook_heads_rw.HEAD, list)
 
-struct security_hook_heads *get_security_hook_heads(bool is_mutable)
+static struct security_hook_heads *get_security_hook_heads(bool is_writable)
 {
-	if (is_mutable)
+	if (is_writable)
 		return &security_hook_heads_rw;
 	return &security_hook_heads_ro;
 }
 #else
 #define FOR_EACH_SECURITY_HOOK_RW(ITERATOR, HEAD)	while (0)
 
-struct security_hook_heads *get_security_hook_heads(bool is_mutable)
+static struct security_hook_heads *get_security_hook_heads(bool is_writable)
 {
-	if (is_mutable)
+	if (is_writable)
 		return ERR_PTR(-ENOTSUPP);
 	return &security_hook_heads_ro;
 }
@@ -120,7 +117,7 @@ static inline void unlock_lsm(int idx)
 }
 
 /**
- * security_delete_hooks - Remove modules hooks from the mutable hooks lists.
+ * security_delete_hooks - Remove modules hooks from the writable hooks lists.
  * @lsm_info: The lsm_info struct for this security module
  *
  * If LSM unloading is disabled, this function will panic. It should not
@@ -128,9 +125,8 @@ static inline void unlock_lsm(int idx)
  */
 void security_delete_hooks(struct lsm_info *info)
 {
-	if (!atomic_read(&security_allow_unregister_hooks))
-		panic("Security module %s is attempting to delete hooks.\n",
-			info->name);
+	if (!security_allow_unregister_hooks)
+		panic("Security hooks are already locked.\n");
 
 	__security_delete_hooks(info);
 
@@ -138,39 +134,17 @@ void security_delete_hooks(struct lsm_info *info)
 }
 EXPORT_SYMBOL_GPL(security_delete_hooks);
 
-static void lock_existing_hooks(void)
-{
-	struct lsm_info *info;
-
-	/*
-	 * Prevent module unloading while we're doing this
-	 * try_module_get may fail (safely), if the module
-	 * is already unloading -- allow that.
-	 */
-	mutex_lock(&module_mutex);
-	mutex_lock(&lsm_info_lock);
-	pr_info("Locking security hooks\n");
-
-	hlist_for_each_entry(info, &lsm_info_head, list)
-		WARN_ON(!try_module_get(info->owner));
-
-	mutex_unlock(&lsm_info_lock);
-	mutex_unlock(&module_mutex);
-}
-
 static int allow_unregister_hooks_set(const char *val,
 					const struct kernel_param *kp)
 {
-	const int current_val = atomic_read(&security_allow_unregister_hooks);
+	const bool current_val = security_allow_unregister_hooks;
 	bool new_val;
-	int ret;
 
-	ret = strtobool(val, &new_val);
-	if (ret)
+	if (strtobool(val, &new_val))
 		return 0;
 
 	/* Noop */
-	if (!!new_val == !!current_val)
+	if (new_val == current_val)
 		return 0;
 
 	/* Do not allow for the transition from false -> true */
@@ -178,16 +152,15 @@ static int allow_unregister_hooks_set(const char *val,
 		return -EPERM;
 
 	/* The only legal transition true -> false */
-	if (atomic_cmpxchg(&security_allow_unregister_hooks, 1, 0) == 1)
-		lock_existing_hooks();
-
+	if (!xchg(&security_allow_unregister_hooks, 1))
+		pr_info("Locked security hooks.\n");
 	return 0;
 }
 
 static int allow_unregister_hooks_get(char *buffer,
 					const struct kernel_param *kp)
 {
-	const int current_val = atomic_read(&security_allow_unregister_hooks);
+	const bool current_val = security_allow_unregister_hooks;
 	int ret;
 
 	ret = sprintf(buffer, "%c\n", current_val ? '1' : '0');
@@ -241,14 +214,13 @@ static int security_module_cb(struct notifier_block *nb, unsigned long val,
 	struct module *mod = data;
 	struct lsm_info *info;
 
-	if (val != MODULE_STATE_GOING)
+	if (val != MODULE_STATE_GOING || security_allow_unregister_hooks)
 		return NOTIFY_DONE;
 
 	mutex_lock(&lsm_info_lock);
 	hlist_for_each_entry(info, &lsm_info_head, list)
 		if (mod == info->owner)
-			panic("Security module %s is being unloaded without deregistering hooks",
-				info->name);
+			panic("Security hooks are already locked.\n");
 	mutex_unlock(&lsm_info_lock);
 
 	return NOTIFY_DONE;
@@ -334,41 +306,29 @@ static void security_add_hook(struct security_hook_list *hook,
 /**
  * security_add_hooks - Add a modules hooks to the hook lists.
  * @lsm_info: The lsm_info struct for this security module
- * @is_mutable: Can these hooks be loaded or unloaded after boot time
+ * @is_writable: Can these hooks be loaded or unloaded after boot time
  *
  * Each LSM has to register its hooks with the infrastructure.
  * Return 0 on success
  */
-int __must_check security_add_hooks(struct lsm_info *info, bool is_mutable)
+int __must_check security_add_hooks(struct lsm_info *info, bool is_writable)
 {
 	struct security_hook_heads *heads;
-	int i, ret = 0;
+	int i;
 
-	heads = get_security_hook_heads(is_mutable);
+	heads = get_security_hook_heads(is_writable);
 	if (IS_ERR(heads))
 		return PTR_ERR(heads);
 
 	if (mutex_lock_killable(&lsm_info_lock))
 		return -EINTR;
 
-	if (!atomic_read(&security_allow_unregister_hooks)) {
-		/*
-		 * Because hook deregistration is not allowed, we need to try
-		 * to get a refcount on the module owner.
-		 */
-		if (!try_module_get(info->owner)) {
-			ret = -EINVAL;
-			goto out;
-		}
-	}
-
 	for (i = 0; i < info->count; i++)
 		security_add_hook(&info->hooks[i], info, heads);
 	hlist_add_tail_rcu(&info->list, &lsm_info_head);
-out:
 	mutex_unlock(&lsm_info_lock);
 
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(security_add_hooks);
 
diff --git a/security/security.h b/security/security.h
index 3e757f5..ed056d5 100644
--- a/security/security.h
+++ b/security/security.h
@@ -1,11 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 #include <linux/mutex.h>
 #include <linux/list.h>
-#include <linux/lsm_hooks.h>
 
-#ifndef __SECURITY_SECURITY_H
-#define __SECURITY_SECURITY_H
 extern struct mutex lsm_info_lock;
 extern struct hlist_head lsm_info_head;
-extern struct security_hook_heads *get_security_hook_heads(bool is_mutable);
-#endif
-- 
1.8.3.1

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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Sargun Dhillon]

On Thu, Apr 26, 2018 at 12:15 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Suggested changes on top of your series.
>
> But I think that your series still lacks critical code which is also not yet
> implemented in Casey's work. The reason call_int_hook() can work for hooks
> which change state (e.g. security_inode_alloc()) is that there is no need to
> call undo function because a hook which might change state (so-called Major LSM
> modules) is always the last entry of the list. If we allow runtime registration
> of LSM modules, there is a possibility that call_int_hook() returns an error
> after some LSM module allocated memory. You need to implement safe transaction
> in order to allow FOR_EACH_SECURITY_HOOK_RW hooks to return an error.
>
My suggestion is we create a whitelist of "minor" hooks". These hooks
should have no state, or blob manipulation. The only downside here, is
sometimes you can cheat, and do what Yama does, in the sense of
keeping its own relationships completely outside of the blobs which
live on the actually tasks structs themselves. Given that Yama has
done this successfully, I think we should trust module authors to not
make these mistakes.

> ---
>  include/linux/lsm_hooks.h |  4 +--
>  security/security.c       | 86 +++++++++++++----------------------------------
>  security/security.h       |  5 ---
>  3 files changed, 25 insertions(+), 70 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 5c227bb..98809d6 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2010,7 +2010,7 @@ struct lsm_info {
>         const char                      *name;
>         const unsigned int              count;
>         struct security_hook_list       *hooks;
> -       struct module                   *owner;
> +       const struct module             *owner;
>  } __randomize_layout;
>
>  struct security_hook_list {
> @@ -2044,7 +2044,7 @@ struct security_hook_list {
>         }
>
>  extern int __must_check security_add_hooks(struct lsm_info *lsm,
> -                                               bool is_mutable);
> +                                          bool is_writable);
>
>  #ifdef CONFIG_SECURITY_SELINUX_DISABLE
>  void __security_delete_hooks(struct lsm_info *lsm);
> diff --git a/security/security.c b/security/security.c
> index 3606512..a4bc9cd 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -48,18 +48,15 @@
>
>  /*
>   * security_allow_unregister_hooks blocks the delete_module syscall for
> - * hooks that are loaded into the  set of mutable hooks by getting a reference
> + * hooks that are loaded into the set of writable hooks by getting a reference
>   * on those modules. This allows built-in modules to still delete their security
>   * hooks, so SELinux will still be able to deregister.
>   *
>   * If an arbitrary module tries to deregister, it will get a return code
>   * indicating failure.
> - *
> - * When this value turns true -> false -- Once, lock_existing_hooks will be
> - * called.
>   */
> -static atomic_t security_allow_unregister_hooks =
> -       ATOMIC_INIT(IS_ENABLED(CONFIG_SECURITY_UNREGISTRABLE_HOOKS) ? 1 : 0);
> +static bool security_allow_unregister_hooks =
> +       IS_ENABLED(CONFIG_SECURITY_UNREGISTRABLE_HOOKS) ? 1 : 0;
>
If this is a static bool, can't it just be IS_ENABLED?

>  #define FOR_EACH_SECURITY_HOOK_RO(ITERATOR, HEAD) \
>         hlist_for_each_entry_rcu(ITERATOR, &security_hook_heads_ro.HEAD, list)
> @@ -90,18 +87,18 @@ void __security_delete_hooks(struct lsm_info *info)
>  #define FOR_EACH_SECURITY_HOOK_RW(ITERATOR, HEAD) \
>         hlist_for_each_entry_rcu(ITERATOR, &security_hook_heads_rw.HEAD, list)
>
> -struct security_hook_heads *get_security_hook_heads(bool is_mutable)
> +static struct security_hook_heads *get_security_hook_heads(bool is_writable)
>  {
> -       if (is_mutable)
> +       if (is_writable)
>                 return &security_hook_heads_rw;
>         return &security_hook_heads_ro;
>  }
>  #else
>  #define FOR_EACH_SECURITY_HOOK_RW(ITERATOR, HEAD)      while (0)
>
> -struct security_hook_heads *get_security_hook_heads(bool is_mutable)
> +static struct security_hook_heads *get_security_hook_heads(bool is_writable)
>  {
> -       if (is_mutable)
> +       if (is_writable)
>                 return ERR_PTR(-ENOTSUPP);
>         return &security_hook_heads_ro;
>  }
> @@ -120,7 +117,7 @@ static inline void unlock_lsm(int idx)
>  }
>
>  /**
> - * security_delete_hooks - Remove modules hooks from the mutable hooks lists.
> + * security_delete_hooks - Remove modules hooks from the writable hooks lists.
>   * @lsm_info: The lsm_info struct for this security module
>   *
>   * If LSM unloading is disabled, this function will panic. It should not
> @@ -128,9 +125,8 @@ static inline void unlock_lsm(int idx)
>   */
>  void security_delete_hooks(struct lsm_info *info)
>  {
> -       if (!atomic_read(&security_allow_unregister_hooks))
> -               panic("Security module %s is attempting to delete hooks.\n",
> -                       info->name);
> +       if (!security_allow_unregister_hooks)
> +               panic("Security hooks are already locked.\n");
I don't understand the benefit of changing the language to this. Why
not something like panic("Security module is attempted to delete
locked hooks")? Otherwise, it can be misconstrued that this method
actually locks hooks. Also, given you have the same error phrase
everywhere, should it just be a #define?
>
>         __security_delete_hooks(info);
>
> @@ -138,39 +134,17 @@ void security_delete_hooks(struct lsm_info *info)
>  }
>  EXPORT_SYMBOL_GPL(security_delete_hooks);
>
> -static void lock_existing_hooks(void)
> -{
> -       struct lsm_info *info;
> -
> -       /*
> -        * Prevent module unloading while we're doing this
> -        * try_module_get may fail (safely), if the module
> -        * is already unloading -- allow that.
> -        */
> -       mutex_lock(&module_mutex);
> -       mutex_lock(&lsm_info_lock);
> -       pr_info("Locking security hooks\n");
> -
> -       hlist_for_each_entry(info, &lsm_info_head, list)
> -               WARN_ON(!try_module_get(info->owner));
> -
> -       mutex_unlock(&lsm_info_lock);
> -       mutex_unlock(&module_mutex);
> -}
Why do you think we should remove this code? I think that it's
valuable to keep in there because it prevents an administrator from
accidentally panicing the system. For example, if you have admin A
lock the security hooks, and then admin B comes along and tries to
unload the module, IMHO they shouldn't get a panic, and by default
(unless they rmmod -f) they should just a nice warning that suggests
they're doing something wrong (like the module is in use).

> -
>  static int allow_unregister_hooks_set(const char *val,
>                                         const struct kernel_param *kp)
>  {
> -       const int current_val = atomic_read(&security_allow_unregister_hooks);
> +       const bool current_val = security_allow_unregister_hooks;
Do we need READ_ONCE here to prevent this from being read multiple
times in the function?

>         bool new_val;
> -       int ret;
>
> -       ret = strtobool(val, &new_val);
> -       if (ret)
> +       if (strtobool(val, &new_val))
>                 return 0;
I think I actually made an error here -- and it should have read
+       ret = strtobool(val, &new_val);
+       if (ret)
+               return ret;

Otherwise the user would not get an error if they did something like
echo "badvalue" into it.


>
>         /* Noop */
> -       if (!!new_val == !!current_val)
> +       if (new_val == current_val)
>                 return 0;
>
>         /* Do not allow for the transition from false -> true */
> @@ -178,16 +152,15 @@ static int allow_unregister_hooks_set(const char *val,
>                 return -EPERM;
>
>         /* The only legal transition true -> false */
> -       if (atomic_cmpxchg(&security_allow_unregister_hooks, 1, 0) == 1)
> -               lock_existing_hooks();
> -
> +       if (!xchg(&security_allow_unregister_hooks, 1))
> +               pr_info("Locked security hooks.\n");
>         return 0;
>  }
>
>  static int allow_unregister_hooks_get(char *buffer,
>                                         const struct kernel_param *kp)
>  {
> -       const int current_val = atomic_read(&security_allow_unregister_hooks);
> +       const bool current_val = security_allow_unregister_hooks;
>         int ret;
>
>         ret = sprintf(buffer, "%c\n", current_val ? '1' : '0');
> @@ -241,14 +214,13 @@ static int security_module_cb(struct notifier_block *nb, unsigned long val,
>         struct module *mod = data;
>         struct lsm_info *info;
>
> -       if (val != MODULE_STATE_GOING)
> +       if (val != MODULE_STATE_GOING || security_allow_unregister_hooks)
>                 return NOTIFY_DONE;
>
>         mutex_lock(&lsm_info_lock);
>         hlist_for_each_entry(info, &lsm_info_head, list)
>                 if (mod == info->owner)
> -                       panic("Security module %s is being unloaded without deregistering hooks",
> -                               info->name);
> +                       panic("Security hooks are already locked.\n");
See above.

>         mutex_unlock(&lsm_info_lock);
>
>         return NOTIFY_DONE;
> @@ -334,41 +306,29 @@ static void security_add_hook(struct security_hook_list *hook,
>  /**
>   * security_add_hooks - Add a modules hooks to the hook lists.
>   * @lsm_info: The lsm_info struct for this security module
> - * @is_mutable: Can these hooks be loaded or unloaded after boot time
> + * @is_writable: Can these hooks be loaded or unloaded after boot time
>   *
>   * Each LSM has to register its hooks with the infrastructure.
>   * Return 0 on success
>   */
> -int __must_check security_add_hooks(struct lsm_info *info, bool is_mutable)
> +int __must_check security_add_hooks(struct lsm_info *info, bool is_writable)
>  {
>         struct security_hook_heads *heads;
> -       int i, ret = 0;
> +       int i;
>
> -       heads = get_security_hook_heads(is_mutable);
> +       heads = get_security_hook_heads(is_writable);
>         if (IS_ERR(heads))
>                 return PTR_ERR(heads);
>
>         if (mutex_lock_killable(&lsm_info_lock))
>                 return -EINTR;
>
> -       if (!atomic_read(&security_allow_unregister_hooks)) {
> -               /*
> -                * Because hook deregistration is not allowed, we need to try
> -                * to get a refcount on the module owner.
> -                */
> -               if (!try_module_get(info->owner)) {
> -                       ret = -EINVAL;
> -                       goto out;
> -               }
> -       }
> -
>         for (i = 0; i < info->count; i++)
>                 security_add_hook(&info->hooks[i], info, heads);
>         hlist_add_tail_rcu(&info->list, &lsm_info_head);
> -out:
>         mutex_unlock(&lsm_info_lock);
>
> -       return ret;
> +       return 0;
>  }
>  EXPORT_SYMBOL_GPL(security_add_hooks);
>
> diff --git a/security/security.h b/security/security.h
> index 3e757f5..ed056d5 100644
> --- a/security/security.h
> +++ b/security/security.h
> @@ -1,11 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
>  #include <linux/mutex.h>
>  #include <linux/list.h>
> -#include <linux/lsm_hooks.h>
>
> -#ifndef __SECURITY_SECURITY_H
> -#define __SECURITY_SECURITY_H
Why get rid of these?
>  extern struct mutex lsm_info_lock;
>  extern struct hlist_head lsm_info_head;
> -extern struct security_hook_heads *get_security_hook_heads(bool is_mutable);
> -#endif
> --
> 1.8.3.1
>
> --
> 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


Thanks for the feedback. I think we're getting there at last.

I'd like Casey and James' feedback as well. I think the big questions are:
1) Can we assume that LSM authors will not shoot themselves in the
foot and break major LSMs?
2) Should we protect the administrator from themselves (try_module_get)?
3) Is it okay to allow hooks to be unloaded at all?

I think no matter what the answers to 1-3 are, we can apply patch 1,
2, and 3 once I include the fix-ups that you suggested here.

Patch 4 is heavily dependent on the answer to (1), and patches 6 is
heavily dependent on the answer to (3).
--
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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Tetsuo Handa]

Sargun Dhillon wrote:
> On Thu, Apr 26, 2018 at 12:15 AM, Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> > Suggested changes on top of your series.
> >
> > But I think that your series still lacks critical code which is also not yet
> > implemented in Casey's work. The reason call_int_hook() can work for hooks
> > which change state (e.g. security_inode_alloc()) is that there is no need to
> > call undo function because a hook which might change state (so-called Major LSM
> > modules) is always the last entry of the list. If we allow runtime registration
> > of LSM modules, there is a possibility that call_int_hook() returns an error
> > after some LSM module allocated memory. You need to implement safe transaction
> > in order to allow FOR_EACH_SECURITY_HOOK_RW hooks to return an error.
> >
> My suggestion is we create a whitelist of "minor" hooks". These hooks
> should have no state, or blob manipulation. The only downside here, is
> sometimes you can cheat, and do what Yama does, in the sense of
> keeping its own relationships completely outside of the blobs which
> live on the actually tasks structs themselves. Given that Yama has
> done this successfully, I think we should trust module authors to not
> make these mistakes.

I didn't understand what you mean. Basically, what this series is doing is

int security_inode_alloc(struct inode *inode)
{
	struct security_hook_list *P;
	inode->i_security = NULL;
	hlist_for_each_entry(P, &security_hook_heads_ro.inode_alloc_security, list) {
		int RC = P->hook.inode_alloc_security(inode);
		if (RC != 0)
			return RC; // <= So far only one major module can stay on the list.
	}
	/*** Start of changes made by this series ***/
	hlist_for_each_entry(P, &security_hook_heads_rw.inode_alloc_security, list) {
		int RC = P->hook.inode_alloc_security(inode);
		if (RC != 0)
			return RC; // <= Not calling inode_free_security() for corresponding successful inode_alloc_security().
	}
	/*** End of changes made by this series ***/
	return 0;
}

and what Casey's series is doing is

int security_inode_alloc(struct inode *inode)
{
	struct security_hook_list *P;
	inode->i_security = NULL;
	hlist_for_each_entry(P, &security_hook_heads.inode_alloc_security, list) {
		int RC = P->hook.inode_alloc_security(inode);
		if (RC != 0) {
			/*** Start of changes made by Casey's series ***/
			hlist_for_each_entry(P, &security_hook_heads.inode_free_security, list) {
				P->hook.inode_free_security(inode); // <= Might call inode_free_security() without corresponding successful inode_alloc_security().
			}
			/*** End of changes made by Casey's series ***/
			return RC;
		}
	}
	return 0;
}

. We need to keep track of which module's inode_free_security() needs to be called

int security_inode_alloc(struct inode *inode)
{
	struct security_hook_list *P1;
	struct security_hook_list *P2;
	inode->i_security = NULL;
	hlist_for_each_entry(P1, &security_hook_heads_ro.inode_alloc_security, list) {
		int RC = P1->hook.inode_alloc_security(inode);
		if (RC != 0)
			goto out;
	}
	hlist_for_each_entry(P1, &security_hook_heads_rw.inode_alloc_security, list) {
		int RC = P1->hook.inode_alloc_security(inode);
		if (RC != 0)
			goto out;
	}
	return 0;
out:
	hlist_for_each_entry(P2, &security_hook_heads_ro.inode_free_security, list) {
		if (P1 == P2)
			goto done;
		P2->hook.inode_free_security(inode);
	}
	hlist_for_each_entry(P2, &security_hook_heads_rw.inode_free_security, list) {
		if (P1 == P2)
			goto done;
		P2->hook.inode_free_security(inode);
	}
done:
	return ret;
}

while handling race condition that foo->inode_alloc_security(inode) returned an error and
foo is removed from the list and is waiting for SRCU grace period before hitting P1 == P2
check and therefore by error calls all LSM modules' ->inode_free_security(inode) hooks.



> > @@ -138,39 +134,17 @@ void security_delete_hooks(struct lsm_info *info)
> >  }
> >  EXPORT_SYMBOL_GPL(security_delete_hooks);
> >
> > -static void lock_existing_hooks(void)
> > -{
> > -       struct lsm_info *info;
> > -
> > -       /*
> > -        * Prevent module unloading while we're doing this
> > -        * try_module_get may fail (safely), if the module
> > -        * is already unloading -- allow that.
> > -        */
> > -       mutex_lock(&module_mutex);
> > -       mutex_lock(&lsm_info_lock);
> > -       pr_info("Locking security hooks\n");
> > -
> > -       hlist_for_each_entry(info, &lsm_info_head, list)
> > -               WARN_ON(!try_module_get(info->owner));
> > -
> > -       mutex_unlock(&lsm_info_lock);
> > -       mutex_unlock(&module_mutex);
> > -}
> Why do you think we should remove this code? I think that it's
> valuable to keep in there because it prevents an administrator from
> accidentally panicing the system. For example, if you have admin A
> lock the security hooks, and then admin B comes along and tries to
> unload the module, IMHO they shouldn't get a panic, and by default
> (unless they rmmod -f) they should just a nice warning that suggests
> they're doing something wrong (like the module is in use).

What I suggested is

 	/* Doing init or already dying? */
 	if (mod->state != MODULE_STATE_LIVE) {
 		/* FIXME: if (force), slam module count damn the torpedoes */
 		pr_debug("%s already dying\n", mod->name);
 		ret = -EBUSY;
 		goto out;
 	}
 
+	ret = security_remove_module(mod);
+	if (ret)
+		goto out;
 	/* If it has an init func, it must have an exit func to unload */
 	if (mod->init && !mod->exit) {
 		forced = try_force_unload(flags);

and check like

int security_remove_module(struct module *mod)
{
	struct lsm_info *info;
	int ret = 0;

	if (security_allow_unregister_hooks)
		return 0;

	mutex_lock(&lsm_info_lock);
	hlist_for_each_entry(info, &lsm_info_head, list)
		if (mod == info->owner)
			ret = -EPERM;
	mutex_unlock(&lsm_info_lock);
	return ret;
}

rather than using register_module_notifier().



> Thanks for the feedback. I think we're getting there at last.
> 
> I'd like Casey and James' feedback as well. I think the big questions are:
> 1) Can we assume that LSM authors will not shoot themselves in the
> foot and break major LSMs?

What "break major LSMs" means? Regardless of whether LKM-based LSMs use
inode->i_security, we need to handle race condition described above.

> 2) Should we protect the administrator from themselves (try_module_get)?

What I suggested is "not to play with module usage count".

> 3) Is it okay to allow hooks to be unloaded at all?
> 
> I think no matter what the answers to 1-3 are, we can apply patch 1,
> 2, and 3 once I include the fix-ups that you suggested here.
> 
> Patch 4 is heavily dependent on the answer to (1), and patches 6 is
> heavily dependent on the answer to (3).
--
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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Sargun Dhillon]

On Thu, Apr 26, 2018 at 5:07 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Sargun Dhillon wrote:
>> On Thu, Apr 26, 2018 at 12:15 AM, Tetsuo Handa
>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>> > Suggested changes on top of your series.
>> >
>> > But I think that your series still lacks critical code which is also not yet
>> > implemented in Casey's work. The reason call_int_hook() can work for hooks
>> > which change state (e.g. security_inode_alloc()) is that there is no need to
>> > call undo function because a hook which might change state (so-called Major LSM
>> > modules) is always the last entry of the list. If we allow runtime registration
>> > of LSM modules, there is a possibility that call_int_hook() returns an error
>> > after some LSM module allocated memory. You need to implement safe transaction
>> > in order to allow FOR_EACH_SECURITY_HOOK_RW hooks to return an error.
>> >
>> My suggestion is we create a whitelist of "minor" hooks". These hooks
>> should have no state, or blob manipulation. The only downside here, is
>> sometimes you can cheat, and do what Yama does, in the sense of
>> keeping its own relationships completely outside of the blobs which
>> live on the actually tasks structs themselves. Given that Yama has
>> done this successfully, I think we should trust module authors to not
>> make these mistakes.
>
> I didn't understand what you mean. Basically, what this series is doing is
>
> int security_inode_alloc(struct inode *inode)
> {
>         struct security_hook_list *P;
>         inode->i_security = NULL;
>         hlist_for_each_entry(P, &security_hook_heads_ro.inode_alloc_security, list) {
>                 int RC = P->hook.inode_alloc_security(inode);
>                 if (RC != 0)
>                         return RC; // <= So far only one major module can stay on the list.
>         }
If no major LSMs exist in the RW series, it'll fall through to here.
>         /*** Start of changes made by this series ***/
>         hlist_for_each_entry(P, &security_hook_heads_rw.inode_alloc_security, list) {
>                 int RC = P->hook.inode_alloc_security(inode);
>                 if (RC != 0)
>                         return RC; // <= Not calling inode_free_security() for corresponding successful inode_alloc_security().
inode_free_security is called on all LSM callbacks -- so, it should
be, as long as people don't load unloadable major LSMs. If we want to
be super conservative here, we can do a try_module_get, and a
module_put if RC != 0 on RW hooks. So, we could have something like
call_int_hook_major, and call_void_hook_major.

>         }
>         /*** End of changes made by this series ***/
>         return 0;
> }
>
> and what Casey's series is doing is
>
> int security_inode_alloc(struct inode *inode)
> {
>         struct security_hook_list *P;
>         inode->i_security = NULL;
>         hlist_for_each_entry(P, &security_hook_heads.inode_alloc_security, list) {
>                 int RC = P->hook.inode_alloc_security(inode);
>                 if (RC != 0) {
>                         /*** Start of changes made by Casey's series ***/
>                         hlist_for_each_entry(P, &security_hook_heads.inode_free_security, list) {
>                                 P->hook.inode_free_security(inode); // <= Might call inode_free_security() without corresponding successful inode_alloc_security().
>                         }
>                         /*** End of changes made by Casey's series ***/
>                         return RC;
>                 }
>         }
>         return 0;
> }
Right, but this could be taken care of by just ensuring that nobody
allocates blobs (major LSM), and only one LSM returns a non-zero to
the *alloc* callbacks? Right now, we have this because one has to be a
"major" LSM in order to use these callbacks, and we ensure only one
major LSM is active at a time.

I suggest that either in the short term we:
1) Trust people not to load a second major LSM,
2) See below.

>
> . We need to keep track of which module's inode_free_security() needs to be called
>
> int security_inode_alloc(struct inode *inode)
> {
>         struct security_hook_list *P1;
>         struct security_hook_list *P2;
>         inode->i_security = NULL;
>         hlist_for_each_entry(P1, &security_hook_heads_ro.inode_alloc_security, list) {
>                 int RC = P1->hook.inode_alloc_security(inode);
>                 if (RC != 0)
>                         goto out;
>         }
>         hlist_for_each_entry(P1, &security_hook_heads_rw.inode_alloc_security, list) {
>                 int RC = P1->hook.inode_alloc_security(inode);
>                 if (RC != 0)
>                         goto out;
>         }
>         return 0;
> out:
>         hlist_for_each_entry(P2, &security_hook_heads_ro.inode_free_security, list) {
>                 if (P1 == P2)
>                         goto done;
>                 P2->hook.inode_free_security(inode);
>         }
>         hlist_for_each_entry(P2, &security_hook_heads_rw.inode_free_security, list) {
>                 if (P1 == P2)
>                         goto done;
>                 P2->hook.inode_free_security(inode);
>         }
> done:
>         return ret;
> }
>
> while handling race condition that foo->inode_alloc_security(inode) returned an error and
> foo is removed from the list and is waiting for SRCU grace period before hitting P1 == P2
> check and therefore by error calls all LSM modules' ->inode_free_security(inode) hooks.
>
>
>
>> > @@ -138,39 +134,17 @@ void security_delete_hooks(struct lsm_info *info)
>> >  }
>> >  EXPORT_SYMBOL_GPL(security_delete_hooks);
>> >
>> > -static void lock_existing_hooks(void)
>> > -{
>> > -       struct lsm_info *info;
>> > -
>> > -       /*
>> > -        * Prevent module unloading while we're doing this
>> > -        * try_module_get may fail (safely), if the module
>> > -        * is already unloading -- allow that.
>> > -        */
>> > -       mutex_lock(&module_mutex);
>> > -       mutex_lock(&lsm_info_lock);
>> > -       pr_info("Locking security hooks\n");
>> > -
>> > -       hlist_for_each_entry(info, &lsm_info_head, list)
>> > -               WARN_ON(!try_module_get(info->owner));
>> > -
>> > -       mutex_unlock(&lsm_info_lock);
>> > -       mutex_unlock(&module_mutex);
>> > -}
>> Why do you think we should remove this code? I think that it's
>> valuable to keep in there because it prevents an administrator from
>> accidentally panicing the system. For example, if you have admin A
>> lock the security hooks, and then admin B comes along and tries to
>> unload the module, IMHO they shouldn't get a panic, and by default
>> (unless they rmmod -f) they should just a nice warning that suggests
>> they're doing something wrong (like the module is in use).
>
> What I suggested is
>
>         /* Doing init or already dying? */
>         if (mod->state != MODULE_STATE_LIVE) {
>                 /* FIXME: if (force), slam module count damn the torpedoes */
>                 pr_debug("%s already dying\n", mod->name);
>                 ret = -EBUSY;
>                 goto out;
>         }
>
> +       ret = security_remove_module(mod);
> +       if (ret)
> +               goto out;
>         /* If it has an init func, it must have an exit func to unload */
>         if (mod->init && !mod->exit) {
>                 forced = try_force_unload(flags);
>
> and check like
>
> int security_remove_module(struct module *mod)
> {
>         struct lsm_info *info;
>         int ret = 0;
>
>         if (security_allow_unregister_hooks)
>                 return 0;
>
>         mutex_lock(&lsm_info_lock);
>         hlist_for_each_entry(info, &lsm_info_head, list)
>                 if (mod == info->owner)
>                         ret = -EPERM;
>         mutex_unlock(&lsm_info_lock);
>         return ret;
> }
>
> rather than using register_module_notifier().
>
>
>
>> Thanks for the feedback. I think we're getting there at last.
>>
>> I'd like Casey and James' feedback as well. I think the big questions are:
>> 1) Can we assume that LSM authors will not shoot themselves in the
>> foot and break major LSMs?
>
> What "break major LSMs" means? Regardless of whether LKM-based LSMs use
> inode->i_security, we need to handle race condition described above.
>
>> 2) Should we protect the administrator from themselves (try_module_get)?
>
> What I suggested is "not to play with module usage count".
>
>> 3) Is it okay to allow hooks to be unloaded at all?
>>
>> I think no matter what the answers to 1-3 are, we can apply patch 1,
>> 2, and 3 once I include the fix-ups that you suggested here.
>>
>> Patch 4 is heavily dependent on the answer to (1), and patches 6 is
>> heavily dependent on the answer to (3).
> --
> 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'

What about something as stupid as:
diff --git a/security/security.c b/security/security.c
index 36065128c6c5..895bdb0b1381 100644
--- a/security/security.c
+++ b/security/security.c
@@ -339,7 +339,8 @@ static void security_add_hook(struct
security_hook_list *hook,
  * Each LSM has to register its hooks with the infrastructure.
  * Return 0 on success
  */
-int __must_check security_add_hooks(struct lsm_info *info, bool is_mutable)
+int __must_check security_add_hooks(struct lsm_info *info, bool is_mutable,
+                                       bool is_major)
 {
        struct security_hook_heads *heads;
        int i, ret = 0;
@@ -348,6 +349,9 @@ int __must_check security_add_hooks(struct
lsm_info *info, bool is_mutable)
        if (IS_ERR(heads))
                return PTR_ERR(heads);

+       if (!info->owner && is_major)
+               return -EPERM;
+
        if (mutex_lock_killable(&lsm_info_lock))
                return -EINTR;


OR
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 5c227bbb2883..9cec723e7cea 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2011,6 +2011,7 @@ struct lsm_info {
        const unsigned int              count;
        struct security_hook_list       *hooks;
        struct module                   *owner;
+       bool                            is_major;
 } __randomize_layout;

 struct security_hook_list {
@@ -2035,12 +2036,13 @@ struct security_hook_list {
                .hook   = { .HEAD = HOOK }      \
        }

-#define LSM_MODULE_INIT(NAME, HOOKS)           \
-       {                                       \
-               .name   = NAME,                 \
-               .hooks  = HOOKS,                \
-               .count  = ARRAY_SIZE(HOOKS),    \
-               .owner  = THIS_MODULE,          \
+#define LSM_MODULE_INIT(NAME, HOOKS, IS_MAJOR)         \
+       {                                               \
+               .name           = NAME,                 \
+               .hooks          = HOOKS,                \
+               .count          = ARRAY_SIZE(HOOKS),    \
+               .owner          = THIS_MODULE,          \
+               .is_major       = IS_MAJOR,             \
        }

 extern int __must_check security_add_hooks(struct lsm_info *lsm,
diff --git a/security/security.c b/security/security.c
index 36065128c6c5..a3bfe735c0e2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -80,6 +80,8 @@ void __security_delete_hooks(struct lsm_info *info)

        mutex_lock(&lsm_info_lock);
        hlist_del(&info->list);
+       if (info->is_major)
+               major_modules_loaded--;
        mutex_unlock(&lsm_info_lock);
 }
 #endif
@@ -331,6 +333,8 @@ static void security_add_hook(struct
security_hook_list *hook,
        head = (struct hlist_head *)(heads) + idx;
        hlist_add_tail_rcu(&hook->list, head);
 }
+static int major_modules_loaded;
+
 /**
  * security_add_hooks - Add a modules hooks to the hook lists.
  * @lsm_info: The lsm_info struct for this security module
@@ -351,6 +355,12 @@ int __must_check security_add_hooks(struct
lsm_info *info, bool is_mutable)
        if (mutex_lock_killable(&lsm_info_lock))
                return -EINTR;

+       if (info->is_major && major_modules_loaded) {
+               ret = -EBUSY;
+               goto out;
+       }
+
+
        if (!atomic_read(&security_allow_unregister_hooks)) {
                /*
                 * Because hook deregistration is not allowed, we need to try
@@ -365,6 +375,8 @@ int __must_check security_add_hooks(struct
lsm_info *info, bool is_mutable)
        for (i = 0; i < info->count; i++)
                security_add_hook(&info->hooks[i], info, heads);
        hlist_add_tail_rcu(&info->list, &lsm_info_head);
+       if (info->is_major)
+               major_modules_loaded++;
 out:
        mutex_unlock(&lsm_info_lock);
--
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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Sargun Dhillon]

On Thu, Apr 26, 2018 at 9:40 AM, Sargun Dhillon <sargun@sargun.me> wrote:
> On Thu, Apr 26, 2018 at 5:07 AM, Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>> Sargun Dhillon wrote:
>>> On Thu, Apr 26, 2018 at 12:15 AM, Tetsuo Handa
>>> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>>> > Suggested changes on top of your series.
>>> >
>>> > But I think that your series still lacks critical code which is also not yet
>>> > implemented in Casey's work. The reason call_int_hook() can work for hooks
>>> > which change state (e.g. security_inode_alloc()) is that there is no need to
>>> > call undo function because a hook which might change state (so-called Major LSM
>>> > modules) is always the last entry of the list. If we allow runtime registration
>>> > of LSM modules, there is a possibility that call_int_hook() returns an error
>>> > after some LSM module allocated memory. You need to implement safe transaction
>>> > in order to allow FOR_EACH_SECURITY_HOOK_RW hooks to return an error.
>>> >
>>> My suggestion is we create a whitelist of "minor" hooks". These hooks
>>> should have no state, or blob manipulation. The only downside here, is
>>> sometimes you can cheat, and do what Yama does, in the sense of
>>> keeping its own relationships completely outside of the blobs which
>>> live on the actually tasks structs themselves. Given that Yama has
>>> done this successfully, I think we should trust module authors to not
>>> make these mistakes.
>>
>> I didn't understand what you mean. Basically, what this series is doing is
>>
>> int security_inode_alloc(struct inode *inode)
>> {
>>         struct security_hook_list *P;
>>         inode->i_security = NULL;
>>         hlist_for_each_entry(P, &security_hook_heads_ro.inode_alloc_security, list) {
>>                 int RC = P->hook.inode_alloc_security(inode);
>>                 if (RC != 0)
>>                         return RC; // <= So far only one major module can stay on the list.
>>         }
> If no major LSMs exist in the RW series, it'll fall through to here.
>>         /*** Start of changes made by this series ***/
>>         hlist_for_each_entry(P, &security_hook_heads_rw.inode_alloc_security, list) {
>>                 int RC = P->hook.inode_alloc_security(inode);
>>                 if (RC != 0)
>>                         return RC; // <= Not calling inode_free_security() for corresponding successful inode_alloc_security().
> inode_free_security is called on all LSM callbacks -- so, it should
> be, as long as people don't load unloadable major LSMs. If we want to
> be super conservative here, we can do a try_module_get, and a
> module_put if RC != 0 on RW hooks. So, we could have something like
> call_int_hook_major, and call_void_hook_major.
>
>>         }
>>         /*** End of changes made by this series ***/
>>         return 0;
>> }
>>
>> and what Casey's series is doing is
>>
>> int security_inode_alloc(struct inode *inode)
>> {
>>         struct security_hook_list *P;
>>         inode->i_security = NULL;
>>         hlist_for_each_entry(P, &security_hook_heads.inode_alloc_security, list) {
>>                 int RC = P->hook.inode_alloc_security(inode);
>>                 if (RC != 0) {
>>                         /*** Start of changes made by Casey's series ***/
>>                         hlist_for_each_entry(P, &security_hook_heads.inode_free_security, list) {
>>                                 P->hook.inode_free_security(inode); // <= Might call inode_free_security() without corresponding successful inode_alloc_security().
>>                         }
>>                         /*** End of changes made by Casey's series ***/
>>                         return RC;
>>                 }
>>         }
>>         return 0;
>> }
> Right, but this could be taken care of by just ensuring that nobody
> allocates blobs (major LSM), and only one LSM returns a non-zero to
> the *alloc* callbacks? Right now, we have this because one has to be a
> "major" LSM in order to use these callbacks, and we ensure only one
> major LSM is active at a time.
>
> I suggest that either in the short term we:
> 1) Trust people not to load a second major LSM,
> 2) See below.
>
>>
>> . We need to keep track of which module's inode_free_security() needs to be called
>>
>> int security_inode_alloc(struct inode *inode)
>> {
>>         struct security_hook_list *P1;
>>         struct security_hook_list *P2;
>>         inode->i_security = NULL;
>>         hlist_for_each_entry(P1, &security_hook_heads_ro.inode_alloc_security, list) {
>>                 int RC = P1->hook.inode_alloc_security(inode);
>>                 if (RC != 0)
>>                         goto out;
>>         }
>>         hlist_for_each_entry(P1, &security_hook_heads_rw.inode_alloc_security, list) {
>>                 int RC = P1->hook.inode_alloc_security(inode);
>>                 if (RC != 0)
>>                         goto out;
>>         }
>>         return 0;
>> out:
>>         hlist_for_each_entry(P2, &security_hook_heads_ro.inode_free_security, list) {
>>                 if (P1 == P2)
>>                         goto done;
>>                 P2->hook.inode_free_security(inode);
>>         }
>>         hlist_for_each_entry(P2, &security_hook_heads_rw.inode_free_security, list) {
>>                 if (P1 == P2)
>>                         goto done;
>>                 P2->hook.inode_free_security(inode);
>>         }
>> done:
>>         return ret;
>> }
>>
>> while handling race condition that foo->inode_alloc_security(inode) returned an error and
>> foo is removed from the list and is waiting for SRCU grace period before hitting P1 == P2
>> check and therefore by error calls all LSM modules' ->inode_free_security(inode) hooks.
>>
>>
>>
>>> > @@ -138,39 +134,17 @@ void security_delete_hooks(struct lsm_info *info)
>>> >  }
>>> >  EXPORT_SYMBOL_GPL(security_delete_hooks);
>>> >
>>> > -static void lock_existing_hooks(void)
>>> > -{
>>> > -       struct lsm_info *info;
>>> > -
>>> > -       /*
>>> > -        * Prevent module unloading while we're doing this
>>> > -        * try_module_get may fail (safely), if the module
>>> > -        * is already unloading -- allow that.
>>> > -        */
>>> > -       mutex_lock(&module_mutex);
>>> > -       mutex_lock(&lsm_info_lock);
>>> > -       pr_info("Locking security hooks\n");
>>> > -
>>> > -       hlist_for_each_entry(info, &lsm_info_head, list)
>>> > -               WARN_ON(!try_module_get(info->owner));
>>> > -
>>> > -       mutex_unlock(&lsm_info_lock);
>>> > -       mutex_unlock(&module_mutex);
>>> > -}
>>> Why do you think we should remove this code? I think that it's
>>> valuable to keep in there because it prevents an administrator from
>>> accidentally panicing the system. For example, if you have admin A
>>> lock the security hooks, and then admin B comes along and tries to
>>> unload the module, IMHO they shouldn't get a panic, and by default
>>> (unless they rmmod -f) they should just a nice warning that suggests
>>> they're doing something wrong (like the module is in use).
>>
>> What I suggested is
>>
>>         /* Doing init or already dying? */
>>         if (mod->state != MODULE_STATE_LIVE) {
>>                 /* FIXME: if (force), slam module count damn the torpedoes */
>>                 pr_debug("%s already dying\n", mod->name);
>>                 ret = -EBUSY;
>>                 goto out;
>>         }
>>
>> +       ret = security_remove_module(mod);
>> +       if (ret)
>> +               goto out;
>>         /* If it has an init func, it must have an exit func to unload */
>>         if (mod->init && !mod->exit) {
>>                 forced = try_force_unload(flags);
>>
>> and check like
>>
>> int security_remove_module(struct module *mod)
>> {
>>         struct lsm_info *info;
>>         int ret = 0;
>>
>>         if (security_allow_unregister_hooks)
>>                 return 0;
>>
>>         mutex_lock(&lsm_info_lock);
>>         hlist_for_each_entry(info, &lsm_info_head, list)
>>                 if (mod == info->owner)
>>                         ret = -EPERM;
>>         mutex_unlock(&lsm_info_lock);
>>         return ret;
>> }
>>
>> rather than using register_module_notifier().
>>
>>
>>
>>> Thanks for the feedback. I think we're getting there at last.
>>>
>>> I'd like Casey and James' feedback as well. I think the big questions are:
>>> 1) Can we assume that LSM authors will not shoot themselves in the
>>> foot and break major LSMs?
>>
>> What "break major LSMs" means? Regardless of whether LKM-based LSMs use
>> inode->i_security, we need to handle race condition described above.
>>
>>> 2) Should we protect the administrator from themselves (try_module_get)?
>>
>> What I suggested is "not to play with module usage count".
>>
>>> 3) Is it okay to allow hooks to be unloaded at all?
>>>
>>> I think no matter what the answers to 1-3 are, we can apply patch 1,
>>> 2, and 3 once I include the fix-ups that you suggested here.
>>>
>>> Patch 4 is heavily dependent on the answer to (1), and patches 6 is
>>> heavily dependent on the answer to (3).
>> --
>> 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'
>
> What about something as stupid as:
> diff --git a/security/security.c b/security/security.c
> index 36065128c6c5..895bdb0b1381 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -339,7 +339,8 @@ static void security_add_hook(struct
> security_hook_list *hook,
>   * Each LSM has to register its hooks with the infrastructure.
>   * Return 0 on success
>   */
> -int __must_check security_add_hooks(struct lsm_info *info, bool is_mutable)
> +int __must_check security_add_hooks(struct lsm_info *info, bool is_mutable,
> +                                       bool is_major)
>  {
>         struct security_hook_heads *heads;
>         int i, ret = 0;
> @@ -348,6 +349,9 @@ int __must_check security_add_hooks(struct
> lsm_info *info, bool is_mutable)
>         if (IS_ERR(heads))
>                 return PTR_ERR(heads);
>
> +       if (!info->owner && is_major)
> +               return -EPERM;
> +
>         if (mutex_lock_killable(&lsm_info_lock))
>                 return -EINTR;
>
>
> OR
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 5c227bbb2883..9cec723e7cea 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2011,6 +2011,7 @@ struct lsm_info {
>         const unsigned int              count;
>         struct security_hook_list       *hooks;
>         struct module                   *owner;
> +       bool                            is_major;
>  } __randomize_layout;
>
>  struct security_hook_list {
> @@ -2035,12 +2036,13 @@ struct security_hook_list {
>                 .hook   = { .HEAD = HOOK }      \
>         }
>
> -#define LSM_MODULE_INIT(NAME, HOOKS)           \
> -       {                                       \
> -               .name   = NAME,                 \
> -               .hooks  = HOOKS,                \
> -               .count  = ARRAY_SIZE(HOOKS),    \
> -               .owner  = THIS_MODULE,          \
> +#define LSM_MODULE_INIT(NAME, HOOKS, IS_MAJOR)         \
> +       {                                               \
> +               .name           = NAME,                 \
> +               .hooks          = HOOKS,                \
> +               .count          = ARRAY_SIZE(HOOKS),    \
> +               .owner          = THIS_MODULE,          \
> +               .is_major       = IS_MAJOR,             \
>         }
>
>  extern int __must_check security_add_hooks(struct lsm_info *lsm,
> diff --git a/security/security.c b/security/security.c
> index 36065128c6c5..a3bfe735c0e2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -80,6 +80,8 @@ void __security_delete_hooks(struct lsm_info *info)
>
>         mutex_lock(&lsm_info_lock);
>         hlist_del(&info->list);
> +       if (info->is_major)
> +               major_modules_loaded--;
>         mutex_unlock(&lsm_info_lock);
>  }
>  #endif
> @@ -331,6 +333,8 @@ static void security_add_hook(struct
> security_hook_list *hook,
>         head = (struct hlist_head *)(heads) + idx;
>         hlist_add_tail_rcu(&hook->list, head);
>  }
> +static int major_modules_loaded;
> +
>  /**
>   * security_add_hooks - Add a modules hooks to the hook lists.
>   * @lsm_info: The lsm_info struct for this security module
> @@ -351,6 +355,12 @@ int __must_check security_add_hooks(struct
> lsm_info *info, bool is_mutable)
>         if (mutex_lock_killable(&lsm_info_lock))
>                 return -EINTR;
>
> +       if (info->is_major && major_modules_loaded) {
> +               ret = -EBUSY;
> +               goto out;
> +       }
> +
> +
>         if (!atomic_read(&security_allow_unregister_hooks)) {
>                 /*
>                  * Because hook deregistration is not allowed, we need to try
> @@ -365,6 +375,8 @@ int __must_check security_add_hooks(struct
> lsm_info *info, bool is_mutable)
>         for (i = 0; i < info->count; i++)
>                 security_add_hook(&info->hooks[i], info, heads);
>         hlist_add_tail_rcu(&info->list, &lsm_info_head);
> +       if (info->is_major)
> +               major_modules_loaded++;
>  out:
>         mutex_unlock(&lsm_info_lock);
---
One other aspect that may not be obvious is on the *_alloc_*,
callbacks, we wouldn't want to call callback's of LSMs which are
non-major.
--
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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Tetsuo Handa]

Sargun Dhillon wrote:
> > inode_free_security is called on all LSM callbacks -- so, it should
> > be, as long as people don't load unloadable major LSMs. If we want to
> > be super conservative here, we can do a try_module_get, and a
> > module_put if RC != 0 on RW hooks. So, we could have something like
> > call_int_hook_major, and call_void_hook_major.

I don't think we want to play with module usage count.

> >
> >>         }
> >>         /*** End of changes made by this series ***/
> >>         return 0;
> >> }
> >>
> >> and what Casey's series is doing is
> >>
> >> int security_inode_alloc(struct inode *inode)
> >> {
> >>         struct security_hook_list *P;
> >>         inode->i_security = NULL;
> >>         hlist_for_each_entry(P, &security_hook_heads.inode_alloc_security, list) {
> >>                 int RC = P->hook.inode_alloc_security(inode);
> >>                 if (RC != 0) {
> >>                         /*** Start of changes made by Casey's series ***/
> >>                         hlist_for_each_entry(P, &security_hook_heads.inode_free_security, list) {
> >>                                 P->hook.inode_free_security(inode); // <= Might call inode_free_security() without corresponding successful inode_alloc_security().
> >>                         }
> >>                         /*** End of changes made by Casey's series ***/
> >>                         return RC;
> >>                 }
> >>         }
> >>         return 0;
> >> }
> > Right, but this could be taken care of by just ensuring that nobody
> > allocates blobs (major LSM), and only one LSM returns a non-zero to
> > the *alloc* callbacks? Right now, we have this because one has to be a
> > "major" LSM in order to use these callbacks, and we ensure only one
> > major LSM is active at a time.

What Casey is trying to do is allow multiple major modules to use their
own blob. Thus,

> >
> > I suggest that either in the short term we:
> > 1) Trust people not to load a second major LSM,

this is not an option.

> > 2) See below.
> >
> > What about something as stupid as:

I don't think we want to do this.

> One other aspect that may not be obvious is on the *_alloc_*,
> callbacks, we wouldn't want to call callback's of LSMs which are
> non-major.

What both your series and Casey's series are missing will be as simple as below.
Apart from whether memory for security blobs should be managed by infrastructure or
by individual module, we will need to do something like below.

PATCH 1/2: Baseline for allowing safe runtime registration.

This patch allows multiple major LSM modules to safely manage their blobs
(though memory for security blobs need to be managed by individual module),
by calling only release hooks where corresponding alloc hook succeeded when
one of alloc hook returned an error. And thereby this patch makes it possible
to load LKM-based LSM modules.
---
 include/linux/lsm_hooks.h                     |  21 +--
 scripts/gcc-plugins/randomize_layout_plugin.c |   2 -
 security/Kconfig                              |  14 +-
 security/security.c                           | 184 +++++++++++++++++---------
 4 files changed, 148 insertions(+), 73 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 9d0b286..083dea6 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2005,10 +2005,10 @@ struct security_hook_heads {
  * For use with generic list macros for common operations.
  */
 struct security_hook_list {
-	struct hlist_node		list;
-	struct hlist_head		*head;
-	union security_list_options	hook;
-	char				*lsm;
+	struct hlist_node			list;
+	const unsigned int			offset;
+	const union security_list_options	hook;
+	const char				*lsm;
 } __randomize_layout;
 
 /*
@@ -2017,14 +2017,16 @@ struct security_hook_list {
  * care of the common case and reduces the amount of
  * text involved.
  */
-#define LSM_HOOK_INIT(HEAD, HOOK) \
-	{ .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } }
+#define LSM_HOOK_INIT(HEAD, HOOK)				\
+	{							\
+		.offset = offsetof(struct security_hook_heads, HEAD),	\
+		.hook = { .HEAD = HOOK }				\
+	}
 
-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);
+extern int security_add_hooks(struct security_hook_list *hooks, int count,
+			      const char *lsm);
 
 #ifdef CONFIG_SECURITY_SELINUX_DISABLE
 /*
@@ -2049,7 +2051,6 @@ static inline void security_delete_hooks(struct security_hook_list *hooks,
 }
 #endif /* CONFIG_SECURITY_SELINUX_DISABLE */
 
-/* Currently required to handle SELinux runtime hook disable. */
 #ifdef CONFIG_SECURITY_WRITABLE_HOOKS
 #define __lsm_ro_after_init
 #else
diff --git a/scripts/gcc-plugins/randomize_layout_plugin.c b/scripts/gcc-plugins/randomize_layout_plugin.c
index 6d5bbd3..d941389 100644
--- a/scripts/gcc-plugins/randomize_layout_plugin.c
+++ b/scripts/gcc-plugins/randomize_layout_plugin.c
@@ -52,8 +52,6 @@ struct whitelist_entry {
 	{ "net/unix/af_unix.c", "unix_skb_parms", "char" },
 	/* big_key payload.data struct splashing */
 	{ "security/keys/big_key.c", "path", "void *" },
-	/* walk struct security_hook_heads as an array of struct hlist_head */
-	{ "security/security.c", "hlist_head", "security_hook_heads" },
 	{ }
 };
 
diff --git a/security/Kconfig b/security/Kconfig
index c430206..84342d1 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -277,5 +277,17 @@ config DEFAULT_SECURITY
 	default "apparmor" if DEFAULT_SECURITY_APPARMOR
 	default "" if DEFAULT_SECURITY_DAC
 
-endmenu
+config SECURITY_RUNTIME_LOADING
+	bool "Allow adding security modules after boot"
+	depends on SECURITY
+	select SECURITY_WRITABLE_HOOKS
+	default n
+	help
+	  This option allows adding security modules which are implemented as
+	  loadable kernel modules after boot.
+
+	  NOTE: selecting this option will disable the '__ro_after_init'
+	  kernel hardening feature for security hooks. If you are unsure
+	  how to answer this question, answer N.
 
+endmenu
diff --git a/security/security.c b/security/security.c
index 7bc2fde..048da2a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -37,7 +37,7 @@
 /* Maximum number of letters for an LSM name string */
 #define SECURITY_NAME_MAX	10
 
-struct security_hook_heads security_hook_heads __lsm_ro_after_init;
+static struct security_hook_heads security_hook_heads __lsm_ro_after_init;
 static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
 
 char *lsm_names;
@@ -66,12 +66,6 @@ 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");
 
 	/*
@@ -112,7 +106,7 @@ static bool match_last_lsm(const char *list, const char *lsm)
 	return !strcmp(last, lsm);
 }
 
-static int lsm_append(char *new, char **result)
+static int lsm_append(const char *new, char **result)
 {
 	char *cp;
 
@@ -162,17 +156,37 @@ int __init security_module_enable(const char *module)
  *
  * Each LSM has to register its hooks with the infrastructure.
  */
-void __init security_add_hooks(struct security_hook_list *hooks, int count,
-				char *lsm)
+#ifdef SECURITY_RUNTIME_LOADING
+EXPORT_SYMBOL_GPL(security_add_hooks);
+#else
+__init
+#endif
+int security_add_hooks(struct security_hook_list *hooks, int count,
+		       const char *lsm)
 {
 	int i;
 
+	/*
+	 * If OOM occurs from __init function, panic() is already called before
+	 * failing the memory allocation. Thus, only LKM-based LSMs will get
+	 * -ENOMEM error.
+	 */
+	if (lsm_append(lsm, &lsm_names) < 0)
+		return -ENOMEM;
 	for (i = 0; i < count; i++) {
+		const unsigned int offset = hooks[i].offset;
+		struct hlist_head *head;
+
+		if (offset % sizeof(struct hlist_head) ||
+		    offset + sizeof(struct hlist_head) >
+		    sizeof(struct security_hook_heads))
+			panic("Invalid offset when adding %s module", lsm);
 		hooks[i].lsm = lsm;
-		hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
+		head = (struct hlist_head *) (((char *) &security_hook_heads)
+					      + offset);
+		hlist_add_tail_rcu(&hooks[i].list, head);
 	}
-	if (lsm_append(lsm, &lsm_names) < 0)
-		panic("%s - Cannot get early memory.\n", __func__);
+	return 0;
 }
 
 int call_lsm_notifier(enum lsm_event event, void *data)
@@ -213,16 +227,62 @@ int unregister_lsm_notifier(struct notifier_block *nb)
 
 #define call_int_hook(FUNC, IRC, ...) ({			\
 	int RC = IRC;						\
+	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;						\
+	}								\
+	RC;								\
+})
+
+#define call_transactional_int_hook(UNDO_FUNC, UNDO_ARG, FUNC, ...) ({	\
+	int RC = 0;							\
+	struct security_hook_list *P1;					\
+	struct security_hook_list *P2;					\
+									\
+	hlist_for_each_entry(P1, &security_hook_heads.FUNC, list) {	\
+		RC = P1->hook.FUNC(__VA_ARGS__);			\
+		if (!RC)						\
+			continue;					\
+		hlist_for_each_entry(P2, &security_hook_heads.FUNC,	\
+				     list) {				\
+			if (P1 == P2)					\
+				break;					\
+			if (P2->hook.UNDO_FUNC)				\
+				P2->hook.UNDO_FUNC(UNDO_ARG);		\
+		}							\
+		break;							\
+	}								\
+	RC;								\
+})
+
+/*
+ * Since CONFIG_SECURITY_NETWORK_XFRM is not ready to handle multiple LSM
+ * hooks, and fortunately currently only SELinux uses it, use macros for
+ * calling only first LSM hook. This implies that LKM-based LSMs won't be
+ * able to use hooks for CONFIG_SECURITY_NETWORK_XFRM if SELinux is in use.
+ */
+#define call_first_void_hook(FUNC, ...)			\
 	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;				\
+			P->hook.FUNC(__VA_ARGS__);		\
+			break;					\
 		}						\
-	} while (0);						\
-	RC;							\
+	} while (0)
+
+#define call_first_int_hook(FUNC, IRC, ...) ({			\
+	int RC = IRC;						\
+	struct security_hook_list *P;				\
+								\
+	hlist_for_each_entry(P, &security_hook_heads.FUNC, list) {	\
+		RC = P->hook.FUNC(__VA_ARGS__);				\
+		break;							\
+	}								\
+	RC;								\
 })
 
 /* Security operations */
@@ -360,7 +420,8 @@ void security_bprm_committed_creds(struct linux_binprm *bprm)
 
 int security_sb_alloc(struct super_block *sb)
 {
-	return call_int_hook(sb_alloc_security, 0, sb);
+	return call_transactional_int_hook(sb_free_security, sb,
+					   sb_alloc_security, sb);
 }
 
 void security_sb_free(struct super_block *sb)
@@ -439,8 +500,13 @@ int security_sb_parse_opts_str(char *options, struct security_mnt_opts *opts)
 
 int security_inode_alloc(struct inode *inode)
 {
+	int ret;
 	inode->i_security = NULL;
-	return call_int_hook(inode_alloc_security, 0, inode);
+	ret = call_transactional_int_hook(inode_free_security, inode,
+					  inode_alloc_security, inode);
+	if (ret)
+		inode->i_security = NULL;
+	return ret;
 }
 
 void security_inode_free(struct inode *inode)
@@ -876,7 +942,8 @@ int security_file_permission(struct file *file, int mask)
 
 int security_file_alloc(struct file *file)
 {
-	return call_int_hook(file_alloc_security, 0, file);
+	return call_transactional_int_hook(file_free_security, file,
+					   file_alloc_security, file);
 }
 
 void security_file_free(struct file *file)
@@ -983,7 +1050,8 @@ int security_file_open(struct file *file, const struct cred *cred)
 
 int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
 {
-	return call_int_hook(task_alloc, 0, task, clone_flags);
+	return call_transactional_int_hook(task_free, task, task_alloc, task,
+					   clone_flags);
 }
 
 void security_task_free(struct task_struct *task)
@@ -1168,7 +1236,8 @@ void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid)
 
 int security_msg_msg_alloc(struct msg_msg *msg)
 {
-	return call_int_hook(msg_msg_alloc_security, 0, msg);
+	return call_transactional_int_hook(msg_msg_free_security, msg,
+					   msg_msg_alloc_security, msg);
 }
 
 void security_msg_msg_free(struct msg_msg *msg)
@@ -1178,7 +1247,8 @@ void security_msg_msg_free(struct msg_msg *msg)
 
 int security_msg_queue_alloc(struct kern_ipc_perm *msq)
 {
-	return call_int_hook(msg_queue_alloc_security, 0, msq);
+	return call_transactional_int_hook(msg_queue_free_security, msq,
+					   msg_queue_alloc_security, msq);
 }
 
 void security_msg_queue_free(struct kern_ipc_perm *msq)
@@ -1210,7 +1280,8 @@ int security_msg_queue_msgrcv(struct kern_ipc_perm *msq, struct msg_msg *msg,
 
 int security_shm_alloc(struct kern_ipc_perm *shp)
 {
-	return call_int_hook(shm_alloc_security, 0, shp);
+	return call_transactional_int_hook(shm_free_security, shp,
+					   shm_alloc_security, shp);
 }
 
 void security_shm_free(struct kern_ipc_perm *shp)
@@ -1235,7 +1306,8 @@ int security_shm_shmat(struct kern_ipc_perm *shp, char __user *shmaddr, int shmf
 
 int security_sem_alloc(struct kern_ipc_perm *sma)
 {
-	return call_int_hook(sem_alloc_security, 0, sma);
+	return call_transactional_int_hook(sem_free_security, sma,
+					   sem_alloc_security, sma);
 }
 
 void security_sem_free(struct kern_ipc_perm *sma)
@@ -1436,7 +1508,9 @@ int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff *skb, u
 
 int security_sk_alloc(struct sock *sk, int family, gfp_t priority)
 {
-	return call_int_hook(sk_alloc_security, 0, sk, family, priority);
+	return call_transactional_int_hook(sk_free_security, sk,
+					   sk_alloc_security, sk, family,
+					   priority);
 }
 
 void security_sk_free(struct sock *sk)
@@ -1598,89 +1672,76 @@ int security_xfrm_policy_alloc(struct xfrm_sec_ctx **ctxp,
 			       struct xfrm_user_sec_ctx *sec_ctx,
 			       gfp_t gfp)
 {
-	return call_int_hook(xfrm_policy_alloc_security, 0, ctxp, sec_ctx, gfp);
+	return call_first_int_hook(xfrm_policy_alloc_security, 0, ctxp,
+				   sec_ctx, gfp);
 }
 EXPORT_SYMBOL(security_xfrm_policy_alloc);
 
 int security_xfrm_policy_clone(struct xfrm_sec_ctx *old_ctx,
 			      struct xfrm_sec_ctx **new_ctxp)
 {
-	return call_int_hook(xfrm_policy_clone_security, 0, old_ctx, new_ctxp);
+	return call_first_int_hook(xfrm_policy_clone_security, 0, old_ctx,
+				   new_ctxp);
 }
 
 void security_xfrm_policy_free(struct xfrm_sec_ctx *ctx)
 {
-	call_void_hook(xfrm_policy_free_security, ctx);
+	call_first_void_hook(xfrm_policy_free_security, ctx);
 }
 EXPORT_SYMBOL(security_xfrm_policy_free);
 
 int security_xfrm_policy_delete(struct xfrm_sec_ctx *ctx)
 {
-	return call_int_hook(xfrm_policy_delete_security, 0, ctx);
+	return call_first_int_hook(xfrm_policy_delete_security, 0, ctx);
 }
 
 int security_xfrm_state_alloc(struct xfrm_state *x,
 			      struct xfrm_user_sec_ctx *sec_ctx)
 {
-	return call_int_hook(xfrm_state_alloc, 0, x, sec_ctx);
+	return call_first_int_hook(xfrm_state_alloc, 0, x, sec_ctx);
 }
 EXPORT_SYMBOL(security_xfrm_state_alloc);
 
 int security_xfrm_state_alloc_acquire(struct xfrm_state *x,
 				      struct xfrm_sec_ctx *polsec, u32 secid)
 {
-	return call_int_hook(xfrm_state_alloc_acquire, 0, x, polsec, secid);
+	return call_first_int_hook(xfrm_state_alloc_acquire, 0, x, polsec,
+				   secid);
 }
 
 int security_xfrm_state_delete(struct xfrm_state *x)
 {
-	return call_int_hook(xfrm_state_delete_security, 0, x);
+	return call_first_int_hook(xfrm_state_delete_security, 0, x);
 }
 EXPORT_SYMBOL(security_xfrm_state_delete);
 
 void security_xfrm_state_free(struct xfrm_state *x)
 {
-	call_void_hook(xfrm_state_free_security, x);
+	call_first_void_hook(xfrm_state_free_security, x);
 }
 
 int security_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir)
 {
-	return call_int_hook(xfrm_policy_lookup, 0, ctx, fl_secid, dir);
+	return call_first_int_hook(xfrm_policy_lookup, 0, ctx, fl_secid, dir);
 }
 
 int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
 				       struct xfrm_policy *xp,
 				       const struct flowi *fl)
 {
-	struct security_hook_list *hp;
-	int rc = 1;
-
-	/*
-	 * Since this function is expected to return 0 or 1, the judgment
-	 * becomes difficult if multiple LSMs supply this call. Fortunately,
-	 * we can use the first LSM's judgment because currently only SELinux
-	 * supplies this call.
-	 *
-	 * 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,
-				list) {
-		rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
-		break;
-	}
-	return rc;
+	/* This function is expected to return 0 or 1. */
+	return call_first_int_hook(xfrm_state_pol_flow_match, 1, x, xp, fl);
 }
 
 int security_xfrm_decode_session(struct sk_buff *skb, u32 *secid)
 {
-	return call_int_hook(xfrm_decode_session, 0, skb, secid, 1);
+	return call_first_int_hook(xfrm_decode_session, 0, skb, secid, 1);
 }
 
 void security_skb_classify_flow(struct sk_buff *skb, struct flowi *fl)
 {
-	int rc = call_int_hook(xfrm_decode_session, 0, skb, &fl->flowi_secid,
-				0);
+	int rc = call_first_int_hook(xfrm_decode_session, 0, skb,
+				     &fl->flowi_secid, 0);
 
 	BUG_ON(rc);
 }
@@ -1693,7 +1754,8 @@ void security_skb_classify_flow(struct sk_buff *skb, struct flowi *fl)
 int security_key_alloc(struct key *key, const struct cred *cred,
 		       unsigned long flags)
 {
-	return call_int_hook(key_alloc, 0, key, cred, flags);
+	return call_transactional_int_hook(key_free, key, key_alloc, key, cred,
+					   flags);
 }
 
 void security_key_free(struct key *key)
@@ -1755,11 +1817,13 @@ int security_bpf_prog(struct bpf_prog *prog)
 }
 int security_bpf_map_alloc(struct bpf_map *map)
 {
-	return call_int_hook(bpf_map_alloc_security, 0, map);
+	return call_transactional_int_hook(bpf_map_free_security, map,
+					   bpf_map_alloc_security, map);
 }
 int security_bpf_prog_alloc(struct bpf_prog_aux *aux)
 {
-	return call_int_hook(bpf_prog_alloc_security, 0, aux);
+	return call_transactional_int_hook(bpf_prog_free_security, aux,
+					   bpf_prog_alloc_security, aux);
 }
 void security_bpf_map_free(struct bpf_map *map)
 {
-- 
1.8.3.1


PATCH 2/2: Baseline for allowing safe runtime unregistration.

This patch makes it possible to unload LKM-based LSM modules, by using
another list for closing possible race window that release hooks are
by error called without corresponding successful alloc hook calls.

Though, it is questionable whether we want to support it at infrastructure
level. We can use a stacking LSM module which allows stacked LSM modules to
register/unregister. In that way, we can avoid lock_lsm()/unlock_lsm() at
infrastructure level. The stacking approach could also be applied for
separating mutable and immutable hooks. That is, the stacking LSM module
itself is chained to immutable (__ro_after_init) hook, and LKM-based LSMs
are chained to mutable (!__ro_after_init) hook.
---
 include/linux/lsm_hooks.h | 27 +++-----------
 security/Kconfig          |  8 +++++
 security/security.c       | 89 +++++++++++++++++++++++++++++++++++++++++++----
 3 files changed, 94 insertions(+), 30 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 083dea6..548e00b 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2006,6 +2006,9 @@ struct security_hook_heads {
  */
 struct security_hook_list {
 	struct hlist_node			list;
+#if defined(CONFIG_SECURITY_SELINUX_DISABLE) || defined(CONFIG_SECURITY_RUNTIME_UNLOADING)
+	struct hlist_node			undo_list;
+#endif
 	const unsigned int			offset;
 	const union security_list_options	hook;
 	const char				*lsm;
@@ -2027,29 +2030,7 @@ struct security_hook_list {
 
 extern int security_add_hooks(struct security_hook_list *hooks, int count,
 			      const char *lsm);
-
-#ifdef CONFIG_SECURITY_SELINUX_DISABLE
-/*
- * Assuring the safety of deleting a security module is up to
- * the security module involved. This may entail ordering the
- * module's hook list in a particular way, refusing to disable
- * the module once a policy is loaded or any number of other
- * actions better imagined than described.
- *
- * The name of the configuration option reflects the only module
- * that currently uses the mechanism. Any developer who thinks
- * 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 */
+extern void security_delete_hooks(struct security_hook_list *hooks, int count);
 
 #ifdef CONFIG_SECURITY_WRITABLE_HOOKS
 #define __lsm_ro_after_init
diff --git a/security/Kconfig b/security/Kconfig
index 84342d1..9be8788 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -290,4 +290,12 @@ config SECURITY_RUNTIME_LOADING
 	  kernel hardening feature for security hooks. If you are unsure
 	  how to answer this question, answer N.
 
+config SECURITY_RUNTIME_UNLOADING
+	bool "Allow removing security modules after boot"
+	depends on SECURITY_RUNTIME_LOADING && SRCU
+	default n
+	help
+	  This option allows removing security modules which are implemented as
+	  loadable kernel modules after boot.
+
 endmenu
diff --git a/security/security.c b/security/security.c
index 048da2a..40892a7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -29,6 +29,7 @@
 #include <linux/backing-dev.h>
 #include <linux/string.h>
 #include <net/flow.h>
+#include <linux/srcu.h>
 
 #include <trace/events/initcall.h>
 
@@ -38,8 +39,29 @@
 #define SECURITY_NAME_MAX	10
 
 static struct security_hook_heads security_hook_heads __lsm_ro_after_init;
+#if defined(CONFIG_SECURITY_SELINUX_DISABLE) || defined(CONFIG_SECURITY_RUNTIME_UNLOADING)
+static struct security_hook_heads security_undo_heads __lsm_ro_after_init;
+#else
+#define security_undo_heads security_hook_heads
+#endif
 static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
 
+#if defined(CONFIG_SECURITY_SELINUX_DISABLE) || defined(CONFIG_SECURITY_RUNTIME_UNLOADING)
+DEFINE_STATIC_SRCU(security_hook_srcu);
+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);
+}
+#else
+static inline int lock_lsm(void) { return 0; }
+static inline void unlock_lsm(int idx) { }
+#endif
+
 char *lsm_names;
 /* Boot-time LSM user choice */
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
@@ -182,6 +204,11 @@ int security_add_hooks(struct security_hook_list *hooks, int count,
 		    sizeof(struct security_hook_heads))
 			panic("Invalid offset when adding %s module", lsm);
 		hooks[i].lsm = lsm;
+#if defined(CONFIG_SECURITY_SELINUX_DISABLE) || defined(CONFIG_SECURITY_RUNTIME_UNLOADING)
+		head = (struct hlist_head *) (((char *) &security_undo_heads)
+					      + offset);
+		hlist_add_tail_rcu(&hooks[i].undo_list, head);
+#endif
 		head = (struct hlist_head *) (((char *) &security_hook_heads)
 					      + offset);
 		hlist_add_tail_rcu(&hooks[i].list, head);
@@ -189,6 +216,34 @@ int security_add_hooks(struct security_hook_list *hooks, int count,
 	return 0;
 }
 
+#if defined(CONFIG_SECURITY_SELINUX_DISABLE) || defined(CONFIG_SECURITY_RUNTIME_UNLOADING)
+/*
+ * Assuring the safety of deleting a security module is up to
+ * the security module involved. This may entail ordering the
+ * module's hook list in a particular way, refusing to disable
+ * the module once a policy is loaded or any number of other
+ * actions better imagined than described.
+ *
+ * The name of the configuration option reflects the only module
+ * that currently uses the mechanism. Any developer who thinks
+ * disabling their module is a good idea needs to be at least as
+ * careful as the SELinux team.
+ */
+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);
+	synchronize_srcu(&security_hook_srcu);
+	for (i = 0; i < count; i++)
+		hlist_del(&hooks[i].undo_list);
+}
+#if defined(CONFIG_SECURITY_RUNTIME_UNLOADING)
+EXPORT_SYMBOL_GPL(security_delete_hooks);
+#endif
+#endif /* CONFIG_SECURITY_SELINUX_DISABLE || CONFIG_SECURITY_RUNTIME_UNLOADING */
+
 int call_lsm_notifier(enum lsm_event event, void *data)
 {
 	return atomic_notifier_call_chain(&lsm_notifier_chain, event, data);
@@ -221,19 +276,23 @@ int unregister_lsm_notifier(struct notifier_block *nb)
 	do {							\
 		struct security_hook_list *P;			\
 								\
+		int srcu = lock_lsm();					\
 		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
 			P->hook.FUNC(__VA_ARGS__);		\
+		unlock_lsm(srcu);				\
 	} while (0)
 
 #define call_int_hook(FUNC, IRC, ...) ({			\
 	int RC = IRC;						\
 	struct security_hook_list *P;				\
+	int srcu = lock_lsm();					\
 								\
 	hlist_for_each_entry(P, &security_hook_heads.FUNC, list) {	\
 		RC = P->hook.FUNC(__VA_ARGS__);				\
 		if (RC != 0)						\
 			break;						\
 	}								\
+	unlock_lsm(srcu);						\
 	RC;								\
 })
 
@@ -241,12 +300,13 @@ int unregister_lsm_notifier(struct notifier_block *nb)
 	int RC = 0;							\
 	struct security_hook_list *P1;					\
 	struct security_hook_list *P2;					\
+	int srcu = lock_lsm();						\
 									\
 	hlist_for_each_entry(P1, &security_hook_heads.FUNC, list) {	\
 		RC = P1->hook.FUNC(__VA_ARGS__);			\
 		if (!RC)						\
 			continue;					\
-		hlist_for_each_entry(P2, &security_hook_heads.FUNC,	\
+		hlist_for_each_entry(P2, &security_undo_heads.FUNC,	\
 				     list) {				\
 			if (P1 == P2)					\
 				break;					\
@@ -255,6 +315,7 @@ int unregister_lsm_notifier(struct notifier_block *nb)
 		}							\
 		break;							\
 	}								\
+	unlock_lsm(srcu);						\
 	RC;								\
 })
 
@@ -267,21 +328,25 @@ int unregister_lsm_notifier(struct notifier_block *nb)
 #define call_first_void_hook(FUNC, ...)			\
 	do {							\
 		struct security_hook_list *P;			\
+		int srcu = lock_lsm();				\
 								\
 		hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
 			P->hook.FUNC(__VA_ARGS__);		\
 			break;					\
 		}						\
+		unlock_lsm(srcu);				\
 	} while (0)
 
 #define call_first_int_hook(FUNC, IRC, ...) ({			\
 	int RC = IRC;						\
 	struct security_hook_list *P;				\
+	int srcu = lock_lsm();					\
 								\
 	hlist_for_each_entry(P, &security_hook_heads.FUNC, list) {	\
 		RC = P->hook.FUNC(__VA_ARGS__);				\
 		break;							\
 	}								\
+	unlock_lsm(srcu);						\
 	RC;								\
 })
 
@@ -375,6 +440,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 = lock_lsm();
 
 	/*
 	 * The module will respond with a positive value if
@@ -390,6 +456,7 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages)
 			break;
 		}
 	}
+	unlock_lsm(srcu);
 	return __vm_enough_memory(mm, pages, cap_sys_admin);
 }
 
@@ -870,38 +937,44 @@ int security_inode_killpriv(struct dentry *dentry)
 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;
 
 	if (unlikely(IS_PRIVATE(inode)))
 		return -EOPNOTSUPP;
 	/*
 	 * Only one module will provide an attribute with a given name.
 	 */
+	srcu = lock_lsm();
 	hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
 		rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
 		if (rc != -EOPNOTSUPP)
-			return rc;
+			break;
 	}
-	return -EOPNOTSUPP;
+	unlock_lsm(srcu);
+	return rc;
 }
 
 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;
 
 	if (unlikely(IS_PRIVATE(inode)))
 		return -EOPNOTSUPP;
 	/*
 	 * Only one module will provide an attribute with a given name.
 	 */
+	srcu = lock_lsm();
 	hlist_for_each_entry(hp, &security_hook_heads.inode_setsecurity, list) {
 		rc = hp->hook.inode_setsecurity(inode, name, value, size,
 								flags);
 		if (rc != -EOPNOTSUPP)
-			return rc;
+			break;
 	}
-	return -EOPNOTSUPP;
+	unlock_lsm(srcu);
+	return rc;
 }
 
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size)
@@ -1206,6 +1279,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 	int thisrc;
 	int rc = -ENOSYS;
 	struct security_hook_list *hp;
+	int srcu = lock_lsm();
 
 	hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
 		thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
@@ -1215,6 +1289,7 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 				break;
 		}
 	}
+	unlock_lsm(srcu);
 	return rc;
 }
 
-- 
1.8.3.1
--
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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Sargun Dhillon]

On Fri, Apr 27, 2018 at 6:25 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> Sargun Dhillon wrote:
>> > inode_free_security is called on all LSM callbacks -- so, it should
>> > be, as long as people don't load unloadable major LSMs. If we want to
>> > be super conservative here, we can do a try_module_get, and a
>> > module_put if RC != 0 on RW hooks. So, we could have something like
>> > call_int_hook_major, and call_void_hook_major.
>
> I don't think we want to play with module usage count.
>
>> >
>> >>         }
>> >>         /*** End of changes made by this series ***/
>> >>         return 0;
>> >> }
>> >>
>> >> and what Casey's series is doing is
>> >>
>> >> int security_inode_alloc(struct inode *inode)
>> >> {
>> >>         struct security_hook_list *P;
>> >>         inode->i_security = NULL;
>> >>         hlist_for_each_entry(P, &security_hook_heads.inode_alloc_security, list) {
>> >>                 int RC = P->hook.inode_alloc_security(inode);
>> >>                 if (RC != 0) {
>> >>                         /*** Start of changes made by Casey's series ***/
>> >>                         hlist_for_each_entry(P, &security_hook_heads.inode_free_security, list) {
>> >>                                 P->hook.inode_free_security(inode); // <= Might call inode_free_security() without corresponding successful inode_alloc_security().
>> >>                         }
>> >>                         /*** End of changes made by Casey's series ***/
>> >>                         return RC;
>> >>                 }
>> >>         }
>> >>         return 0;
>> >> }
>> > Right, but this could be taken care of by just ensuring that nobody
>> > allocates blobs (major LSM), and only one LSM returns a non-zero to
>> > the *alloc* callbacks? Right now, we have this because one has to be a
>> > "major" LSM in order to use these callbacks, and we ensure only one
>> > major LSM is active at a time.
>
> What Casey is trying to do is allow multiple major modules to use their
> own blob. Thus,
>
>> >
>> > I suggest that either in the short term we:
>> > 1) Trust people not to load a second major LSM,
>
> this is not an option.
>
This is exactly what people do today?
>> > 2) See below.
>> >
>> > What about something as stupid as:
>
> I don't think we want to do this.
>
We have the limit today of not allowing people to load two major LSMs.
Why not wait till later to solve this problem, and for now, reject
when people install two major LSMs? I think we can fix the dynamic
loading problem _first_ and the multiple major LSM problem _second_

>> One other aspect that may not be obvious is on the *_alloc_*,
>> callbacks, we wouldn't want to call callback's of LSMs which are
>> non-major.
--
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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Sargun Dhillon]

On Wed, Apr 25, 2018 at 1:58 AM, Sargun Dhillon <sargun@sargun.me> wrote:
> The primary security benefit of this patchset is the introduction of
> read-only hooks, even if some security modules have mutable hooks.
> Currently, if you have any LSMs with mutable hooks it will render all
> heads, and list nodes mutable. These are a prime place to attack because
> being able to manipulate those hooks is a way to bypass all LSMs easily
> and to create a persistent, covert channel to intercept nearly all
> calls.
>
> There is a shares SRCU between all security hooks to facilitate safe
> LSM- unloading. 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.
>
> If CONFIG_SECURITY_WRITABLE_HOOKS is enabled, then hooks can be loaded
> at runtime. The module can check the return code of security_add_hooks
> to determine whether or not they can install their hooks, independently
> of checking for the Kconfig value. In the future, we may make it so that
> runtime hook loading is also disabled, after boot. We can do that in a
> follow-up patch.
>
> If CONFIG_SECURITY_UNREGISTRABLE_HOOKS is enabled, then hooks can be
> unloaded at runtime. This behaviour can be disabled by setting
> security.allow_unregister_hooks to 0. Once set, it requires a reboot
> to be reset.
>
> SELinux is exempt from the CONFIG_SECURITY_UNREGISTRABLE_HOOKS
> rule, and can unregister at any time. SELinux exposes a mechanism
> to disable itself, prior to policy loading. This can be disabled
> at compilation time, and it depends on CONFIG_SECURITY_WRITABLE_HOOKS.
> Changing this behaviour would require breaking the uapi.
>
> This patch can allow for a new style of LSMs. There are many cases
> where LSM "policies" would be better defined in some formal
> programming language, like C. This is either due to flexibility,
> or performance, for functions in the hot path.
>
> It also unlocks development usecases, but those come as a secondary
> benefit to the earlier feature. There has been an appetite for
> out-of-tree LSMs:
> http://kernsec.org/pipermail/linux-security-module-archive/2017-March/000119.html
> Casey's work has been great enabling minor LSM stacking, and a variety
> of LSMs have been proposed on list that haven't made it into the kernel
> but fit into the minor LSM stacking model.
>
> One of the other big changes is the introduction of the lsm_info
> structure. This encapsulates various information about the LSM
> itself, and can be used at a later time.
>
> This patch was tested with CONFIG_PROVE_LOCKING to prove the correctness
> of the locking in lock_existing_hooks. It grabs the module lock
> to prevent new hooks from being loaded, or current hooks from being
> unloaded. It then grabs the lsm_info_lock, which is only grabbed
> on hook modification, or inspection of the loaded LSMs.
>
> The other thing that this introduces is monitoring of LKM
> unloading. If an LSM's LKM is unloaded when it is not
> supposed to be, it will crash the system.
>
> Thanks to Tetsuo for their review, and feedback.
>
> Changes since:
> v6:
>   * introduce LSM info
>   * Crash by default instead of leaving it up to module authors
>     to deal with things going wrong
>   * Monitor module unloading, to ensure that attempts to
>     unload the LKM for an LSM crashes the system.
> v5:
>   * Rename "CONFIG_SECURITY_UNLOADABLE_MODULES" to
>     "CONFIG_SECURITY_UNREGISTRABLE_HOOKS" which enables arbitrary hook
>     deregistration. If disabled, hooks are not allowed to deregister.
>     SELinux is exempt.
>   * Rename security.allow_unload_hooks to
>     security.allow_unregister_hooks
>   * Make it so that both security_add_hooks, and security_delete_hooks
>     return errors upon being unsuccessful, allowing module authors to
>     make the best decision for their use case.
>   * Get rid of LSM_HOOK_INIT_MUTABLE
> v4:
>   * Introduce the configuration flag "CONFIG_SECURITY_UNLOADABLE_MODULES"
>     to disable module unloading
>   * Introduce the kernel parameter security.allow_unload_hooks
> v3:
>   * Instead of taking the approach of a "null hook", using the approach of
>     a second set of hooks -- this was mostly done through the
>     FOR_EACH_SECURITY_HOOK_MUTABLE macro, which gets compiled out if
>     CONFIG_SECURITY_WRITABLE_HOOKS is disabled.
> 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 (6):
>   security: Move LSM registration arguments to struct lsm_info
>   security: Make security_hook_heads private
>   security: Introduce mutable (RW) hooks
>   security: Expose security_add_hooks externally and add error handling
>   security: Panic on forced unloading of security module
>   security: Add SECURITY_UNREGISTRABLE_HOOKS to allow for hook removal
>
>  include/linux/lsm_hooks.h                     |  67 ++--
>  scripts/gcc-plugins/randomize_layout_plugin.c |   2 -
>  security/Kconfig                              |  12 +
>  security/apparmor/lsm.c                       |   8 +-
>  security/commoncap.c                          |   8 +-
>  security/inode.c                              |  56 +++-
>  security/loadpin/loadpin.c                    |   6 +-
>  security/security.c                           | 421 +++++++++++++++++++++-----
>  security/security.h                           |  11 +
>  security/selinux/hooks.c                      |  16 +-
>  security/smack/smack_lsm.c                    |   5 +-
>  security/tomoyo/tomoyo.c                      |   6 +-
>  security/yama/yama_lsm.c                      |   6 +-
>  13 files changed, 491 insertions(+), 133 deletions(-)
>  create mode 100644 security/security.h
>
> --
> 2.14.1
>
James, Casey,
Should I respin patches 1-5 with the fixes that Tetsuo suggested, and
do you want to pick those up? It seems like other than the few errors,
those look good. And then we can figure out to deal with patch 6
later?
--
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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Casey Schaufler]

On 4/27/2018 1:21 PM, Sargun Dhillon wrote:
> On Fri, Apr 27, 2018 at 6:25 AM, Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
>> Sargun Dhillon wrote:
>> ...
>>>> I suggest that either in the short term we:
>>>> 1) Trust people not to load a second major LSM,
>> this is not an option.
>>
> This is exactly what people do today?

The existing code provides no mechanism whereby multiple
"major" modules can be used at the same time. If you added
a "minor" module, and it used security blobs Bad Things(tm)
could happen.

>>>> 2) See below.
>>>>
>>>> What about something as stupid as:
>> I don't think we want to do this.
>>
> We have the limit today of not allowing people to load two major LSMs.
> Why not wait till later to solve this problem, and for now, reject
> when people install two major LSMs? I think we can fix the dynamic
> loading problem _first_ and the multiple major LSM problem _second_

I think that we're on the verge of having a major merge collision.
I hope to have the multiple major module code seriously reviewed as of
4.18 and start putting real pressure on getting it in for 4.19/4.20.
The advent of the Age of Containers is driving this.

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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Casey Schaufler]

On 4/27/2018 1:32 PM, Sargun Dhillon wrote:
> On Wed, Apr 25, 2018 at 1:58 AM, Sargun Dhillon <sargun@sargun.me> wrote:
>> The primary security benefit of this patchset is the introduction of
>> read-only hooks, even if some security modules have mutable hooks.
>> ...
>>
> James, Casey,
> Should I respin patches 1-5 with the fixes that Tetsuo suggested, and
> do you want to pick those up? It seems like other than the few errors,
> those look good. And then we can figure out to deal with patch 6
> later?

It is my firm hope that the multiple major module changes are
going to start being seriously considered in the next release or
two. That would reduce the complexity of what you're trying to
accomplish because at the point all modules will be equal. I have
always committed to making design choices that aren't going to
prevent loadable/unloadable modules. I have also expressed no
interest in doing it myself. From my selfish perspective I would
like to see module loading follow my work, as having yet another
major merge effort will delay the clean-up I know I'll have to do
after 4.18.

My suggestion/request would be that you rebase your patches on
the stacking set, which should be out for 4.18 mid-week.



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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Tetsuo Handa]

Casey Schaufler wrote:
> I think that we're on the verge of having a major merge collision.

I think that the reason of major merge collision is that your patchset is
too big to review.

  > That's a lot of overhead. The smaller the patches, the more work
  > required to make the patch set. You're right that the current patches
  > are too big. Moving forward I will be providing smaller, easier to
  > deal with patches. The sheer overhead of rebaseing a large patch set
  > has slowed down the development considerably. Minions. What I need are
  > minions.

You are holding the global lock for patchset which you did not get response.
If you agree with breaking into smaller patches, we will be able to solve
the major merge collision.

> I hope to have the multiple major module code seriously reviewed as of
> 4.18 and start putting real pressure on getting it in for 4.19/4.20.

Before you repost your patchset, please answer questions below.

Q1: Apart from whether memory for security blobs should be managed by
    infrastructure or by individual module, how do you guarantee that
    only release hook where corresponding alloc hook succeeded when
    one of alloc hooks returned an error?

My patch 1/2 provides that mechanism. Please show your patch which
implements only that functionality instead of whole patchset.

  > It is my firm hope that the multiple major module changes are
  > going to start being seriously considered in the next release or
  > two. That would reduce the complexity of what you're trying to
  > accomplish because at the point all modules will be equal. I have
  > always committed to making design choices that aren't going to
  > prevent loadable/unloadable modules. I have also expressed no
  > interest in doing it myself. From my selfish perspective I would
  > like to see module loading follow my work, as having yet another
  > major merge effort will delay the clean-up I know I'll have to do
  > after 4.18.

Depending on your patch, it is possible that managing memory for security
blobs by infrastructure can become harmful for LKM-based LSMs, for the
amount of memory which will be needed by LKM-based LSMs is not known
until LKM-based LSMs are loaded.

LKM-based LSMs might be forced to manage their security blobs without
using infrastructure-managed ->security field. Then, that will not make
"all modules equal".

So, please show your approach which allows LKM-based LSMs to handle
infrastructure-managed ->security field (if you want to use
infrastructure-managed ->security field).

> The advent of the Age of Containers is driving this.

I've been asking for runtime loading of LSMs before the Age of
Containers comes. ;-)

Q2: What functionality are Containers people want LSM?
    Use different LSM module for different container?
    Runtime load/unload of LSM modules?
--
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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Casey Schaufler]

On 4/29/2018 4:49 AM, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>> I think that we're on the verge of having a major merge collision.
> I think that the reason of major merge collision is that your patchset is
> too big to review.

My patch set is large because it has to address all of the
aspects of multiple modules running on the system before some
important members of the community will look at it seriously.

>
>   > That's a lot of overhead. The smaller the patches, the more work
>   > required to make the patch set. You're right that the current patches
>   > are too big. Moving forward I will be providing smaller, easier to
>   > deal with patches. The sheer overhead of rebaseing a large patch set
>   > has slowed down the development considerably. Minions. What I need are
>   > minions.
>
> You are holding the global lock for patchset which you did not get response.
> If you agree with breaking into smaller patches, we will be able to solve
> the major merge collision.

I will be presenting the set of smaller patches once I finish merging
them into 4.18. It's a bigger jobs than usual because of the change from
lists to hlists, the significant changes to AppArmor and the early stages
of SELinux namespacing. I hope to have it done mid week, but the changes
need to get tested, and there are lots of configurations involved.

I am perfectly amenable to the patches going in in logical stages.
In fact, that is what I would recommend.

>> I hope to have the multiple major module code seriously reviewed as of
>> 4.18 and start putting real pressure on getting it in for 4.19/4.20.
> Before you repost your patchset, please answer questions below.
>
> Q1: Apart from whether memory for security blobs should be managed by
>     infrastructure or by individual module, how do you guarantee that
>     only release hook where corresponding alloc hook succeeded when
>     one of alloc hooks returned an error?

The infrastructure allocates a zeroed blob. Module "alloc" hooks
are called. The module specific hooks set their data as desired.
If a module alloc hook fails the complete set of free hooks can
be called because the module data will either have been set or
will be still be zeroed. This means that module free hooks have
to accommodate the possibility the the module specific data was
never set.

The reality is that many of the free hooks go away with the
infrastructure blob management. Those that remain can easily
be confirmed to be safe in the case where all the modules data
is NULL.
?

> My patch 1/2 provides that mechanism. Please show your patch which
> implements only that functionality instead of whole patchset.

It's not an explicit change. It is an artifact of the infrastructure
allocating and freeing the data. You can't tease the removal of the
module free hooks apart from the infrastructure freeing the data.

>   > It is my firm hope that the multiple major module changes are
>   > going to start being seriously considered in the next release or
>   > two. That would reduce the complexity of what you're trying to
>   > accomplish because at the point all modules will be equal. I have
>   > always committed to making design choices that aren't going to
>   > prevent loadable/unloadable modules. I have also expressed no
>   > interest in doing it myself. From my selfish perspective I would
>   > like to see module loading follow my work, as having yet another
>   > major merge effort will delay the clean-up I know I'll have to do
>   > after 4.18.
>
> Depending on your patch, it is possible that managing memory for security
> blobs by infrastructure can become harmful for LKM-based LSMs, for the
> amount of memory which will be needed by LKM-based LSMs is not known
> until LKM-based LSMs are loaded.

It's certainly not more "harmful" than the single blob mechanism we
have now. The patches I'm proposing don't make the situation any worse
than it is today. As I've maintained all along, I'm not trying to
address LKM-based modules.

There is no reason I can see that blob sizes couldn't change while
a system is running (yes, I have looked into it) provided that the
blob size and content list is recorded in the blob and that the dynamic
modules check this information before trying to use it.

	smack_inode(struct inode *inode)
	{
		return inode->i_security + smack_blob_sizes.lbs_inode;
	}

	lkm_based_module_inode(struct inode *inode)
	{
		struct lsm_inode_blob_header *ibh = inode->security;
		if (some_appropriate_check(ibh, LKM_BASED_MODULE_ID))
			return inode->i_security + lkm_based_module_blob_sizes.lbs_inode;
		/* No blob for this object */
		return NULL;
	}

A refinement to this might keep the offset for dynamic module data
in the blob header. That would be more flexible, but would consume
more memory for each blob.

> LKM-based LSMs might be forced to manage their security blobs without
> using infrastructure-managed ->security field. Then, that will not make
> "all modules equal".

Regardless of how the data is managed the information about how much
is being used and where to find it has to be kept somewhere. With
static modules it is easier because you only need to figure that out
once, at the beginning. If you're going to have blob data changing
you have to look it up or figure it out at every reference. It's
not impossible. It's not even really that hard, but it won't be
cheap.

> So, please show your approach which allows LKM-based LSMs to handle
> infrastructure-managed ->security field (if you want to use
> infrastructure-managed ->security field).

I'll repeat, I don't have code to do it because I have explicitly
excluded lkm-based modules from my todo list. If I were doing it,
I would propose an infrastructure managed header in the security
blobs containing sufficient information for the modules to determine
if the data they are looking for is present. lkm-based modules would
have to be coded to accept that they might get blobs that were created
before the module was loaded, and that do not contain data for that
module.

The blob header mechanism would only be included if lkm-based security
module support was configured.

>> The advent of the Age of Containers is driving this.
> I've been asking for runtime loading of LSMs before the Age of
> Containers comes. ;-)

Yes, you have!

> Q2: What functionality are Containers people want LSM?

It varies. Container people are a diverse lot.

>     Use different LSM module for different container?

That is definitely one use case. You might run Smack on the base
system and label each container based on who payed for it, and
let the security module set within the container be whatever the
job demands.

>     Runtime load/unload of LSM modules?

I would see that as something you might do on container
startup, but managing the module set could get ugly.

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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Sargun Dhillon]

On Sun, Apr 29, 2018 at 2:23 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/29/2018 4:49 AM, Tetsuo Handa wrote:
>> Casey Schaufler wrote:
>>> I think that we're on the verge of having a major merge collision.
>> I think that the reason of major merge collision is that your patchset is
>> too big to review.
>
> My patch set is large because it has to address all of the
> aspects of multiple modules running on the system before some
> important members of the community will look at it seriously.
>
>>
>>   > That's a lot of overhead. The smaller the patches, the more work
>>   > required to make the patch set. You're right that the current patches
>>   > are too big. Moving forward I will be providing smaller, easier to
>>   > deal with patches. The sheer overhead of rebaseing a large patch set
>>   > has slowed down the development considerably. Minions. What I need are
>>   > minions.
>>
>> You are holding the global lock for patchset which you did not get response.
>> If you agree with breaking into smaller patches, we will be able to solve
>> the major merge collision.
>
> I will be presenting the set of smaller patches once I finish merging
> them into 4.18. It's a bigger jobs than usual because of the change from
> lists to hlists, the significant changes to AppArmor and the early stages
> of SELinux namespacing. I hope to have it done mid week, but the changes
> need to get tested, and there are lots of configurations involved.
>
> I am perfectly amenable to the patches going in in logical stages.
> In fact, that is what I would recommend.
>
I guess I'm just a little bit frustrated, because, in my mind, some of
my patches provide immediate value, and are ready to be reviewed, and
or respun. Testuo did a good job reviewing 1-5, and I think that if
those are his only issues, then we're good to go.
I've:
1) Do safe hook loading for sane security modules (those that don't
try to overload major hooks), without compromising the security of
other hooks
2) Suggested a whitelist of major hooks to prevent insane modules from
being loaded

If Casey's patches land, we can just not do (2), and we're good to go.

>>> I hope to have the multiple major module code seriously reviewed as of
>>> 4.18 and start putting real pressure on getting it in for 4.19/4.20.
>> Before you repost your patchset, please answer questions below.
>>
>> Q1: Apart from whether memory for security blobs should be managed by
>>     infrastructure or by individual module, how do you guarantee that
>>     only release hook where corresponding alloc hook succeeded when
>>     one of alloc hooks returned an error?
>
> The infrastructure allocates a zeroed blob. Module "alloc" hooks
> are called. The module specific hooks set their data as desired.
> If a module alloc hook fails the complete set of free hooks can
> be called because the module data will either have been set or
> will be still be zeroed. This means that module free hooks have
> to accommodate the possibility the the module specific data was
> never set.
>
> The reality is that many of the free hooks go away with the
> infrastructure blob management. Those that remain can easily
> be confirmed to be safe in the case where all the modules data
> is NULL.
>
>
>> My patch 1/2 provides that mechanism. Please show your patch which
>> implements only that functionality instead of whole patchset.
>
> It's not an explicit change. It is an artifact of the infrastructure
> allocating and freeing the data. You can't tease the removal of the
> module free hooks apart from the infrastructure freeing the data.
>
>>   > It is my firm hope that the multiple major module changes are
>>   > going to start being seriously considered in the next release or
>>   > two. That would reduce the complexity of what you're trying to
>>   > accomplish because at the point all modules will be equal. I have
>>   > always committed to making design choices that aren't going to
>>   > prevent loadable/unloadable modules. I have also expressed no
>>   > interest in doing it myself. From my selfish perspective I would
>>   > like to see module loading follow my work, as having yet another
>>   > major merge effort will delay the clean-up I know I'll have to do
>>   > after 4.18.
>>
>> Depending on your patch, it is possible that managing memory for security
>> blobs by infrastructure can become harmful for LKM-based LSMs, for the
>> amount of memory which will be needed by LKM-based LSMs is not known
>> until LKM-based LSMs are loaded.
>
> It's certainly not more "harmful" than the single blob mechanism we
> have now. The patches I'm proposing don't make the situation any worse
> than it is today. As I've maintained all along, I'm not trying to
> address LKM-based modules.
>
> There is no reason I can see that blob sizes couldn't change while
> a system is running (yes, I have looked into it) provided that the
> blob size and content list is recorded in the blob and that the dynamic
> modules check this information before trying to use it.
>
>         smack_inode(struct inode *inode)
>         {
>                 return inode->i_security + smack_blob_sizes.lbs_inode;
>         }
>
>         lkm_based_module_inode(struct inode *inode)
>         {
>                 struct lsm_inode_blob_header *ibh = inode->security;
>                 if (some_appropriate_check(ibh, LKM_BASED_MODULE_ID))
>                         return inode->i_security + lkm_based_module_blob_sizes.lbs_inode;
>                 /* No blob for this object */
>                 return NULL;
>         }
>
> A refinement to this might keep the offset for dynamic module data
> in the blob header. That would be more flexible, but would consume
> more memory for each blob.
>
>> LKM-based LSMs might be forced to manage their security blobs without
>> using infrastructure-managed ->security field. Then, that will not make
>> "all modules equal".
>
> Regardless of how the data is managed the information about how much
> is being used and where to find it has to be kept somewhere. With
> static modules it is easier because you only need to figure that out
> once, at the beginning. If you're going to have blob data changing
> you have to look it up or figure it out at every reference. It's
> not impossible. It's not even really that hard, but it won't be
> cheap.
>
>> So, please show your approach which allows LKM-based LSMs to handle
>> infrastructure-managed ->security field (if you want to use
>> infrastructure-managed ->security field).
>
> I'll repeat, I don't have code to do it because I have explicitly
> excluded lkm-based modules from my todo list. If I were doing it,
> I would propose an infrastructure managed header in the security
> blobs containing sufficient information for the modules to determine
> if the data they are looking for is present. lkm-based modules would
> have to be coded to accept that they might get blobs that were created
> before the module was loaded, and that do not contain data for that
> module.
>
> The blob header mechanism would only be included if lkm-based security
> module support was configured.
>
>>> The advent of the Age of Containers is driving this.
>> I've been asking for runtime loading of LSMs before the Age of
>> Containers comes. ;-)
>
> Yes, you have!
>
>> Q2: What functionality are Containers people want LSM?
>
> It varies. Container people are a diverse lot.
>
>>     Use different LSM module for different container?
>
One dumb use case I have is the ability to limit interactions with
PTYs for containerized applications. Another aspect of this is being
able to write policies in C, and actually being able to control the
nitty gritty of what's going on, versus actively fighting with the
LSM(s).

> That is definitely one use case. You might run Smack on the base
> system and label each container based on who payed for it, and
> let the security module set within the container be whatever the
> job demands.
>
>>     Runtime load/unload of LSM modules?
>
> I would see that as something you might do on container
> startup, but managing the module set could get ugly.
>
I can forgo this, if neccessary. I see benefits around it for surgical
fixes, specifically, CVEs, but I can understand if it's not worth the
complexity tradeoff.
--
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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Casey Schaufler]

On 4/30/2018 9:11 AM, Sargun Dhillon wrote:
> On Sun, Apr 29, 2018 at 2:23 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 4/29/2018 4:49 AM, Tetsuo Handa wrote:
>>> Casey Schaufler wrote:
>>>> I think that we're on the verge of having a major merge collision.
>>> I think that the reason of major merge collision is that your patchset is
>>> too big to review.
>> My patch set is large because it has to address all of the
>> aspects of multiple modules running on the system before some
>> important members of the community will look at it seriously.
>>
>>>   > That's a lot of overhead. The smaller the patches, the more work
>>>   > required to make the patch set. You're right that the current patches
>>>   > are too big. Moving forward I will be providing smaller, easier to
>>>   > deal with patches. The sheer overhead of rebaseing a large patch set
>>>   > has slowed down the development considerably. Minions. What I need are
>>>   > minions.
>>>
>>> You are holding the global lock for patchset which you did not get response.
>>> If you agree with breaking into smaller patches, we will be able to solve
>>> the major merge collision.
>> I will be presenting the set of smaller patches once I finish merging
>> them into 4.18. It's a bigger jobs than usual because of the change from
>> lists to hlists, the significant changes to AppArmor and the early stages
>> of SELinux namespacing. I hope to have it done mid week, but the changes
>> need to get tested, and there are lots of configurations involved.
>>
>> I am perfectly amenable to the patches going in in logical stages.
>> In fact, that is what I would recommend.
>>
> I guess I'm just a little bit frustrated, because, in my mind, some of
> my patches provide immediate value, and are ready to be reviewed, and
> or respun. Testuo did a good job reviewing 1-5, and I think that if
> those are his only issues, then we're good to go.
> I've:
> 1) Do safe hook loading for sane security modules (those that don't
> try to overload major hooks), without compromising the security of
> other hooks
> 2) Suggested a whitelist of major hooks to prevent insane modules from
> being loaded
>
> If Casey's patches land, we can just not do (2), and we're good to go.

I understand your frustration. As much as I would like to
avoid having the stacking work take any longer than it already
has (I *do* have a boatload of other stuff I want to get done)
I suggest that if you can get consensus that the lkm-based
security module work should be accepted that you go forward and
get it in. I know that I have at least one major round of review
before the stacking can really be seriously considered. I knew
the job was dangerous when I took it.


>>>> ...
> One dumb use case I have is the ability to limit interactions with
> PTYs for containerized applications. Another aspect of this is being
> able to write policies in C, and actually being able to control the
> nitty gritty of what's going on, versus actively fighting with the
> LSM(s).

You don't need lkm-based modules to do that, but I can see
why you might want to do it that way. It's also something that
I would expect LandLock to be a solution for, but again you'd
need to be running LandLock when you identified the issue.

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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Sargun Dhillon]

On Mon, Apr 30, 2018 at 9:46 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 4/30/2018 9:11 AM, Sargun Dhillon wrote:
>> On Sun, Apr 29, 2018 at 2:23 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>> On 4/29/2018 4:49 AM, Tetsuo Handa wrote:
>>>> Casey Schaufler wrote:
>>>>> I think that we're on the verge of having a major merge collision.
>>>> I think that the reason of major merge collision is that your patchset is
>>>> too big to review.
>>> My patch set is large because it has to address all of the
>>> aspects of multiple modules running on the system before some
>>> important members of the community will look at it seriously.
>>>
>>>>   > That's a lot of overhead. The smaller the patches, the more work
>>>>   > required to make the patch set. You're right that the current patches
>>>>   > are too big. Moving forward I will be providing smaller, easier to
>>>>   > deal with patches. The sheer overhead of rebaseing a large patch set
>>>>   > has slowed down the development considerably. Minions. What I need are
>>>>   > minions.
>>>>
>>>> You are holding the global lock for patchset which you did not get response.
>>>> If you agree with breaking into smaller patches, we will be able to solve
>>>> the major merge collision.
>>> I will be presenting the set of smaller patches once I finish merging
>>> them into 4.18. It's a bigger jobs than usual because of the change from
>>> lists to hlists, the significant changes to AppArmor and the early stages
>>> of SELinux namespacing. I hope to have it done mid week, but the changes
>>> need to get tested, and there are lots of configurations involved.
>>>
>>> I am perfectly amenable to the patches going in in logical stages.
>>> In fact, that is what I would recommend.
>>>
>> I guess I'm just a little bit frustrated, because, in my mind, some of
>> my patches provide immediate value, and are ready to be reviewed, and
>> or respun. Testuo did a good job reviewing 1-5, and I think that if
>> those are his only issues, then we're good to go.
>> I've:
>> 1) Do safe hook loading for sane security modules (those that don't
>> try to overload major hooks), without compromising the security of
>> other hooks
>> 2) Suggested a whitelist of major hooks to prevent insane modules from
>> being loaded
>>
>> If Casey's patches land, we can just not do (2), and we're good to go.
>
> I understand your frustration. As much as I would like to
> avoid having the stacking work take any longer than it already
> has (I *do* have a boatload of other stuff I want to get done)
> I suggest that if you can get consensus that the lkm-based
> security module work should be accepted that you go forward and
> get it in. I know that I have at least one major round of review
> before the stacking can really be seriously considered. I knew
> the job was dangerous when I took it.
>
>
Curious, consensus from whom? I thought you owned this part of the tree.
>>>>> ...
>> One dumb use case I have is the ability to limit interactions with
>> PTYs for containerized applications. Another aspect of this is being
>> able to write policies in C, and actually being able to control the
>> nitty gritty of what's going on, versus actively fighting with the
>> LSM(s).
>
> You don't need lkm-based modules to do that, but I can see
> why you might want to do it that way. It's also something that
> I would expect LandLock to be a solution for, but again you'd
> need to be running LandLock when you identified the issue.
>
I've been a long-time proponent of programmable LSMs (before it was
cool): https://lkml.org/lkml/2016/8/4/58


Do you know where Landlock is in terms of getting merged?
--
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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Casey Schaufler]

On 4/30/2018 11:25 AM, Sargun Dhillon wrote:
> On Mon, Apr 30, 2018 at 9:46 AM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>> On 4/30/2018 9:11 AM, Sargun Dhillon wrote:
>>> On Sun, Apr 29, 2018 at 2:23 PM, Casey Schaufler <casey@schaufler-ca.com> wrote:
>>>> On 4/29/2018 4:49 AM, Tetsuo Handa wrote:
>>>>> Casey Schaufler wrote:
>>>>>> I think that we're on the verge of having a major merge collision.
>>>>> I think that the reason of major merge collision is that your patchset is
>>>>> too big to review.
>>>> My patch set is large because it has to address all of the
>>>> aspects of multiple modules running on the system before some
>>>> important members of the community will look at it seriously.
>>>>
>>>>>   > That's a lot of overhead. The smaller the patches, the more work
>>>>>   > required to make the patch set. You're right that the current patches
>>>>>   > are too big. Moving forward I will be providing smaller, easier to
>>>>>   > deal with patches. The sheer overhead of rebaseing a large patch set
>>>>>   > has slowed down the development considerably. Minions. What I need are
>>>>>   > minions.
>>>>>
>>>>> You are holding the global lock for patchset which you did not get response.
>>>>> If you agree with breaking into smaller patches, we will be able to solve
>>>>> the major merge collision.
>>>> I will be presenting the set of smaller patches once I finish merging
>>>> them into 4.18. It's a bigger jobs than usual because of the change from
>>>> lists to hlists, the significant changes to AppArmor and the early stages
>>>> of SELinux namespacing. I hope to have it done mid week, but the changes
>>>> need to get tested, and there are lots of configurations involved.
>>>>
>>>> I am perfectly amenable to the patches going in in logical stages.
>>>> In fact, that is what I would recommend.
>>>>
>>> I guess I'm just a little bit frustrated, because, in my mind, some of
>>> my patches provide immediate value, and are ready to be reviewed, and
>>> or respun. Testuo did a good job reviewing 1-5, and I think that if
>>> those are his only issues, then we're good to go.
>>> I've:
>>> 1) Do safe hook loading for sane security modules (those that don't
>>> try to overload major hooks), without compromising the security of
>>> other hooks
>>> 2) Suggested a whitelist of major hooks to prevent insane modules from
>>> being loaded
>>>
>>> If Casey's patches land, we can just not do (2), and we're good to go.
>> I understand your frustration. As much as I would like to
>> avoid having the stacking work take any longer than it already
>> has (I *do* have a boatload of other stuff I want to get done)
>> I suggest that if you can get consensus that the lkm-based
>> security module work should be accepted that you go forward and
>> get it in. I know that I have at least one major round of review
>> before the stacking can really be seriously considered. I knew
>> the job was dangerous when I took it.
>>
>>
> Curious, consensus from whom? I thought you owned this part of the tree.

James Morris owns the security tree in general, while various
other people own the module sub-trees. Consensus would require
at least James, John Johansen (AppArmor), Paul Moore (SELinux,
Audit and Netlabel), Tetsuo Handa (TOMOYO), Kees Cook (Yama, LoadPin),
Serge Hallyn (Capabilities), Mimi Zohar (Integrity), David Howells
(Keys) and Myself (Smack). Beyond that, there will have to be
buy in from many vocal developers, not the least of which is Linus.

I don't own this part of the tree, but I have fixed it up a bit.

>>>>>> ...
>>>
>>
>
> Do you know where Landlock is in terms of getting merged?

Probably behind the general module stacking. :-o
It's a prime example of a major module that would
be used in conjunction with other major modules.

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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[James Morris]

On Mon, 30 Apr 2018, Sargun Dhillon wrote:

> I guess I'm just a little bit frustrated, because, in my mind, some of
> my patches provide immediate value, and are ready to be reviewed, and
> or respun.

I'm not seeing much value in this functionality, given that SELinux is the 
only unloadable LSM, and that is really just an historical workaround 
which may be normalized at some point.

Patch 1 may be useful on its own.

-- 
James Morris
<jmorris@namei.org>

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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[Sargun Dhillon]

On Mon, Apr 30, 2018 at 2:16 PM, James Morris <jmorris@namei.org> wrote:
> On Mon, 30 Apr 2018, Sargun Dhillon wrote:
>
>> I guess I'm just a little bit frustrated, because, in my mind, some of
>> my patches provide immediate value, and are ready to be reviewed, and
>> or respun.
>
> I'm not seeing much value in this functionality, given that SELinux is the
> only unloadable LSM, and that is really just an historical workaround
> which may be normalized at some point.
>
> Patch 1 may be useful on its own.
Do you not think patch 2 is also useful? Is it worth me re-rolling 1-2
independently?

Do you think not think that minor loadable LSMs are valuable? -- And
if so, do you think it's okay with, or without guardrails?
>
> --
> James Morris
> <jmorris@namei.org>
>
--
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

[PATCH v7 1/6] security: Move LSM registration arguments to struct lsm_info

[James Morris]

On Wed, 25 Apr 2018, Sargun Dhillon wrote:

> +	/* This field is not currently in use */
> +	struct lsm_info			*info;

It does seem to be in use.

>  static const struct file_operations lsm_ops = {
> -	.read = lsm_read,
> -	.llseek = generic_file_llseek,
> +	.open		= lsm_ops_open,
> +	.read		= seq_read,
> +	.llseek		= seq_lseek,
> +	.release	= seq_release,
>  };

Please mention this change in the changelog, or probably better, make it a 
separate patch.


-- 
James Morris
<jmorris@namei.org>

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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[James Morris]

On Mon, 30 Apr 2018, Sargun Dhillon wrote:

> On Mon, Apr 30, 2018 at 2:16 PM, James Morris <jmorris@namei.org> wrote:
> > On Mon, 30 Apr 2018, Sargun Dhillon wrote:
> >
> >> I guess I'm just a little bit frustrated, because, in my mind, some of
> >> my patches provide immediate value, and are ready to be reviewed, and
> >> or respun.
> >
> > I'm not seeing much value in this functionality, given that SELinux is the
> > only unloadable LSM, and that is really just an historical workaround
> > which may be normalized at some point.
> >
> > Patch 1 may be useful on its own.
> Do you not think patch 2 is also useful? Is it worth me re-rolling 1-2
> independently?
> 

Yes, please split out the seq file change and submit them as independent 
changes.

> Do you think not think that minor loadable LSMs are valuable? -- And
> if so, do you think it's okay with, or without guardrails?

Potentially, but we can't add infrastructure for non-existent or out of 
tree code.



-- 
James Morris
<jmorris@namei.org>

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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[James Morris]

On Mon, 30 Apr 2018, Sargun Dhillon wrote:

> > It varies. Container people are a diverse lot.
> >
> >>     Use different LSM module for different container?
> >
> One dumb use case I have is the ability to limit interactions with
> PTYs for containerized applications. Another aspect of this is being
> able to write policies in C, and actually being able to control the
> nitty gritty of what's going on, versus actively fighting with the
> LSM(s).

Writing policies in C sounds like a breach of the principle of separating 
mechanism and policy.  This has been an important aspect of the LSM API 
and also of the major LSMs.  Hard-coded security policy is likely to be 
brittle, inflexible, and difficult to manage.  It's also blurring the 
boundary of LSM with the rest of the kernel, and making it even more 
difficult for core kernel developers to know what might break in LSM land 
when they make changes elsewhere.

Can you provide an example of what you want to do?


-- 
James Morris
<jmorris@namei.org>

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

[PATCH v7 1/6] security: Move LSM registration arguments to struct lsm_info

[Kees Cook]

On Wed, Apr 25, 2018 at 1:58 AM, Sargun Dhillon <sargun@sargun.me> wrote:
> Previously, when LSMs registered, they independently passed their name
> and hook count. This had two implications:
>
> 1) Is required us to clone the name, so we could present it in
>    security FS. This required memory allocation at start time.
> 2) Every time we wanted to tie more information back from
>    the security hooks, to the LSM, we would have to add
>    duplicated fields in struct security_hook_list.
>
> It also introduces a new file -- security/security.h, which is meant
> to be private headers to be shared only between pieces of security
> "infrastructure".
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
>  include/linux/lsm_hooks.h  | 44 ++++++++++-------------
>  security/apparmor/lsm.c    |  6 ++--
>  security/commoncap.c       |  8 +++--
>  security/inode.c           | 56 +++++++++++++++++++++++++----
>  security/loadpin/loadpin.c |  4 ++-
>  security/security.c        | 89 +++++++++++++++++++++++-----------------------
>  security/security.h        | 10 ++++++
>  security/selinux/hooks.c   |  6 ++--
>  security/smack/smack_lsm.c |  3 +-
>  security/tomoyo/tomoyo.c   |  4 ++-
>  security/yama/yama_lsm.c   |  4 ++-
>  11 files changed, 147 insertions(+), 87 deletions(-)
>  create mode 100644 security/security.h
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 9d0b286f3dba..65f346cb6639 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2004,11 +2004,20 @@ struct security_hook_heads {
>   * Security module hook list structure.
>   * For use with generic list macros for common operations.
>   */
> +struct security_hook_list;
> +struct lsm_info {
> +       struct hlist_node               list;
> +       const char                      *name;
> +       const unsigned int              count;
> +       struct security_hook_list       *hooks;
> +} __randomize_layout;
> +
>  struct security_hook_list {
>         struct hlist_node               list;
>         struct hlist_head               *head;
>         union security_list_options     hook;
> -       char                            *lsm;
> +       /* This field is not currently in use */
> +       struct lsm_info                 *info;

const?

>  } __randomize_layout;
>
>  /*
> @@ -2020,33 +2029,18 @@ struct security_hook_list {
>  #define LSM_HOOK_INIT(HEAD, HOOK) \
>         { .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } }
>
> -extern struct security_hook_heads security_hook_heads;
> -extern char *lsm_names;
> +#define LSM_MODULE_INIT(NAME, HOOKS)           \
> +       {                                       \
> +               .name   = NAME,                 \
> +               .hooks  = HOOKS,                \
> +               .count  = ARRAY_SIZE(HOOKS),    \
> +       }

Instead of leaving this so every LSM has to do all the declarations, how about:

#define LSM_MODULE(NAME) \
    static const struct lsm_info NAME ## _info = { \
        .name = #NAME, \
        .hooks = NAME ## _hooks, \
        .count = ARRAY_SIZE(NAME ## _hooks), \
    }

> +static struct lsm_info apparmor_info =
> +       LSM_MODULE_INIT("apparmor", apparmor_hooks);

This becomes just:

LSM_MODULE(apparmor);

> +static struct lsm_info capability_info =
> +       LSM_MODULE_INIT("capability", capability_hooks);

LSM_MODULE(capability);

etc...

-Kees

-- 
Kees Cook
Pixel Security
--
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

[PATCH v7 1/6] security: Move LSM registration arguments to struct lsm_info

[Sargun Dhillon]

On Tue, May 1, 2018 at 12:19 PM, Kees Cook <keescook@chromium.org> wrote:
> On Wed, Apr 25, 2018 at 1:58 AM, Sargun Dhillon <sargun@sargun.me> wrote:
>> Previously, when LSMs registered, they independently passed their name
>> and hook count. This had two implications:
>>
>> 1) Is required us to clone the name, so we could present it in
>>    security FS. This required memory allocation at start time.
>> 2) Every time we wanted to tie more information back from
>>    the security hooks, to the LSM, we would have to add
>>    duplicated fields in struct security_hook_list.
>>
>> It also introduces a new file -- security/security.h, which is meant
>> to be private headers to be shared only between pieces of security
>> "infrastructure".
>>
>> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
>> ---
>>  include/linux/lsm_hooks.h  | 44 ++++++++++-------------
>>  security/apparmor/lsm.c    |  6 ++--
>>  security/commoncap.c       |  8 +++--
>>  security/inode.c           | 56 +++++++++++++++++++++++++----
>>  security/loadpin/loadpin.c |  4 ++-
>>  security/security.c        | 89 +++++++++++++++++++++++-----------------------
>>  security/security.h        | 10 ++++++
>>  security/selinux/hooks.c   |  6 ++--
>>  security/smack/smack_lsm.c |  3 +-
>>  security/tomoyo/tomoyo.c   |  4 ++-
>>  security/yama/yama_lsm.c   |  4 ++-
>>  11 files changed, 147 insertions(+), 87 deletions(-)
>>  create mode 100644 security/security.h
>>
>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> index 9d0b286f3dba..65f346cb6639 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -2004,11 +2004,20 @@ struct security_hook_heads {
>>   * Security module hook list structure.
>>   * For use with generic list macros for common operations.
>>   */
>> +struct security_hook_list;
>> +struct lsm_info {
>> +       struct hlist_node               list;
>> +       const char                      *name;
>> +       const unsigned int              count;
>> +       struct security_hook_list       *hooks;
>> +} __randomize_layout;
>> +
>>  struct security_hook_list {
>>         struct hlist_node               list;
>>         struct hlist_head               *head;
>>         union security_list_options     hook;
>> -       char                            *lsm;
>> +       /* This field is not currently in use */
>> +       struct lsm_info                 *info;
>
> const?
>
>>  } __randomize_layout;
>>
>>  /*
>> @@ -2020,33 +2029,18 @@ struct security_hook_list {
>>  #define LSM_HOOK_INIT(HEAD, HOOK) \
>>         { .head = &security_hook_heads.HEAD, .hook = { .HEAD = HOOK } }
>>
>> -extern struct security_hook_heads security_hook_heads;
>> -extern char *lsm_names;
>> +#define LSM_MODULE_INIT(NAME, HOOKS)           \
>> +       {                                       \
>> +               .name   = NAME,                 \
>> +               .hooks  = HOOKS,                \
>> +               .count  = ARRAY_SIZE(HOOKS),    \
>> +       }
>
> Instead of leaving this so every LSM has to do all the declarations, how about:
>
> #define LSM_MODULE(NAME) \
>     static const struct lsm_info NAME ## _info = { \
>         .name = #NAME, \
>         .hooks = NAME ## _hooks, \
>         .count = ARRAY_SIZE(NAME ## _hooks), \
>     }
>
>> +static struct lsm_info apparmor_info =
>> +       LSM_MODULE_INIT("apparmor", apparmor_hooks);
>
> This becomes just:
>
> LSM_MODULE(apparmor);
>
>> +static struct lsm_info capability_info =
>> +       LSM_MODULE_INIT("capability", capability_hooks);
>
> LSM_MODULE(capability);
I thought about this, but I think that the implicit aspect of
statically defining "X_hooks" in the same file is kind of confusing.

>
> etc...
>
> -Kees
>
> --
> Kees Cook
> Pixel Security
--
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

[PATCH v7 0/6] Safe LSM (un)loading, and immutable hooks

[James Morris]

On Tue, 1 May 2018, Micka?l Sala?n wrote:

> > I've been a long-time proponent of programmable LSMs (before it was
> > cool): https://lkml.org/lkml/2016/8/4/58
> 
> Happy to know that Landlock started before programmable access-control
> was cool, too. :)
> https://lkml.kernel.org/r/1458784008-16277-1-git-send-email-mic at digikod.net
> 

Not to brag or anything, but this was c.2000 (AD)  
https://linux.die.net/man/3/libipq ;-)


Landlock is by far the better approach to this than C, and may generally 
solve the use-case of (un)loadable, simple, ad-hoc policies.

-- 
James Morris
<jmorris@namei.org>