* [PATCH v6 0/1] Safe LSM (un)loading, and immutable hooks
@ 2018-04-12 18:41 Sargun Dhillon
2018-04-12 18:42 ` [PATCH v6 1/1] security: Add mechanism to safely (un)load LSMs after boot time Sargun Dhillon
0 siblings, 1 reply; 5+ messages in thread
From: Sargun Dhillon @ 2018-04-12 18:41 UTC (permalink / raw)
To: linux-security-module
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.
Thanks to Tetsuo for their review, and feedback.
Changes since:
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 (1):
security: Add mechanism to safely (un)load LSMs after boot time
include/linux/lsm_hooks.h | 51 +++----
security/Kconfig | 18 ++-
security/apparmor/lsm.c | 6 +-
security/commoncap.c | 7 +-
security/loadpin/loadpin.c | 2 +-
security/security.c | 370 ++++++++++++++++++++++++++++++++++++++++-----
security/selinux/hooks.c | 22 ++-
security/smack/smack_lsm.c | 5 +-
security/tomoyo/tomoyo.c | 5 +-
security/yama/yama_lsm.c | 5 +-
10 files changed, 413 insertions(+), 78 deletions(-)
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v6 1/1] security: Add mechanism to safely (un)load LSMs after boot time
2018-04-12 18:41 [PATCH v6 0/1] Safe LSM (un)loading, and immutable hooks Sargun Dhillon
@ 2018-04-12 18:42 ` Sargun Dhillon
2018-04-13 14:13 ` Tetsuo Handa
0 siblings, 1 reply; 5+ messages in thread
From: Sargun Dhillon @ 2018-04-12 18:42 UTC (permalink / raw)
To: linux-security-module
This patch introduces a mechanism to add mutable hooks and immutable
hooks to the callback chain. It does this by having two different
sets of heads -- traditional (immutable), and optionally (mutable)
security hooks. Immutable security hooks are always marked
__ro_after_init, whereas mutable hooks are allowed to register
whenever they'd like.
If a module tries to load a mutable set of hooks when not supported,
the kernel will respond with -EINVAL. If this happens, the module
should either report the error to the user, crash, or panic, as it
could leave the user in a false sense of security.
It also wraps the hook unloading and execution with an SRCU. One
SRCU is used across all hooks, as the SRCU struct can be memory
intensive, and hook execution time, in general, should be relatively
short.
Deregistration is gated by a kernel parameter
"security.allow_unregister_hooks". It is set to true by default, but
it can be disabled. This prevents, at runtime from hooks being
deregistered. It can be set either at boot time, or at runtime.
It cannot be reset without rebooting the system.
SELinux is unaffected, and can continue to come and go as it pleases.
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Signed-off-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
---
include/linux/lsm_hooks.h | 51 +++----
security/Kconfig | 18 ++-
security/apparmor/lsm.c | 6 +-
security/commoncap.c | 7 +-
security/loadpin/loadpin.c | 2 +-
security/security.c | 370 ++++++++++++++++++++++++++++++++++++++++-----
security/selinux/hooks.c | 22 ++-
security/smack/smack_lsm.c | 5 +-
security/tomoyo/tomoyo.c | 5 +-
security/yama/yama_lsm.c | 5 +-
10 files changed, 413 insertions(+), 78 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index ac491137b10a..699df4c17981 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -28,6 +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
@@ -1963,10 +1964,11 @@ 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 hook_idx;
+ const union security_list_options hook;
+ struct module *owner;
+ char *lsm;
} __randomize_layout;
/*
@@ -1975,17 +1977,25 @@ 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 HOOK_OFFSET(HEAD) offsetof(struct security_hook_heads, HEAD)
+#define HOOK_SIZE(HEAD) FIELD_SIZEOF(struct security_hook_heads, HEAD)
+#define LSM_HOOK_IDX(HEAD) (HOOK_OFFSET(HEAD) / HOOK_SIZE(HEAD))
-extern struct security_hook_heads security_hook_heads;
+#define LSM_HOOK_INIT(HEAD, HOOK) \
+ { \
+ .hook = { .HEAD = HOOK }, \
+ .owner = THIS_MODULE, \
+ .hook_idx = LSM_HOOK_IDX(HEAD), \
+ }
extern char *lsm_names;
-extern void security_add_hooks(struct security_hook_list *hooks, int count,
- char *lsm);
-
-#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+extern int __must_check security_add_hooks(struct security_hook_list *hooks,
+ const int count, char *lsm,
+ const bool is_mutable);
+#ifdef CONFIG_SECURITY_UNREGISTRABLE_HOOKS
/*
+ * Used to facilitate runtime hook 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
@@ -1997,22 +2007,9 @@ extern void security_add_hooks(struct security_hook_list *hooks, int count,
* disabling their module is a good idea needs to be at least as
* careful as the SELinux team.
*/
-static inline void security_delete_hooks(struct security_hook_list *hooks,
- int count)
-{
- int i;
-
- for (i = 0; i < count; i++)
- hlist_del_rcu(&hooks[i].list);
-}
-#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
-
-/* Currently required to handle SELinux runtime hook disable. */
-#ifdef CONFIG_SECURITY_WRITABLE_HOOKS
-#define __lsm_ro_after_init
-#else
-#define __lsm_ro_after_init __ro_after_init
-#endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
+extern int __must_check security_delete_hooks(struct security_hook_list *hooks,
+ int count);
+#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..3dc1e51f97c6 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -33,8 +33,24 @@ config SECURITY
config SECURITY_WRITABLE_HOOKS
depends on SECURITY
- bool
default n
+ bool "Enable mutable security module hooks"
+ help
+ This allows for arbitrary security modules to be loaded by the user
+ after boot time.
+
+ If you are unsure how to answer this question, answer 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 user
+ after boot time. This feature can be disabled via the kernel command
+ line or via sysfs after boot time.
+
+ If you are unsure how to answer this question, answer N.
config SECURITYFS
bool "Enable the securityfs filesystem"
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 77bdfa7f8428..8bb078114a16 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -743,7 +743,7 @@ static int apparmor_task_kill(struct task_struct *target, struct siginfo *info,
return error;
}
-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),
@@ -1161,8 +1161,8 @@ static int __init apparmor_init(void)
aa_free_root_ns();
goto buffers_out;
}
- security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
- "apparmor");
+ BUG_ON(security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
+ "apparmor", false));
/* Report that AppArmor successfully initialized */
apparmor_initialized = 1;
diff --git a/security/commoncap.c b/security/commoncap.c
index 48620c93d697..6fd7fe833ca8 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 = {
+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),
@@ -1362,8 +1362,9 @@ struct security_hook_list capability_hooks[] __lsm_ro_after_init = {
void __init capability_add_hooks(void)
{
- security_add_hooks(capability_hooks, ARRAY_SIZE(capability_hooks),
- "capability");
+ BUG_ON(security_add_hooks(capability_hooks,
+ ARRAY_SIZE(capability_hooks),
+ "capability", false));
}
#endif /* CONFIG_SECURITY */
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index dbe6efde77a0..aaef8d927603 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -174,7 +174,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),
};
diff --git a/security/security.c b/security/security.c
index dd246a38b3f0..2d86ab219ddc 100644
--- a/security/security.c
+++ b/security/security.c
@@ -29,14 +29,35 @@
#include <linux/backing-dev.h>
#include <linux/string.h>
#include <net/flow.h>
+#include <linux/srcu.h>
+#include <linux/mutex.h>
+
+#define SECURITY_HOOK_COUNT \
+ (sizeof(security_hook_heads) / sizeof(struct hlist_head))
#define MAX_LSM_EVM_XATTR 2
/* 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 __ro_after_init;
+
static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
+static DEFINE_MUTEX(security_hook_mutex);
+/*
+ * 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 bool security_allow_unregister_hooks =
+ IS_ENABLED(CONFIG_SECURITY_UNREGISTRABLE_HOOKS);
char *lsm_names;
/* Boot-time LSM user choice */
@@ -53,6 +74,193 @@ static void __init do_security_initcalls(void)
}
}
+#define FOR_EACH_SECURITY_HOOK(ITERATOR, HEAD) \
+ hlist_for_each_entry(ITERATOR, &security_hook_heads.HEAD, list)
+
+#ifdef CONFIG_SECURITY_WRITABLE_HOOKS
+/*
+ * If mutable security hooks are enabled, it exposes a second set of
+ * security_hook_heads. These security_hook_heads will only be executed
+ * if all immutable hooks are executed successfully.
+ */
+static struct security_hook_heads security_hook_heads_mutable;
+#define FOR_EACH_SECURITY_HOOK_MUTABLE(ITERATOR, HEAD) \
+ hlist_for_each_entry(ITERATOR, &security_hook_heads_mutable.HEAD, list)
+
+static struct hlist_head *get_heads(bool is_mutable)
+{
+ if (is_mutable)
+ return (struct hlist_head *)&security_hook_heads_mutable;
+ return (struct hlist_head *)&security_hook_heads;
+}
+#else
+#define FOR_EACH_SECURITY_HOOK_MUTABLE(ITERATOR, HEAD) while (0)
+
+static struct hlist_head *get_heads(bool is_mutable)
+{
+ if (is_mutable)
+ return ERR_PTR(-EINVAL);
+ return (struct hlist_head *)&security_hook_heads;
+}
+#endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
+
+#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);
+}
+
+static inline void synchronize_lsm(void)
+{
+ synchronize_srcu(&security_hook_srcu);
+}
+
+/**
+ * security_delete_hooks - Remove modules hooks from the mutable hooks lists.
+ * @hooks: the hooks to remove
+ * @count: the number of hooks to remove
+ *
+ * 0 is returned on success, otherwise -errno is returned on failure.
+ * If an error is returned, it is up to the LSM author to handle the error
+ * appropriately. If they do not, their code could be unloaded, while
+ * leaving dangling pointers.
+ *
+ * At best, this will cause a kernel oops. At worst case scenario, this can
+ * lead to arbitrary code execution. Therefore, it is critical that module
+ * authors check the return code here, and act appropriately. In most cases
+ * a failure should result in panic.
+ */
+int __must_check security_delete_hooks(struct security_hook_list *hooks,
+ int count)
+{
+ int i, ret = 0;
+
+ mutex_lock(&security_hook_mutex);
+ if (security_allow_unregister_hooks)
+ for (i = 0; i < count; i++)
+ hlist_del_rcu(&hooks[i].list);
+ else
+ ret = -EPERM;
+ mutex_unlock(&security_hook_mutex);
+
+ if (!ret)
+ synchronize_lsm();
+ return ret;
+}
+EXPORT_SYMBOL_GPL(security_delete_hooks);
+
+static void lock_existing_hooks(void)
+{
+ struct hlist_head *list = (struct hlist_head *)
+ &security_hook_heads_mutable;
+ struct security_hook_list *P;
+ int i;
+
+ /*
+ * 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);
+ for (i = 0; i < SECURITY_HOOK_COUNT; i++)
+ hlist_for_each_entry(P, &list[i], list)
+ if (P->owner)
+ WARN_ON(!try_module_get(P->owner));
+ mutex_unlock(&module_mutex);
+}
+#else
+
+static inline int lock_lsm(void)
+{
+ return 0;
+}
+
+static inline void lock_existing_hooks(void) {}
+static inline void unlock_lsm(int idx) {}
+static inline void synchronize_lsm(void) {}
+#endif /* CONFIG_SECURITY_UNREGISTRABLE_HOOKS */
+
+#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+/*
+ * Always succeeds.
+ * Only to be called by legacy code, like SELinux's delete hooks mechanism
+ * as it ignores whether or not allow_unregister_hooks is set.
+ */
+void __security_delete_hooks(struct security_hook_list *hooks, int count)
+{
+ int i;
+
+ mutex_lock(&security_hook_mutex);
+ for (i = 0; i < count; i++)
+ hlist_del_rcu(&hooks[i].list);
+ mutex_unlock(&security_hook_mutex);
+
+ synchronize_lsm();
+}
+#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
+
+static int allow_unregister_hooks_set(const char *val,
+ const struct kernel_param *kp)
+{
+ bool new_val;
+ int ret = 0;
+
+ ret = strtobool(val, &new_val);
+ if (ret)
+ return ret;
+
+ mutex_lock(&security_hook_mutex);
+ /*
+ * This is a noop
+ * false -> false
+ * true -> true
+ */
+ if (new_val == security_allow_unregister_hooks)
+ goto out;
+
+ /* Do not allow for the transition from false -> true */
+ if (new_val) {
+ ret = -EPERM;
+ goto out;
+ }
+
+ /* The only legal transition true -> false */
+ security_allow_unregister_hooks = new_val;
+ /* We now need to "lock" all of the existing hooks */
+ lock_existing_hooks();
+out:
+ mutex_unlock(&security_hook_mutex);
+ return ret;
+}
+
+static int allow_unregister_hooks_get(char *buffer,
+ const struct kernel_param *kp)
+{
+ int ret;
+
+ mutex_lock(&security_hook_mutex);
+ ret = sprintf(buffer, "%c\n",
+ security_allow_unregister_hooks ? '1' : '0');
+ mutex_unlock(&security_hook_mutex);
+
+ 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");
+
/**
* security_init - initializes the security framework
*
@@ -60,12 +268,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");
/*
@@ -153,21 +355,41 @@ int __init security_module_enable(const char *module)
* @hooks: the hooks to add
* @count: the number of hooks to add
* @lsm: the name of the security module
+ * @is_mutable: True if dynamic registration and/or unregistration is needed.
*
* Each LSM has to register its hooks with the infrastructure.
+ * 0 is returned on success, otherwise -errno is returned on failure.
*/
-void __init security_add_hooks(struct security_hook_list *hooks, int count,
- char *lsm)
+int __must_check security_add_hooks(struct security_hook_list *hooks,
+ const int count, char *lsm,
+ const bool is_mutable)
{
- int i;
+ struct hlist_head *heads;
+ int i, hook_idx, ret = 0;
+
+ mutex_lock(&security_hook_mutex);
+ heads = get_heads(is_mutable);
+ if (IS_ERR(heads)) {
+ ret = PTR_ERR(heads);
+ goto out;
+ }
for (i = 0; i < count; i++) {
+ hook_idx = hooks[i].hook_idx;
hooks[i].lsm = lsm;
- hlist_add_tail_rcu(&hooks[i].list, hooks[i].head);
+ hlist_add_tail_rcu(&hooks[i].list, &heads[hook_idx]);
+ if (!security_allow_unregister_hooks && hooks[i].owner)
+ WARN_ON(!try_module_get(hooks->owner));
}
+
if (lsm_append(lsm, &lsm_names) < 0)
- panic("%s - Cannot get early memory.\n", __func__);
+ panic("%s - Cannot get memory.\n", __func__);
+out:
+ mutex_unlock(&security_hook_mutex);
+
+ return ret;
}
+EXPORT_SYMBOL_GPL(security_add_hooks);
int call_lsm_notifier(enum lsm_event event, void *data)
{
@@ -200,25 +422,49 @@ EXPORT_SYMBOL(unregister_lsm_notifier);
#define call_void_hook(FUNC, ...) \
do { \
struct security_hook_list *P; \
+ int srcu_idx; \
\
- hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
+ FOR_EACH_SECURITY_HOOK(P, FUNC) \
+ P->hook.FUNC(__VA_ARGS__); \
+ srcu_idx = lock_lsm(); \
+ FOR_EACH_SECURITY_HOOK_MUTABLE(P, FUNC) \
P->hook.FUNC(__VA_ARGS__); \
+ unlock_lsm(srcu_idx); \
} 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; \
+ int srcu_idx; \
\
- hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
+ FOR_EACH_SECURITY_HOOK(P, FUNC) { \
+ RC = P->hook.FUNC(__VA_ARGS__); \
+ if (RC != 0) \
+ goto LABEL; \
+ } \
+ srcu_idx = lock_lsm(); \
+ FOR_EACH_SECURITY_HOOK_MUTABLE(P, FUNC) { \
RC = P->hook.FUNC(__VA_ARGS__); \
if (RC != 0) \
break; \
} \
+ unlock_lsm(srcu_idx); \
} 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)
@@ -308,8 +554,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 rc, srcu_idx;
/*
* The module will respond with a positive value if
* it thinks the __vm_enough_memory() call should be
@@ -317,13 +562,25 @@ 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(hp, vm_enough_memory) {
+ rc = hp->hook.vm_enough_memory(mm, pages);
+ if (rc <= 0) {
+ cap_sys_admin = 0;
+ goto out;
+ }
+ }
+
+ srcu_idx = lock_lsm();
+ FOR_EACH_SECURITY_HOOK_MUTABLE(hp, vm_enough_memory) {
rc = hp->hook.vm_enough_memory(mm, pages);
if (rc <= 0) {
cap_sys_admin = 0;
break;
}
}
+ unlock_lsm(srcu_idx);
+
+out:
return __vm_enough_memory(mm, pages, cap_sys_admin);
}
@@ -798,40 +1055,64 @@ 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 srcu_idx, 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(hp, inode_getsecurity) {
rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
if (rc != -EOPNOTSUPP)
- return rc;
+ goto out;
}
- return -EOPNOTSUPP;
+
+ srcu_idx = lock_lsm();
+ FOR_EACH_SECURITY_HOOK_MUTABLE(hp, inode_getsecurity) {
+ rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
+ if (rc != -EOPNOTSUPP)
+ break;
+ }
+ unlock_lsm(srcu_idx);
+
+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 srcu_idx, 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(hp, inode_setsecurity) {
rc = hp->hook.inode_setsecurity(inode, name, value, size,
- flags);
+ flags);
if (rc != -EOPNOTSUPP)
- return rc;
+ goto out;
}
- return -EOPNOTSUPP;
+
+ srcu_idx = lock_lsm();
+ FOR_EACH_SECURITY_HOOK_MUTABLE(hp, inode_setsecurity) {
+ rc = hp->hook.inode_setsecurity(inode, name, value, size,
+ flags);
+ if (rc != -EOPNOTSUPP)
+ break;
+ }
+ unlock_lsm(srcu_idx);
+
+out:
+ return rc;
}
+
+
int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size)
{
if (unlikely(IS_PRIVATE(inode)))
@@ -1120,13 +1401,23 @@ int security_task_kill(struct task_struct *p, struct siginfo *info,
}
int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
- unsigned long arg4, unsigned long arg5)
+ unsigned long arg4, unsigned long arg5)
{
- int thisrc;
- int rc = -ENOSYS;
+ int srcu_idx, thisrc, rc = -ENOSYS;
struct security_hook_list *hp;
- hlist_for_each_entry(hp, &security_hook_heads.task_prctl, list) {
+ FOR_EACH_SECURITY_HOOK(hp, task_prctl) {
+ thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
+ if (thisrc != -ENOSYS) {
+ rc = thisrc;
+ if (thisrc != 0)
+ goto out;
+ }
+
+ }
+
+ srcu_idx = lock_lsm();
+ FOR_EACH_SECURITY_HOOK_MUTABLE(hp, task_prctl) {
thisrc = hp->hook.task_prctl(option, arg2, arg3, arg4, arg5);
if (thisrc != -ENOSYS) {
rc = thisrc;
@@ -1134,6 +1425,9 @@ int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
break;
}
}
+ unlock_lsm(srcu_idx);
+
+out:
return rc;
}
@@ -1618,7 +1912,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
@@ -1629,11 +1923,19 @@ 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(hp, xfrm_state_pol_flow_match) {
+ rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
+ goto out;
+ }
+
+ srcu_idx = lock_lsm();
+ FOR_EACH_SECURITY_HOOK_MUTABLE(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 8abd542c6b7c..c405d3ae2c61 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6396,7 +6396,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),
@@ -6629,6 +6635,8 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
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;
@@ -6654,7 +6662,8 @@ static __init int selinux_init(void)
0, SLAB_PANIC, NULL);
avc_init();
- security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
+ BUG_ON(security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks),
+ "selinux", is_mutable));
if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
panic("SELinux: Unable to register AVC netcache callback\n");
@@ -6783,6 +6792,13 @@ static void selinux_nf_ip_exit(void)
#endif /* CONFIG_NETFILTER */
#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+/*
+ * __security_delete_hooks does not respect security.allow_unload_hooks, and
+ * it not meant to be used by new LSMs. Therefore, this is defined here, rather
+ * than in a shared header.
+ */
+extern void __security_delete_hooks(struct security_hook_list *hooks,
+ int count);
static int selinux_disabled;
int selinux_disable(void)
@@ -6802,7 +6818,7 @@ int selinux_disable(void)
selinux_disabled = 1;
selinux_enabled = 0;
- security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
+ __security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
/* Try to destroy the avc node cache */
avc_disable();
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index feada2665322..7ce4395444f7 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4682,7 +4682,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),
@@ -4900,7 +4900,8 @@ static __init int smack_init(void)
/*
* Register with LSM
*/
- security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack");
+ BUG_ON(security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack",
+ false));
return 0;
}
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 213b8c593668..544a614af175 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),
@@ -543,7 +543,8 @@ static int __init tomoyo_init(void)
if (!security_module_enable("tomoyo"))
return 0;
/* register ourselves with the security framework */
- security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo");
+ BUG_ON(security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo",
+ false));
printk(KERN_INFO "TOMOYO Linux initialized\n");
cred->security = &tomoyo_kernel_domain;
tomoyo_mm_init();
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index ffda91a4a1aa..2e4bbb087c49 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),
@@ -480,6 +480,7 @@ 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");
+ BUG_ON(security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama",
+ 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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v6 1/1] security: Add mechanism to safely (un)load LSMs after boot time
2018-04-12 18:42 ` [PATCH v6 1/1] security: Add mechanism to safely (un)load LSMs after boot time Sargun Dhillon
@ 2018-04-13 14:13 ` Tetsuo Handa
2018-04-14 20:14 ` Sargun Dhillon
0 siblings, 1 reply; 5+ messages in thread
From: Tetsuo Handa @ 2018-04-13 14:13 UTC (permalink / raw)
To: linux-security-module
I think we can introduce a struct for passing arguments of
security_add_hooks()/security_delete_hooks(). Then, we don't
need to increment module usage count as many as hooks used.
---
include/linux/lsm_hooks.h | 37 +++++---
scripts/gcc-plugins/randomize_layout_plugin.c | 2 -
security/apparmor/lsm.c | 6 +-
security/commoncap.c | 7 +-
security/loadpin/loadpin.c | 5 +-
security/security.c | 117 ++++++++++++++------------
security/selinux/hooks.c | 10 +--
security/smack/smack_lsm.c | 4 +-
security/tomoyo/tomoyo.c | 5 +-
security/yama/yama_lsm.c | 5 +-
10 files changed, 112 insertions(+), 86 deletions(-)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index c577d3d..4df62dd 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2007,10 +2007,17 @@ struct security_hook_heads {
*/
struct security_hook_list {
struct hlist_node list;
- const unsigned int hook_idx;
+ const unsigned int offset;
const union security_list_options hook;
- struct module *owner;
- char *lsm;
+ const char *lsm; /* Currently unused. */
+} __randomize_layout;
+
+struct lsm_info {
+ struct list_head list;
+ const char *name;
+ struct module *owner;
+ const int count;
+ struct security_hook_list *hooks;
} __randomize_layout;
/*
@@ -2020,20 +2027,27 @@ struct security_hook_list {
* text involved.
*/
#define HOOK_OFFSET(HEAD) offsetof(struct security_hook_heads, HEAD)
-#define HOOK_SIZE(HEAD) FIELD_SIZEOF(struct security_hook_heads, HEAD)
-#define LSM_HOOK_IDX(HEAD) (HOOK_OFFSET(HEAD) / HOOK_SIZE(HEAD))
#define LSM_HOOK_INIT(HEAD, HOOK) \
{ \
.hook = { .HEAD = HOOK }, \
- .owner = THIS_MODULE, \
- .hook_idx = LSM_HOOK_IDX(HEAD), \
+ .offset = HOOK_OFFSET(HEAD), \
}
extern char *lsm_names;
-extern int __must_check security_add_hooks(struct security_hook_list *hooks,
- const int count, char *lsm,
- const bool is_mutable);
+#define LSM_MODULE_INIT(NAME, HOOKS) \
+ { \
+ .name = NAME, \
+ .hooks = HOOKS, \
+ .count = ARRAY_SIZE(HOOKS), \
+ .owner = THIS_MODULE, \
+ }
+
+extern int __must_check security_add_hooks(struct lsm_info *lsm,
+ const bool is_mutable);
+#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+extern void __security_delete_hooks(struct lsm_info *lsm);
+#endif
#ifdef CONFIG_SECURITY_UNREGISTRABLE_HOOKS
/*
* Used to facilitate runtime hook unloading
@@ -2049,8 +2063,7 @@ extern int __must_check security_add_hooks(struct security_hook_list *hooks,
* disabling their module is a good idea needs to be at least as
* careful as the SELinux team.
*/
-extern int __must_check security_delete_hooks(struct security_hook_list *hooks,
- int count);
+extern int __must_check security_delete_hooks(struct lsm_info *lsm);
#endif /* CONFIG_SECURITY_UNREGISTRABLE_HOOKS */
extern int __init security_module_enable(const char *module);
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/apparmor/lsm.c b/security/apparmor/lsm.c
index 2a591bd..12c98aa8 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1191,6 +1191,9 @@ static void apparmor_sock_graft(struct sock *sk, struct socket *parent)
LSM_HOOK_INIT(task_kill, apparmor_task_kill),
};
+static struct lsm_info apparmor_module =
+ LSM_MODULE_INIT("apparmor", apparmor_hooks);
+
/*
* AppArmor sysfs module parameters
*/
@@ -1562,8 +1565,7 @@ static int __init apparmor_init(void)
aa_free_root_ns();
goto buffers_out;
}
- BUG_ON(security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
- "apparmor", false));
+ BUG_ON(security_add_hooks(&apparmor_module, false));
/* Report that AppArmor successfully initialized */
apparmor_initialized = 1;
diff --git a/security/commoncap.c b/security/commoncap.c
index a215059..8c0df17 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1362,11 +1362,12 @@ struct security_hook_list capability_hooks[] __ro_after_init = {
LSM_HOOK_INIT(vm_enough_memory, cap_vm_enough_memory),
};
+static struct lsm_info capability_module =
+ LSM_MODULE_INIT("capability", capability_hooks);
+
void __init capability_add_hooks(void)
{
- BUG_ON(security_add_hooks(capability_hooks,
- ARRAY_SIZE(capability_hooks),
- "capability", false));
+ BUG_ON(security_add_hooks(&capability_module, false));
}
#endif /* CONFIG_SECURITY */
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index 7c84ca8..60f85a5 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -178,10 +178,13 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
};
+static struct lsm_info loadpin_module =
+ 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");
+ BUG_ON(security_add_hooks(&loadpin_module, 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 dd69889..c4e5437 100644
--- a/security/security.c
+++ b/security/security.c
@@ -32,9 +32,6 @@
#include <linux/srcu.h>
#include <linux/mutex.h>
-#define SECURITY_HOOK_COUNT \
- (sizeof(security_hook_heads) / sizeof(struct hlist_head))
-
#include <trace/events/initcall.h>
#define MAX_LSM_EVM_XATTR 2
@@ -43,7 +40,7 @@
#define SECURITY_NAME_MAX 10
static struct security_hook_heads security_hook_heads __ro_after_init;
-
+static LIST_HEAD(registered_lsm_modules);
static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
static DEFINE_MUTEX(security_hook_mutex);
/*
@@ -93,20 +90,20 @@ static void __init do_security_initcalls(void)
#define FOR_EACH_SECURITY_HOOK_MUTABLE(ITERATOR, HEAD) \
hlist_for_each_entry(ITERATOR, &security_hook_heads_mutable.HEAD, list)
-static struct hlist_head *get_heads(bool is_mutable)
+static struct security_hook_heads *get_heads(bool is_mutable)
{
if (is_mutable)
- return (struct hlist_head *)&security_hook_heads_mutable;
- return (struct hlist_head *)&security_hook_heads;
+ return &security_hook_heads_mutable;
+ return &security_hook_heads;
}
#else
#define FOR_EACH_SECURITY_HOOK_MUTABLE(ITERATOR, HEAD) while (0)
-static struct hlist_head *get_heads(bool is_mutable)
+static struct security_hook_heads *get_heads(bool is_mutable)
{
if (is_mutable)
return ERR_PTR(-EINVAL);
- return (struct hlist_head *)&security_hook_heads;
+ return &security_hook_heads;
}
#endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
@@ -129,8 +126,7 @@ static inline void synchronize_lsm(void)
/**
* security_delete_hooks - Remove modules hooks from the mutable hooks lists.
- * @hooks: the hooks to remove
- * @count: the number of hooks to remove
+ * @lsm: Module info,
*
* 0 is returned on success, otherwise -errno is returned on failure.
* If an error is returned, it is up to the LSM author to handle the error
@@ -142,31 +138,29 @@ static inline void synchronize_lsm(void)
* authors check the return code here, and act appropriately. In most cases
* a failure should result in panic.
*/
-int __must_check security_delete_hooks(struct security_hook_list *hooks,
- int count)
+int __must_check security_delete_hooks(struct lsm_info *lsm)
{
- int i, ret = 0;
+ struct security_hook_list *hooks = lsm->hooks;
+ const int count = lsm->count;
+ int i, ret = -EPERM;
- mutex_lock(&security_hook_mutex);
- if (security_allow_unregister_hooks)
+ if (mutex_lock_killable(&security_hook_mutex))
+ return -EINTR;
+ if (security_allow_unregister_hooks) {
for (i = 0; i < count; i++)
hlist_del_rcu(&hooks[i].list);
- else
- ret = -EPERM;
- mutex_unlock(&security_hook_mutex);
-
- if (!ret)
+ list_del(&lsm->list);
synchronize_lsm();
+ ret = 0;
+ }
+ mutex_unlock(&security_hook_mutex);
return ret;
}
EXPORT_SYMBOL_GPL(security_delete_hooks);
static void lock_existing_hooks(void)
{
- struct hlist_head *list = (struct hlist_head *)
- &security_hook_heads_mutable;
- struct security_hook_list *P;
- int i;
+ struct lsm_info *lsm;
/*
* Prevent module unloading while we're doing this
@@ -174,10 +168,8 @@ static void lock_existing_hooks(void)
* is already unloading -- allow that.
*/
mutex_lock(&module_mutex);
- for (i = 0; i < SECURITY_HOOK_COUNT; i++)
- hlist_for_each_entry(P, &list[i], list)
- if (P->owner)
- WARN_ON(!try_module_get(P->owner));
+ list_foreach_entry(lsm, ®istered_lsm_modules, list)
+ try_module_get(lsm->owner);
mutex_unlock(&module_mutex);
}
#else
@@ -198,16 +190,19 @@ static inline void synchronize_lsm(void) {}
* Only to be called by legacy code, like SELinux's delete hooks mechanism
* as it ignores whether or not allow_unregister_hooks is set.
*/
-void __security_delete_hooks(struct security_hook_list *hooks, int count)
+void __security_delete_hooks(struct lsm_info *lsm)
{
int i;
+ struct security_hook_list *hooks = lsm->hooks;
+ const int count = lsm->count;
mutex_lock(&security_hook_mutex);
for (i = 0; i < count; i++)
hlist_del_rcu(&hooks[i].list);
+ list_del(&lsm->list);
+ synchronize_lsm();
mutex_unlock(&security_hook_mutex);
- synchronize_lsm();
}
#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
@@ -314,7 +309,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;
@@ -358,41 +353,53 @@ int __init security_module_enable(const char *module)
/**
* 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: Module info,
* @is_mutable: True if dynamic registration and/or unregistration is needed.
*
* Each LSM has to register its hooks with the infrastructure.
* 0 is returned on success, otherwise -errno is returned on failure.
*/
-int __must_check security_add_hooks(struct security_hook_list *hooks,
- const int count, char *lsm,
- const bool is_mutable)
+int __must_check security_add_hooks(struct lsm_info *lsm,
+ const bool is_mutable)
{
- struct hlist_head *heads;
- int i, hook_idx, ret = 0;
+ struct security_hook_heads *heads = get_heads(is_mutable);
+ struct security_hook_list *hooks = lsm->hooks;
+ const int count = lsm->count;
+ int i, ret;
- mutex_lock(&security_hook_mutex);
- heads = get_heads(is_mutable);
- if (IS_ERR(heads)) {
- ret = PTR_ERR(heads);
+ INIT_LIST_HEAD(&lsm->list);
+ if (IS_ERR(heads))
+ return PTR_ERR(heads);
+ for (i = 0; i < count; i++) {
+ unsigned int offset = hooks[i].offset;
+
+ if (offset % sizeof(struct hlist_head) ||
+ offset + sizeof(struct hlist_head) >
+ sizeof(struct security_hook_heads))
+ return -EINVAL;
+ }
+ if (!security_allow_unregister_hooks &&
+ !try_module_get(lsm->owner))
+ return -EINVAL;
+ if (mutex_lock_killable(&security_hook_mutex)) {
+ module_put(lsm->owner);
+ return -EINTR;
+ }
+ ret = lsm_append(lsm->name, &lsm_names);
+ if (ret) {
+ module_put(lsm->owner);
goto out;
}
-
+ list_add(&lsm->list, ®istered_lsm_modules);
for (i = 0; i < count; i++) {
- hook_idx = hooks[i].hook_idx;
- hooks[i].lsm = lsm;
- hlist_add_tail_rcu(&hooks[i].list, &heads[hook_idx]);
- if (!security_allow_unregister_hooks && hooks[i].owner)
- WARN_ON(!try_module_get(hooks->owner));
- }
+ struct hlist_head *head = (struct hlist_head *)
+ (((char *) heads) + hooks[i].offset);
- if (lsm_append(lsm, &lsm_names) < 0)
- panic("%s - Cannot get memory.\n", __func__);
-out:
+ hooks[i].lsm = lsm->name;
+ hlist_add_tail_rcu(&hooks[i].list, head);
+ }
+ out:
mutex_unlock(&security_hook_mutex);
-
return ret;
}
EXPORT_SYMBOL_GPL(security_add_hooks);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 72466eb..0140e2b 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -7090,6 +7090,9 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
#endif
};
+static struct lsm_info selinux_module =
+ LSM_MODULE_INIT("selinux", selinux_hooks);
+
static __init int selinux_init(void)
{
const bool is_mutable = IS_ENABLED(CONFIG_SECURITY_SELINUX_DISABLE);
@@ -7131,8 +7134,7 @@ static __init int selinux_init(void)
hashtab_cache_init();
- BUG_ON(security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks),
- "selinux", is_mutable));
+ BUG_ON(security_add_hooks(&selinux_module, is_mutable));
if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
panic("SELinux: Unable to register AVC netcache callback\n");
@@ -7266,8 +7268,6 @@ static void selinux_nf_ip_exit(void)
* it not meant to be used by new LSMs. Therefore, this is defined here, rather
* than in a shared header.
*/
-extern void __security_delete_hooks(struct security_hook_list *hooks,
- int count);
int selinux_disable(struct selinux_state *state)
{
if (state->initialized) {
@@ -7286,7 +7286,7 @@ int selinux_disable(struct selinux_state *state)
selinux_enabled = 0;
- __security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
+ __security_delete_hooks(&selinux_module);
/* Try to destroy the avc node cache */
avc_disable();
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index f69b4d9..6b9566e 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -4764,6 +4764,7 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
LSM_HOOK_INIT(dentry_create_files_as, smack_dentry_create_files_as),
};
+static struct lsm_info smack_module = LSM_MODULE_INIT("smack", smack_hooks);
static __init void init_smack_known_list(void)
{
@@ -4842,8 +4843,7 @@ static __init int smack_init(void)
/*
* Register with LSM
*/
- BUG_ON(security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack",
- false));
+ BUG_ON(security_add_hooks(&smack_module, false));
return 0;
}
diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
index 544a614..0917f71 100644
--- a/security/tomoyo/tomoyo.c
+++ b/security/tomoyo/tomoyo.c
@@ -528,6 +528,8 @@ static int tomoyo_socket_sendmsg(struct socket *sock, struct msghdr *msg,
LSM_HOOK_INIT(socket_sendmsg, tomoyo_socket_sendmsg),
};
+static struct lsm_info tomoyo_module = LSM_MODULE_INIT("tomoyo", tomoyo_hooks);
+
/* Lock for GC. */
DEFINE_SRCU(tomoyo_ss);
@@ -543,8 +545,7 @@ static int __init tomoyo_init(void)
if (!security_module_enable("tomoyo"))
return 0;
/* register ourselves with the security framework */
- BUG_ON(security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo",
- false));
+ BUG_ON(security_add_hooks(&tomoyo_module, 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 2e4bbb0..ee520b0 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -430,6 +430,8 @@ int yama_ptrace_traceme(struct task_struct *parent)
LSM_HOOK_INIT(task_free, yama_task_free),
};
+static struct lsm_info yama_module = 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,7 +482,6 @@ static inline void yama_init_sysctl(void) { }
void __init yama_add_hooks(void)
{
pr_info("Yama: becoming mindful.\n");
- BUG_ON(security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama",
- false));
+ BUG_ON(security_add_hooks(&yama_module, false));
yama_init_sysctl();
}
--
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v6 1/1] security: Add mechanism to safely (un)load LSMs after boot time
2018-04-13 14:13 ` Tetsuo Handa
@ 2018-04-14 20:14 ` Sargun Dhillon
2018-04-15 12:03 ` Tetsuo Handa
0 siblings, 1 reply; 5+ messages in thread
From: Sargun Dhillon @ 2018-04-14 20:14 UTC (permalink / raw)
To: linux-security-module
On Fri, Apr 13, 2018 at 7:13 AM, Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
> I think we can introduce a struct for passing arguments of
> security_add_hooks()/security_delete_hooks(). Then, we don't
> need to increment module usage count as many as hooks used.
>
> ---
> include/linux/lsm_hooks.h | 37 +++++---
> scripts/gcc-plugins/randomize_layout_plugin.c | 2 -
> security/apparmor/lsm.c | 6 +-
> security/commoncap.c | 7 +-
> security/loadpin/loadpin.c | 5 +-
> security/security.c | 117 ++++++++++++++------------
> security/selinux/hooks.c | 10 +--
> security/smack/smack_lsm.c | 4 +-
> security/tomoyo/tomoyo.c | 5 +-
> security/yama/yama_lsm.c | 5 +-
> 10 files changed, 112 insertions(+), 86 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index c577d3d..4df62dd 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -2007,10 +2007,17 @@ struct security_hook_heads {
> */
> struct security_hook_list {
> struct hlist_node list;
> - const unsigned int hook_idx;
> + const unsigned int offset;
> const union security_list_options hook;
> - struct module *owner;
> - char *lsm;
> + const char *lsm; /* Currently unused. */
> +} __randomize_layout;
> +
> +struct lsm_info {
> + struct list_head list;
> + const char *name;
> + struct module *owner;
> + const int count;
> + struct security_hook_list *hooks;
> } __randomize_layout;
>
> /*
> @@ -2020,20 +2027,27 @@ struct security_hook_list {
> * text involved.
> */
> #define HOOK_OFFSET(HEAD) offsetof(struct security_hook_heads, HEAD)
> -#define HOOK_SIZE(HEAD) FIELD_SIZEOF(struct security_hook_heads, HEAD)
> -#define LSM_HOOK_IDX(HEAD) (HOOK_OFFSET(HEAD) / HOOK_SIZE(HEAD))
>
> #define LSM_HOOK_INIT(HEAD, HOOK) \
> { \
> .hook = { .HEAD = HOOK }, \
> - .owner = THIS_MODULE, \
> - .hook_idx = LSM_HOOK_IDX(HEAD), \
> + .offset = HOOK_OFFSET(HEAD), \
> }
> extern char *lsm_names;
>
> -extern int __must_check security_add_hooks(struct security_hook_list *hooks,
> - const int count, char *lsm,
> - const bool is_mutable);
> +#define LSM_MODULE_INIT(NAME, HOOKS) \
> + { \
> + .name = NAME, \
> + .hooks = HOOKS, \
> + .count = ARRAY_SIZE(HOOKS), \
> + .owner = THIS_MODULE, \
> + }
> +
> +extern int __must_check security_add_hooks(struct lsm_info *lsm,
> + const bool is_mutable);
> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> +extern void __security_delete_hooks(struct lsm_info *lsm);
> +#endif
Why expose __security_delete_hooks in lsm_hooks.h? Given SELinux is
the only consumer, and shouldn't be using it much longer, I think it
makes sense to let it stay part of the SELinux hooks.c.
> #ifdef CONFIG_SECURITY_UNREGISTRABLE_HOOKS
> /*
> * Used to facilitate runtime hook unloading
> @@ -2049,8 +2063,7 @@ extern int __must_check security_add_hooks(struct security_hook_list *hooks,
> * disabling their module is a good idea needs to be at least as
> * careful as the SELinux team.
> */
> -extern int __must_check security_delete_hooks(struct security_hook_list *hooks,
> - int count);
> +extern int __must_check security_delete_hooks(struct lsm_info *lsm);
> #endif /* CONFIG_SECURITY_UNREGISTRABLE_HOOKS */
>
> extern int __init security_module_enable(const char *module);
> 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/apparmor/lsm.c b/security/apparmor/lsm.c
> index 2a591bd..12c98aa8 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1191,6 +1191,9 @@ static void apparmor_sock_graft(struct sock *sk, struct socket *parent)
> LSM_HOOK_INIT(task_kill, apparmor_task_kill),
> };
>
> +static struct lsm_info apparmor_module =
> + LSM_MODULE_INIT("apparmor", apparmor_hooks);
> +
> /*
> * AppArmor sysfs module parameters
> */
> @@ -1562,8 +1565,7 @@ static int __init apparmor_init(void)
> aa_free_root_ns();
> goto buffers_out;
> }
> - BUG_ON(security_add_hooks(apparmor_hooks, ARRAY_SIZE(apparmor_hooks),
> - "apparmor", false));
> + BUG_ON(security_add_hooks(&apparmor_module, false));
>
> /* Report that AppArmor successfully initialized */
> apparmor_initialized = 1;
> diff --git a/security/commoncap.c b/security/commoncap.c
> index a215059..8c0df17 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -1362,11 +1362,12 @@ struct security_hook_list capability_hooks[] __ro_after_init = {
> LSM_HOOK_INIT(vm_enough_memory, cap_vm_enough_memory),
> };
>
> +static struct lsm_info capability_module =
> + LSM_MODULE_INIT("capability", capability_hooks);
> +
> void __init capability_add_hooks(void)
> {
> - BUG_ON(security_add_hooks(capability_hooks,
> - ARRAY_SIZE(capability_hooks),
> - "capability", false));
> + BUG_ON(security_add_hooks(&capability_module, false));
> }
>
> #endif /* CONFIG_SECURITY */
> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> index 7c84ca8..60f85a5 100644
> --- a/security/loadpin/loadpin.c
> +++ b/security/loadpin/loadpin.c
> @@ -178,10 +178,13 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
> LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
> };
>
> +static struct lsm_info loadpin_module =
> + 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");
> + BUG_ON(security_add_hooks(&loadpin_module, 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 dd69889..c4e5437 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -32,9 +32,6 @@
> #include <linux/srcu.h>
> #include <linux/mutex.h>
>
> -#define SECURITY_HOOK_COUNT \
> - (sizeof(security_hook_heads) / sizeof(struct hlist_head))
> -
> #include <trace/events/initcall.h>
>
> #define MAX_LSM_EVM_XATTR 2
> @@ -43,7 +40,7 @@
> #define SECURITY_NAME_MAX 10
>
> static struct security_hook_heads security_hook_heads __ro_after_init;
> -
> +static LIST_HEAD(registered_lsm_modules);
> static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
> static DEFINE_MUTEX(security_hook_mutex);
> /*
> @@ -93,20 +90,20 @@ static void __init do_security_initcalls(void)
> #define FOR_EACH_SECURITY_HOOK_MUTABLE(ITERATOR, HEAD) \
> hlist_for_each_entry(ITERATOR, &security_hook_heads_mutable.HEAD, list)
>
> -static struct hlist_head *get_heads(bool is_mutable)
> +static struct security_hook_heads *get_heads(bool is_mutable)
> {
> if (is_mutable)
> - return (struct hlist_head *)&security_hook_heads_mutable;
> - return (struct hlist_head *)&security_hook_heads;
> + return &security_hook_heads_mutable;
> + return &security_hook_heads;
> }
> #else
> #define FOR_EACH_SECURITY_HOOK_MUTABLE(ITERATOR, HEAD) while (0)
>
> -static struct hlist_head *get_heads(bool is_mutable)
> +static struct security_hook_heads *get_heads(bool is_mutable)
> {
> if (is_mutable)
> return ERR_PTR(-EINVAL);
> - return (struct hlist_head *)&security_hook_heads;
> + return &security_hook_heads;
> }
> #endif /* CONFIG_SECURITY_WRITABLE_HOOKS */
>
> @@ -129,8 +126,7 @@ static inline void synchronize_lsm(void)
>
> /**
> * security_delete_hooks - Remove modules hooks from the mutable hooks lists.
> - * @hooks: the hooks to remove
> - * @count: the number of hooks to remove
> + * @lsm: Module info,
> *
> * 0 is returned on success, otherwise -errno is returned on failure.
> * If an error is returned, it is up to the LSM author to handle the error
> @@ -142,31 +138,29 @@ static inline void synchronize_lsm(void)
> * authors check the return code here, and act appropriately. In most cases
> * a failure should result in panic.
> */
> -int __must_check security_delete_hooks(struct security_hook_list *hooks,
> - int count)
> +int __must_check security_delete_hooks(struct lsm_info *lsm)
> {
> - int i, ret = 0;
> + struct security_hook_list *hooks = lsm->hooks;
> + const int count = lsm->count;
> + int i, ret = -EPERM;
>
> - mutex_lock(&security_hook_mutex);
> - if (security_allow_unregister_hooks)
> + if (mutex_lock_killable(&security_hook_mutex))
Why mutex_lock_killable?
> + return -EINTR;
> + if (security_allow_unregister_hooks) {
> for (i = 0; i < count; i++)
> hlist_del_rcu(&hooks[i].list);
> - else
> - ret = -EPERM;
> - mutex_unlock(&security_hook_mutex);
> -
> - if (!ret)
> + list_del(&lsm->list);
> synchronize_lsm();
> + ret = 0;
> + }
Why put this in the mutex? It could hold the mutex for a very long
time if a bunch of long(er)-running callbacks are in place.
> + mutex_unlock(&security_hook_mutex);
> return ret;
> }
> EXPORT_SYMBOL_GPL(security_delete_hooks);
>
> static void lock_existing_hooks(void)
> {
> - struct hlist_head *list = (struct hlist_head *)
> - &security_hook_heads_mutable;
> - struct security_hook_list *P;
> - int i;
> + struct lsm_info *lsm;
>
> /*
> * Prevent module unloading while we're doing this
> @@ -174,10 +168,8 @@ static void lock_existing_hooks(void)
> * is already unloading -- allow that.
> */
> mutex_lock(&module_mutex);
> - for (i = 0; i < SECURITY_HOOK_COUNT; i++)
> - hlist_for_each_entry(P, &list[i], list)
> - if (P->owner)
> - WARN_ON(!try_module_get(P->owner));
> + list_foreach_entry(lsm, ®istered_lsm_modules, list)
> + try_module_get(lsm->owner);
> mutex_unlock(&module_mutex);
> }
> #else
> @@ -198,16 +190,19 @@ static inline void synchronize_lsm(void) {}
> * Only to be called by legacy code, like SELinux's delete hooks mechanism
> * as it ignores whether or not allow_unregister_hooks is set.
> */
> -void __security_delete_hooks(struct security_hook_list *hooks, int count)
> +void __security_delete_hooks(struct lsm_info *lsm)
> {
> int i;
> + struct security_hook_list *hooks = lsm->hooks;
> + const int count = lsm->count;
>
> mutex_lock(&security_hook_mutex);
> for (i = 0; i < count; i++)
> hlist_del_rcu(&hooks[i].list);
> + list_del(&lsm->list);
> + synchronize_lsm();
> mutex_unlock(&security_hook_mutex);
>
> - synchronize_lsm();
> }
> #endif /* CONFIG_SECURITY_SELINUX_DISABLE */
>
> @@ -314,7 +309,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;
>
> @@ -358,41 +353,53 @@ int __init security_module_enable(const char *module)
>
> /**
> * 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: Module info,
> * @is_mutable: True if dynamic registration and/or unregistration is needed.
> *
> * Each LSM has to register its hooks with the infrastructure.
> * 0 is returned on success, otherwise -errno is returned on failure.
> */
> -int __must_check security_add_hooks(struct security_hook_list *hooks,
> - const int count, char *lsm,
> - const bool is_mutable)
> +int __must_check security_add_hooks(struct lsm_info *lsm,
> + const bool is_mutable)
> {
> - struct hlist_head *heads;
> - int i, hook_idx, ret = 0;
> + struct security_hook_heads *heads = get_heads(is_mutable);
> + struct security_hook_list *hooks = lsm->hooks;
> + const int count = lsm->count;
> + int i, ret;
>
> - mutex_lock(&security_hook_mutex);
> - heads = get_heads(is_mutable);
> - if (IS_ERR(heads)) {
> - ret = PTR_ERR(heads);
> + INIT_LIST_HEAD(&lsm->list);
Can't this happen in the macro to setup LSM_INFO?
> + if (IS_ERR(heads))
> + return PTR_ERR(heads);
> + for (i = 0; i < count; i++) {
> + unsigned int offset = hooks[i].offset;
> +
> + if (offset % sizeof(struct hlist_head) ||
> + offset + sizeof(struct hlist_head) >
> + sizeof(struct security_hook_heads))
> + return -EINVAL;
> + }
> + if (!security_allow_unregister_hooks &&
> + !try_module_get(lsm->owner))
> + return -EINVAL;
What case is it okay if try_module_get fails? Won't this return
-EINVAL on built-in modules because lsm->owner is NULL?
> + if (mutex_lock_killable(&security_hook_mutex)) {
> + module_put(lsm->owner);
> + return -EINTR;
> + }
> + ret = lsm_append(lsm->name, &lsm_names);
> + if (ret) {
> + module_put(lsm->owner);
> goto out;
> }
> -
> + list_add(&lsm->list, ®istered_lsm_modules);
> for (i = 0; i < count; i++) {
> - hook_idx = hooks[i].hook_idx;
> - hooks[i].lsm = lsm;
> - hlist_add_tail_rcu(&hooks[i].list, &heads[hook_idx]);
> - if (!security_allow_unregister_hooks && hooks[i].owner)
> - WARN_ON(!try_module_get(hooks->owner));
> - }
> + struct hlist_head *head = (struct hlist_head *)
> + (((char *) heads) + hooks[i].offset);
>
> - if (lsm_append(lsm, &lsm_names) < 0)
> - panic("%s - Cannot get memory.\n", __func__);
> -out:
> + hooks[i].lsm = lsm->name;
> + hlist_add_tail_rcu(&hooks[i].list, head);
> + }
> + out:
> mutex_unlock(&security_hook_mutex);
> -
> return ret;
> }
> EXPORT_SYMBOL_GPL(security_add_hooks);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 72466eb..0140e2b 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -7090,6 +7090,9 @@ static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
> #endif
> };
>
> +static struct lsm_info selinux_module =
> + LSM_MODULE_INIT("selinux", selinux_hooks);
> +
> static __init int selinux_init(void)
> {
> const bool is_mutable = IS_ENABLED(CONFIG_SECURITY_SELINUX_DISABLE);
> @@ -7131,8 +7134,7 @@ static __init int selinux_init(void)
>
> hashtab_cache_init();
>
> - BUG_ON(security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks),
> - "selinux", is_mutable));
> + BUG_ON(security_add_hooks(&selinux_module, is_mutable));
>
> if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
> panic("SELinux: Unable to register AVC netcache callback\n");
> @@ -7266,8 +7268,6 @@ static void selinux_nf_ip_exit(void)
> * it not meant to be used by new LSMs. Therefore, this is defined here, rather
> * than in a shared header.
> */
> -extern void __security_delete_hooks(struct security_hook_list *hooks,
> - int count);
> int selinux_disable(struct selinux_state *state)
> {
> if (state->initialized) {
> @@ -7286,7 +7286,7 @@ int selinux_disable(struct selinux_state *state)
>
> selinux_enabled = 0;
>
> - __security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
> + __security_delete_hooks(&selinux_module);
>
> /* Try to destroy the avc node cache */
> avc_disable();
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index f69b4d9..6b9566e 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -4764,6 +4764,7 @@ static int smack_dentry_create_files_as(struct dentry *dentry, int mode,
> LSM_HOOK_INIT(dentry_create_files_as, smack_dentry_create_files_as),
> };
>
> +static struct lsm_info smack_module = LSM_MODULE_INIT("smack", smack_hooks);
>
> static __init void init_smack_known_list(void)
> {
> @@ -4842,8 +4843,7 @@ static __init int smack_init(void)
> /*
> * Register with LSM
> */
> - BUG_ON(security_add_hooks(smack_hooks, ARRAY_SIZE(smack_hooks), "smack",
> - false));
> + BUG_ON(security_add_hooks(&smack_module, false));
>
> return 0;
> }
> diff --git a/security/tomoyo/tomoyo.c b/security/tomoyo/tomoyo.c
> index 544a614..0917f71 100644
> --- a/security/tomoyo/tomoyo.c
> +++ b/security/tomoyo/tomoyo.c
> @@ -528,6 +528,8 @@ static int tomoyo_socket_sendmsg(struct socket *sock, struct msghdr *msg,
> LSM_HOOK_INIT(socket_sendmsg, tomoyo_socket_sendmsg),
> };
>
> +static struct lsm_info tomoyo_module = LSM_MODULE_INIT("tomoyo", tomoyo_hooks);
> +
> /* Lock for GC. */
> DEFINE_SRCU(tomoyo_ss);
>
> @@ -543,8 +545,7 @@ static int __init tomoyo_init(void)
> if (!security_module_enable("tomoyo"))
> return 0;
> /* register ourselves with the security framework */
> - BUG_ON(security_add_hooks(tomoyo_hooks, ARRAY_SIZE(tomoyo_hooks), "tomoyo",
> - false));
> + BUG_ON(security_add_hooks(&tomoyo_module, 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 2e4bbb0..ee520b0 100644
> --- a/security/yama/yama_lsm.c
> +++ b/security/yama/yama_lsm.c
> @@ -430,6 +430,8 @@ int yama_ptrace_traceme(struct task_struct *parent)
> LSM_HOOK_INIT(task_free, yama_task_free),
> };
>
> +static struct lsm_info yama_module = 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,7 +482,6 @@ static inline void yama_init_sysctl(void) { }
> void __init yama_add_hooks(void)
> {
> pr_info("Yama: becoming mindful.\n");
> - BUG_ON(security_add_hooks(yama_hooks, ARRAY_SIZE(yama_hooks), "yama",
> - false));
> + BUG_ON(security_add_hooks(&yama_module, false));
> yama_init_sysctl();
> }
> --
> 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
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v6 1/1] security: Add mechanism to safely (un)load LSMs after boot time
2018-04-14 20:14 ` Sargun Dhillon
@ 2018-04-15 12:03 ` Tetsuo Handa
0 siblings, 0 replies; 5+ messages in thread
From: Tetsuo Handa @ 2018-04-15 12:03 UTC (permalink / raw)
To: linux-security-module
Sargun Dhillon wrote:
> On Fri, Apr 13, 2018 at 7:13 AM, Tetsuo Handa
> <penguin-kernel@i-love.sakura.ne.jp> wrote:
> > +#define LSM_MODULE_INIT(NAME, HOOKS) \
> > + { \
> > + .name = NAME, \
> > + .hooks = HOOKS, \
> > + .count = ARRAY_SIZE(HOOKS), \
> > + .owner = THIS_MODULE, \
> > + }
> > +
> > +extern int __must_check security_add_hooks(struct lsm_info *lsm,
> > + const bool is_mutable);
> > +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> > +extern void __security_delete_hooks(struct lsm_info *lsm);
> > +#endif
> Why expose __security_delete_hooks in lsm_hooks.h? Given SELinux is
> the only consumer, and shouldn't be using it much longer, I think it
> makes sense to let it stay part of the SELinux hooks.c.
Because scripts/checkpatch.pl says so. In fact, I was puzzled by strange
NULL pointer dereference bug at list_del() in __security_delete_hooks(), for
the compiler did not find for me that I forgot to update the argument of
__security_delete_hooks() in security/selinux/hooks.c .
> > +int __must_check security_add_hooks(struct lsm_info *lsm,
> > + const bool is_mutable)
> > {
> > - struct hlist_head *heads;
> > - int i, hook_idx, ret = 0;
> > + struct security_hook_heads *heads = get_heads(is_mutable);
> > + struct security_hook_list *hooks = lsm->hooks;
> > + const int count = lsm->count;
> > + int i, ret;
> >
> > - mutex_lock(&security_hook_mutex);
> > - heads = get_heads(is_mutable);
> > - if (IS_ERR(heads)) {
> > - ret = PTR_ERR(heads);
> > + INIT_LIST_HEAD(&lsm->list);
> Can't this happen in the macro to setup LSM_INFO?
Yes if the variable's name is passed into the LSM_MODULE_INIT() macro, for
LIST_HEAD_INIT() needs the variable's address.
> > + if (IS_ERR(heads))
> > + return PTR_ERR(heads);
> > + for (i = 0; i < count; i++) {
> > + unsigned int offset = hooks[i].offset;
> > +
> > + if (offset % sizeof(struct hlist_head) ||
> > + offset + sizeof(struct hlist_head) >
> > + sizeof(struct security_hook_heads))
> > + return -EINVAL;
> > + }
> > + if (!security_allow_unregister_hooks &&
> > + !try_module_get(lsm->owner))
> > + return -EINVAL;
> What case is it okay if try_module_get fails? Won't this return
> -EINVAL on built-in modules because lsm->owner is NULL?
try_module_get(NULL) == true.
> > +int __must_check security_delete_hooks(struct lsm_info *lsm)
> > {
> > - int i, ret = 0;
> > + struct security_hook_list *hooks = lsm->hooks;
> > + const int count = lsm->count;
> > + int i, ret = -EPERM;
> >
> > - mutex_lock(&security_hook_mutex);
> > - if (security_allow_unregister_hooks)
> > + if (mutex_lock_killable(&security_hook_mutex))
> Why mutex_lock_killable?
If a function can fail safely, it is preferable to allow it be interrupted
when waiting for something.
By the way, how do you guarantee that security_delete_hooks() was called before
delete_module() request of that lsm module starts? Well, rereading your patch:
/**
* security_delete_hooks - Remove modules hooks from the mutable hooks lists.
* @hooks: the hooks to remove
* @count: the number of hooks to remove
*
* 0 is returned on success, otherwise -errno is returned on failure.
* If an error is returned, it is up to the LSM author to handle the error
* appropriately. If they do not, their code could be unloaded, while
* leaving dangling pointers.
*
* At best, this will cause a kernel oops. At worst case scenario, this can
* lead to arbitrary code execution. Therefore, it is critical that module
* authors check the return code here, and act appropriately. In most cases
* a failure should result in panic.
*/
Your patch tries to prevent security_delete_hooks() from module_exit() function by
holding module_mutex when try_module_get() is called, doesn't it? And you leave
handling of CONFIG_MODULE_FORCE_UNLOAD=y kernels to the module authors when forced
unload is used, don't you? But what the authors can do when security_delete_hooks()
returned an error? They can do nothing other than calling panic(). I think that
security_delete_hooks() is effectively not allowed to fail...
By the way, did you test with CONFIG_PROVE_LOCKING=y? lock_existing_hooks() has
security_hook_mutex -> module_mutex dependency while init_module()/delete_module()
have module_mutex -> security_hook_mutex dependency?
--
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-04-15 12:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-12 18:41 [PATCH v6 0/1] Safe LSM (un)loading, and immutable hooks Sargun Dhillon
2018-04-12 18:42 ` [PATCH v6 1/1] security: Add mechanism to safely (un)load LSMs after boot time Sargun Dhillon
2018-04-13 14:13 ` Tetsuo Handa
2018-04-14 20:14 ` Sargun Dhillon
2018-04-15 12:03 ` Tetsuo Handa
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.