[RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[James Morris]

Subsequent patches will add RO hardening to LSM hooks, however, SELinux
still needs to be able to perform runtime disablement after init to handle
architectures where init-time disablement via boot parameters is not feasible.

Introduce a new kernel configuration parameter CONFIG_SECURITY_WRITABLE_HOOKS,
and a helper macro __lsm_ro_after_init, to handle this case.

Signed-off-by: James Morris <james.l.morris@oracle.com>
---
 include/linux/lsm_hooks.h |    7 +++++++
 security/Kconfig          |    5 +++++
 security/selinux/Kconfig  |    6 ++++++
 3 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index e29d4c6..c4b149f 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1908,6 +1908,13 @@ 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
+#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/Kconfig b/security/Kconfig
index 118f454..f6f90c4 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -31,6 +31,11 @@ config SECURITY
 
 	  If you are unsure how to answer this question, answer N.
 
+config SECURITY_WRITABLE_HOOKS
+	depends on SECURITY
+	bool
+	default n
+
 config SECURITYFS
 	bool "Enable the securityfs filesystem"
 	help
diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
index ea7e3ef..8af7a69 100644
--- a/security/selinux/Kconfig
+++ b/security/selinux/Kconfig
@@ -40,6 +40,7 @@ config SECURITY_SELINUX_BOOTPARAM_VALUE
 config SECURITY_SELINUX_DISABLE
 	bool "NSA SELinux runtime disable"
 	depends on SECURITY_SELINUX
+	select SECURITY_WRITABLE_HOOKS
 	default n
 	help
 	  This option enables writing to a selinuxfs node 'disable', which
@@ -50,6 +51,11 @@ config SECURITY_SELINUX_DISABLE
 	  portability across platforms where boot parameters are difficult
 	  to employ.
 
+	  NOTE: selecting this option will disable the '__ro_after_init'
+	  kernel hardening feature for security hooks.   Please consider
+	  using the selinux=0 boot parameter instead of enabling this
+	  option.
+
 	  If you are unsure how to answer this question, answer N.
 
 config SECURITY_SELINUX_DEVELOP
-- 
1.7.1

[kernel-hardening] [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[James Morris]

Subsequent patches will add RO hardening to LSM hooks, however, SELinux
still needs to be able to perform runtime disablement after init to handle
architectures where init-time disablement via boot parameters is not feasible.

Introduce a new kernel configuration parameter CONFIG_SECURITY_WRITABLE_HOOKS,
and a helper macro __lsm_ro_after_init, to handle this case.

Signed-off-by: James Morris <james.l.morris@oracle.com>
---
 include/linux/lsm_hooks.h |    7 +++++++
 security/Kconfig          |    5 +++++
 security/selinux/Kconfig  |    6 ++++++
 3 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index e29d4c6..c4b149f 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1908,6 +1908,13 @@ 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
+#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/Kconfig b/security/Kconfig
index 118f454..f6f90c4 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -31,6 +31,11 @@ config SECURITY
 
 	  If you are unsure how to answer this question, answer N.
 
+config SECURITY_WRITABLE_HOOKS
+	depends on SECURITY
+	bool
+	default n
+
 config SECURITYFS
 	bool "Enable the securityfs filesystem"
 	help
diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
index ea7e3ef..8af7a69 100644
--- a/security/selinux/Kconfig
+++ b/security/selinux/Kconfig
@@ -40,6 +40,7 @@ config SECURITY_SELINUX_BOOTPARAM_VALUE
 config SECURITY_SELINUX_DISABLE
 	bool "NSA SELinux runtime disable"
 	depends on SECURITY_SELINUX
+	select SECURITY_WRITABLE_HOOKS
 	default n
 	help
 	  This option enables writing to a selinuxfs node 'disable', which
@@ -50,6 +51,11 @@ config SECURITY_SELINUX_DISABLE
 	  portability across platforms where boot parameters are difficult
 	  to employ.
 
+	  NOTE: selecting this option will disable the '__ro_after_init'
+	  kernel hardening feature for security hooks.   Please consider
+	  using the selinux=0 boot parameter instead of enabling this
+	  option.
+
 	  If you are unsure how to answer this question, answer N.
 
 config SECURITY_SELINUX_DEVELOP
-- 
1.7.1

Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Tetsuo Handa]

James Morris wrote:
> > Loadable kernel modules used by antivirus software temporarily modify syscall tables
> > ( http://stackoverflow.com/questions/13876369/system-call-interception-in-linux-kernel-module-kernel-3-5 )
> > in order to register hooks for execve()/open()/close(). It is very frustrating for
> > many users if you enforce CONFIG_MODULES=n or forbid post-__init registration of hooks.
> 
> We don't cater to out of tree code.
> 
> Additionally, I'd also seriously question whether the security benefits of 
> kernel AV outweigh its security risks.

Are you aware that loadable kernel modules used by antivirus software are used for
relaying events to userspace daemons? Such modules don't do scanning inside kernel.
Why such modules are considered as security risks?

Are you aware that SELinux (or AppArmor or whatever so-called MAC) is not always
enabled in all enterprise systems? Are you aware that there are systems where
administrators cannot afford using rule based access restriction mechanisms?
In such systems, the security benefits of loadable security modules (provided by
AV or alike) can outweigh read only LSM hooks.

> diff --git a/security/Kconfig b/security/Kconfig
> index 118f454..f6f90c4 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -31,6 +31,11 @@ config SECURITY
>  
>  	  If you are unsure how to answer this question, answer N.
>  
> +config SECURITY_WRITABLE_HOOKS
> +	depends on SECURITY
> +	bool
> +	default n
> +

This configuration option must not be set to N without big fat explanation
about implications of setting this option to N.

Honestly, I still don't like this option, regardless of whether SELinux
needs this option or not.

[kernel-hardening] Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Tetsuo Handa]

James Morris wrote:
> > Loadable kernel modules used by antivirus software temporarily modify syscall tables
> > ( http://stackoverflow.com/questions/13876369/system-call-interception-in-linux-kernel-module-kernel-3-5 )
> > in order to register hooks for execve()/open()/close(). It is very frustrating for
> > many users if you enforce CONFIG_MODULES=n or forbid post-__init registration of hooks.
> 
> We don't cater to out of tree code.
> 
> Additionally, I'd also seriously question whether the security benefits of 
> kernel AV outweigh its security risks.

Are you aware that loadable kernel modules used by antivirus software are used for
relaying events to userspace daemons? Such modules don't do scanning inside kernel.
Why such modules are considered as security risks?

Are you aware that SELinux (or AppArmor or whatever so-called MAC) is not always
enabled in all enterprise systems? Are you aware that there are systems where
administrators cannot afford using rule based access restriction mechanisms?
In such systems, the security benefits of loadable security modules (provided by
AV or alike) can outweigh read only LSM hooks.

> diff --git a/security/Kconfig b/security/Kconfig
> index 118f454..f6f90c4 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -31,6 +31,11 @@ config SECURITY
>  
>  	  If you are unsure how to answer this question, answer N.
>  
> +config SECURITY_WRITABLE_HOOKS
> +	depends on SECURITY
> +	bool
> +	default n
> +

This configuration option must not be set to N without big fat explanation
about implications of setting this option to N.

Honestly, I still don't like this option, regardless of whether SELinux
needs this option or not.

Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[James Morris]

On Tue, 14 Feb 2017, Tetsuo Handa wrote:

> > diff --git a/security/Kconfig b/security/Kconfig
> > index 118f454..f6f90c4 100644
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -31,6 +31,11 @@ config SECURITY
> >  
> >  	  If you are unsure how to answer this question, answer N.
> >  
> > +config SECURITY_WRITABLE_HOOKS
> > +	depends on SECURITY
> > +	bool
> > +	default n
> > +
> 
> This configuration option must not be set to N without big fat explanation
> about implications of setting this option to N.

It's not visible in the config menu, it's only there to support SELinux 
runtime disablement, otherwise it wouldn't even be an option.

> 
> Honestly, I still don't like this option, regardless of whether SELinux
> needs this option or not.
> 

I agree, it would be better to just enable RO hardening without an option 
to disable it.

-- 
James Morris
<jmorris@namei.org>

[kernel-hardening] Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[James Morris]

On Tue, 14 Feb 2017, Tetsuo Handa wrote:

> > diff --git a/security/Kconfig b/security/Kconfig
> > index 118f454..f6f90c4 100644
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -31,6 +31,11 @@ config SECURITY
> >  
> >  	  If you are unsure how to answer this question, answer N.
> >  
> > +config SECURITY_WRITABLE_HOOKS
> > +	depends on SECURITY
> > +	bool
> > +	default n
> > +
> 
> This configuration option must not be set to N without big fat explanation
> about implications of setting this option to N.

It's not visible in the config menu, it's only there to support SELinux 
runtime disablement, otherwise it wouldn't even be an option.

> 
> Honestly, I still don't like this option, regardless of whether SELinux
> needs this option or not.
> 

I agree, it would be better to just enable RO hardening without an option 
to disable it.

-- 
James Morris
<jmorris@namei.org>

Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Tetsuo Handa]

James Morris wrote:
> On Tue, 14 Feb 2017, Tetsuo Handa wrote:
> 
> > > diff --git a/security/Kconfig b/security/Kconfig
> > > index 118f454..f6f90c4 100644
> > > --- a/security/Kconfig
> > > +++ b/security/Kconfig
> > > @@ -31,6 +31,11 @@ config SECURITY
> > >  
> > >  	  If you are unsure how to answer this question, answer N.
> > >  
> > > +config SECURITY_WRITABLE_HOOKS
> > > +	depends on SECURITY
> > > +	bool
> > > +	default n
> > > +
> > 
> > This configuration option must not be set to N without big fat explanation
> > about implications of setting this option to N.
> 
> It's not visible in the config menu, it's only there to support SELinux 
> runtime disablement, otherwise it wouldn't even be an option.
> 
> > 
> > Honestly, I still don't like this option, regardless of whether SELinux
> > needs this option or not.
> > 
> 
> I agree, it would be better to just enable RO hardening without an option 
> to disable it.

Here is my version. printk() is only for testing purpose and will be removed
in the final version.

  (1) Use set_memory_ro() instead of __ro_after_init so that runtime disablement
      of SELinux and runtime enablement of dynamically loadable LSMs can use
      set_memory_rw().

  (2) Replace "struct security_hook_list"->head with "struct security_hook_list"->offset
      so that "struct security_hook_heads security_hook_heads;" can become a local variable
      in security/security.c .

  (3) (Not in this patch.) The "struct security_hook_list" variable in each LSM
      module is considered as "__init" and "const" because the content is copied to
      dynamically allocated (and protected via set_memory_ro()) memory pages.

  (4) (Not in this patch.) If we add EXPORT_SYMBOL_GPL(security_add_hooks), dynamically
      loadable LSMs are legally allowed.

 include/linux/lsm_hooks.h |   31 +++++++---------
 security/security.c       |   89 ++++++++++++++++++++++++++++++++++++++++++----
 security/selinux/hooks.c  |   12 +++++-
 3 files changed, 107 insertions(+), 25 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index e29d4c6..00050a7 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1865,9 +1865,16 @@ struct security_hook_heads {
  */
 struct security_hook_list {
 	struct list_head		list;
-	struct list_head		*head;
 	union security_list_options	hook;
-	char				*lsm;
+	const char			*lsm;
+	unsigned int			offset;
+};
+
+struct lsm_entry {
+	struct list_head head;
+	unsigned int order;
+	unsigned int count;
+	struct security_hook_list hooks[0];
 };
 
 /*
@@ -1877,14 +1884,13 @@ struct security_hook_list {
  * text involved.
  */
 #define LSM_HOOK_INIT(HEAD, HOOK) \
-	{ .head = &security_hook_heads.HEAD, .hook = { .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 struct lsm_entry *security_add_hooks(const 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
@@ -1898,15 +1904,8 @@ 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++)
-		list_del_rcu(&hooks[i].list);
-}
-#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
+extern void security_delete_hooks(struct lsm_entry *entry);
+#endif
 
 extern int __init security_module_enable(const char *module);
 extern void __init capability_add_hooks(void);
diff --git a/security/security.c b/security/security.c
index d0e07f2..dffe69b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -32,6 +32,7 @@
 /* Maximum number of letters for an LSM name string */
 #define SECURITY_NAME_MAX	10
 
+static bool lsm_initialized;
 char *lsm_names;
 /* Boot-time LSM user choice */
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
@@ -47,6 +48,38 @@ static void __init do_security_initcalls(void)
 	}
 }
 
+static struct security_hook_heads security_hook_heads;
+static LIST_HEAD(lsm_entry_list);
+#ifdef CONFIG_DEBUG_RODATA
+static inline void mark_security_hooks_ro(void)
+{
+	struct lsm_entry *tmp;
+
+	list_for_each_entry(tmp, &lsm_entry_list, head) {
+		printk("Marking LSM hooks at %p read only.\n", tmp);
+		set_memory_ro((unsigned long) tmp, 1 << tmp->order);
+	}
+}
+
+static inline void mark_security_hooks_rw(void)
+{
+	struct lsm_entry *tmp;
+
+	list_for_each_entry(tmp, &lsm_entry_list, head) {
+		printk("Marking LSM hooks at %p read write.\n", tmp);
+		set_memory_rw((unsigned long) tmp, 1 << tmp->order);
+	}
+}
+#else
+static inline void mark_security_hooks_ro(void)
+{
+}
+
+static inline void mark_security_hooks_rw(void)
+{
+}
+#endif
+
 /**
  * security_init - initializes the security framework
  *
@@ -68,6 +101,8 @@ int __init security_init(void)
 	 */
 	do_security_initcalls();
 
+	lsm_initialized = true;
+	mark_security_hooks_ro();
 	return 0;
 }
 
@@ -79,7 +114,7 @@ static int __init choose_lsm(char *str)
 }
 __setup("security=", choose_lsm);
 
-static int lsm_append(char *new, char **result)
+static int lsm_append(const char *new, char **result)
 {
 	char *cp;
 
@@ -122,18 +157,58 @@ 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)
+struct lsm_entry *security_add_hooks(const struct security_hook_list *hooks,
+				     int count, const char *lsm)
 {
 	int i;
+	/*
+	 * entry has to be an PAGE_ALIGNED multiple of PAGE_SIZE memory
+	 * in order to pass to set_memory_ro()/set_memory_rw().
+	 */
+	const size_t size = sizeof(*hooks) * count + sizeof(struct lsm_entry);
+	const unsigned int order = get_order(size);
+	struct lsm_entry *entry = kmalloc_order(size, GFP_KERNEL | __GFP_ZERO,
+						order);
 
+	if (!entry || !PAGE_ALIGNED(entry) || lsm_append(lsm, &lsm_names) < 0) {
+		kfree(entry);
+		goto out;
+	}
+	if (lsm_initialized)
+		mark_security_hooks_rw();
+	printk("Allocating LSM hooks for %s at %p\n", lsm, entry);
+	entry->order = order;
+	entry->count = count;
+	memcpy(&entry->hooks[0], hooks, size - sizeof(struct lsm_entry));
 	for (i = 0; i < count; i++) {
-		hooks[i].lsm = lsm;
-		list_add_tail_rcu(&hooks[i].list, hooks[i].head);
+		entry->hooks[i].lsm = lsm;
+		list_add_tail_rcu(&entry->hooks[i].list, (struct list_head *)
+				  (((char *) &security_hook_heads)
+				   + entry->hooks[i].offset));
 	}
-	if (lsm_append(lsm, &lsm_names) < 0)
+	list_add_tail(&entry->head, &lsm_entry_list);
+	if (lsm_initialized)
+		mark_security_hooks_ro();
+	return entry;
+ out:
+	if (!lsm_initialized)
 		panic("%s - Cannot get early memory.\n", __func__);
+	return NULL;
+}
+
+#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+void security_delete_hooks(struct lsm_entry *entry)
+{
+	int i;
+
+	mark_security_hooks_rw();
+	list_del_rcu(&entry->head);
+	for (i = 0; i < entry->count; i++)
+		list_del_rcu(&entry->hooks[i].list);
+	/* Not calling kfree() in order to avoid races. */
+	mark_security_hooks_ro();
 }
+#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
 
 /*
  * Hook list operation macros.
@@ -1622,7 +1697,7 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule,
 }
 #endif /* CONFIG_AUDIT */
 
-struct security_hook_heads security_hook_heads = {
+static struct security_hook_heads security_hook_heads = {
 	.binder_set_context_mgr =
 		LIST_HEAD_INIT(security_hook_heads.binder_set_context_mgr),
 	.binder_transaction =
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9bc12bc..4668590 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6319,6 +6319,10 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
 #endif
 };
 
+#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+static struct lsm_entry *selinux_entry;
+#endif
+
 static __init int selinux_init(void)
 {
 	if (!security_module_enable("selinux")) {
@@ -6346,7 +6350,11 @@ static __init int selinux_init(void)
 					    0, SLAB_PANIC, NULL);
 	avc_init();
 
-	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
+#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+	selinux_entry =
+#endif
+		security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks),
+				   "selinux");
 
 	if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
 		panic("SELinux: Unable to register AVC netcache callback\n");
@@ -6475,7 +6483,7 @@ int selinux_disable(void)
 	selinux_disabled = 1;
 	selinux_enabled = 0;
 
-	security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
+	security_delete_hooks(selinux_entry);
 
 	/* Try to destroy the avc node cache */
 	avc_disable();

[kernel-hardening] Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Tetsuo Handa]

James Morris wrote:
> On Tue, 14 Feb 2017, Tetsuo Handa wrote:
> 
> > > diff --git a/security/Kconfig b/security/Kconfig
> > > index 118f454..f6f90c4 100644
> > > --- a/security/Kconfig
> > > +++ b/security/Kconfig
> > > @@ -31,6 +31,11 @@ config SECURITY
> > >  
> > >  	  If you are unsure how to answer this question, answer N.
> > >  
> > > +config SECURITY_WRITABLE_HOOKS
> > > +	depends on SECURITY
> > > +	bool
> > > +	default n
> > > +
> > 
> > This configuration option must not be set to N without big fat explanation
> > about implications of setting this option to N.
> 
> It's not visible in the config menu, it's only there to support SELinux 
> runtime disablement, otherwise it wouldn't even be an option.
> 
> > 
> > Honestly, I still don't like this option, regardless of whether SELinux
> > needs this option or not.
> > 
> 
> I agree, it would be better to just enable RO hardening without an option 
> to disable it.

Here is my version. printk() is only for testing purpose and will be removed
in the final version.

  (1) Use set_memory_ro() instead of __ro_after_init so that runtime disablement
      of SELinux and runtime enablement of dynamically loadable LSMs can use
      set_memory_rw().

  (2) Replace "struct security_hook_list"->head with "struct security_hook_list"->offset
      so that "struct security_hook_heads security_hook_heads;" can become a local variable
      in security/security.c .

  (3) (Not in this patch.) The "struct security_hook_list" variable in each LSM
      module is considered as "__init" and "const" because the content is copied to
      dynamically allocated (and protected via set_memory_ro()) memory pages.

  (4) (Not in this patch.) If we add EXPORT_SYMBOL_GPL(security_add_hooks), dynamically
      loadable LSMs are legally allowed.

 include/linux/lsm_hooks.h |   31 +++++++---------
 security/security.c       |   89 ++++++++++++++++++++++++++++++++++++++++++----
 security/selinux/hooks.c  |   12 +++++-
 3 files changed, 107 insertions(+), 25 deletions(-)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index e29d4c6..00050a7 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1865,9 +1865,16 @@ struct security_hook_heads {
  */
 struct security_hook_list {
 	struct list_head		list;
-	struct list_head		*head;
 	union security_list_options	hook;
-	char				*lsm;
+	const char			*lsm;
+	unsigned int			offset;
+};
+
+struct lsm_entry {
+	struct list_head head;
+	unsigned int order;
+	unsigned int count;
+	struct security_hook_list hooks[0];
 };
 
 /*
@@ -1877,14 +1884,13 @@ struct security_hook_list {
  * text involved.
  */
 #define LSM_HOOK_INIT(HEAD, HOOK) \
-	{ .head = &security_hook_heads.HEAD, .hook = { .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 struct lsm_entry *security_add_hooks(const 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
@@ -1898,15 +1904,8 @@ 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++)
-		list_del_rcu(&hooks[i].list);
-}
-#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
+extern void security_delete_hooks(struct lsm_entry *entry);
+#endif
 
 extern int __init security_module_enable(const char *module);
 extern void __init capability_add_hooks(void);
diff --git a/security/security.c b/security/security.c
index d0e07f2..dffe69b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -32,6 +32,7 @@
 /* Maximum number of letters for an LSM name string */
 #define SECURITY_NAME_MAX	10
 
+static bool lsm_initialized;
 char *lsm_names;
 /* Boot-time LSM user choice */
 static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
@@ -47,6 +48,38 @@ static void __init do_security_initcalls(void)
 	}
 }
 
+static struct security_hook_heads security_hook_heads;
+static LIST_HEAD(lsm_entry_list);
+#ifdef CONFIG_DEBUG_RODATA
+static inline void mark_security_hooks_ro(void)
+{
+	struct lsm_entry *tmp;
+
+	list_for_each_entry(tmp, &lsm_entry_list, head) {
+		printk("Marking LSM hooks at %p read only.\n", tmp);
+		set_memory_ro((unsigned long) tmp, 1 << tmp->order);
+	}
+}
+
+static inline void mark_security_hooks_rw(void)
+{
+	struct lsm_entry *tmp;
+
+	list_for_each_entry(tmp, &lsm_entry_list, head) {
+		printk("Marking LSM hooks at %p read write.\n", tmp);
+		set_memory_rw((unsigned long) tmp, 1 << tmp->order);
+	}
+}
+#else
+static inline void mark_security_hooks_ro(void)
+{
+}
+
+static inline void mark_security_hooks_rw(void)
+{
+}
+#endif
+
 /**
  * security_init - initializes the security framework
  *
@@ -68,6 +101,8 @@ int __init security_init(void)
 	 */
 	do_security_initcalls();
 
+	lsm_initialized = true;
+	mark_security_hooks_ro();
 	return 0;
 }
 
@@ -79,7 +114,7 @@ static int __init choose_lsm(char *str)
 }
 __setup("security=", choose_lsm);
 
-static int lsm_append(char *new, char **result)
+static int lsm_append(const char *new, char **result)
 {
 	char *cp;
 
@@ -122,18 +157,58 @@ 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)
+struct lsm_entry *security_add_hooks(const struct security_hook_list *hooks,
+				     int count, const char *lsm)
 {
 	int i;
+	/*
+	 * entry has to be an PAGE_ALIGNED multiple of PAGE_SIZE memory
+	 * in order to pass to set_memory_ro()/set_memory_rw().
+	 */
+	const size_t size = sizeof(*hooks) * count + sizeof(struct lsm_entry);
+	const unsigned int order = get_order(size);
+	struct lsm_entry *entry = kmalloc_order(size, GFP_KERNEL | __GFP_ZERO,
+						order);
 
+	if (!entry || !PAGE_ALIGNED(entry) || lsm_append(lsm, &lsm_names) < 0) {
+		kfree(entry);
+		goto out;
+	}
+	if (lsm_initialized)
+		mark_security_hooks_rw();
+	printk("Allocating LSM hooks for %s at %p\n", lsm, entry);
+	entry->order = order;
+	entry->count = count;
+	memcpy(&entry->hooks[0], hooks, size - sizeof(struct lsm_entry));
 	for (i = 0; i < count; i++) {
-		hooks[i].lsm = lsm;
-		list_add_tail_rcu(&hooks[i].list, hooks[i].head);
+		entry->hooks[i].lsm = lsm;
+		list_add_tail_rcu(&entry->hooks[i].list, (struct list_head *)
+				  (((char *) &security_hook_heads)
+				   + entry->hooks[i].offset));
 	}
-	if (lsm_append(lsm, &lsm_names) < 0)
+	list_add_tail(&entry->head, &lsm_entry_list);
+	if (lsm_initialized)
+		mark_security_hooks_ro();
+	return entry;
+ out:
+	if (!lsm_initialized)
 		panic("%s - Cannot get early memory.\n", __func__);
+	return NULL;
+}
+
+#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+void security_delete_hooks(struct lsm_entry *entry)
+{
+	int i;
+
+	mark_security_hooks_rw();
+	list_del_rcu(&entry->head);
+	for (i = 0; i < entry->count; i++)
+		list_del_rcu(&entry->hooks[i].list);
+	/* Not calling kfree() in order to avoid races. */
+	mark_security_hooks_ro();
 }
+#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
 
 /*
  * Hook list operation macros.
@@ -1622,7 +1697,7 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule,
 }
 #endif /* CONFIG_AUDIT */
 
-struct security_hook_heads security_hook_heads = {
+static struct security_hook_heads security_hook_heads = {
 	.binder_set_context_mgr =
 		LIST_HEAD_INIT(security_hook_heads.binder_set_context_mgr),
 	.binder_transaction =
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 9bc12bc..4668590 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6319,6 +6319,10 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
 #endif
 };
 
+#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+static struct lsm_entry *selinux_entry;
+#endif
+
 static __init int selinux_init(void)
 {
 	if (!security_module_enable("selinux")) {
@@ -6346,7 +6350,11 @@ static __init int selinux_init(void)
 					    0, SLAB_PANIC, NULL);
 	avc_init();
 
-	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
+#ifdef CONFIG_SECURITY_SELINUX_DISABLE
+	selinux_entry =
+#endif
+		security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks),
+				   "selinux");
 
 	if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
 		panic("SELinux: Unable to register AVC netcache callback\n");
@@ -6475,7 +6483,7 @@ int selinux_disable(void)
 	selinux_disabled = 1;
 	selinux_enabled = 0;
 
-	security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
+	security_delete_hooks(selinux_entry);
 
 	/* Try to destroy the avc node cache */
 	avc_disable();

Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Casey Schaufler]

On 2/15/2017 6:42 AM, Tetsuo Handa wrote:
> James Morris wrote:
>> On Tue, 14 Feb 2017, Tetsuo Handa wrote:
>>
>>>> diff --git a/security/Kconfig b/security/Kconfig
>>>> index 118f454..f6f90c4 100644
>>>> --- a/security/Kconfig
>>>> +++ b/security/Kconfig
>>>> @@ -31,6 +31,11 @@ config SECURITY
>>>>  
>>>>  	  If you are unsure how to answer this question, answer N.
>>>>  
>>>> +config SECURITY_WRITABLE_HOOKS
>>>> +	depends on SECURITY
>>>> +	bool
>>>> +	default n
>>>> +
>>> This configuration option must not be set to N without big fat explanation
>>> about implications of setting this option to N.
>> It's not visible in the config menu, it's only there to support SELinux 
>> runtime disablement, otherwise it wouldn't even be an option.
>>
>>> Honestly, I still don't like this option, regardless of whether SELinux
>>> needs this option or not.
>>>
>> I agree, it would be better to just enable RO hardening without an option 
>> to disable it.
> Here is my version. printk() is only for testing purpose and will be removed
> in the final version.
>
>   (1) Use set_memory_ro() instead of __ro_after_init so that runtime disablement
>       of SELinux and runtime enablement of dynamically loadable LSMs can use
>       set_memory_rw().
>
>   (2) Replace "struct security_hook_list"->head with "struct security_hook_list"->offset
>       so that "struct security_hook_heads security_hook_heads;" can become a local variable
>       in security/security.c .
>
>   (3) (Not in this patch.) The "struct security_hook_list" variable in each LSM
>       module is considered as "__init" and "const" because the content is copied to
>       dynamically allocated (and protected via set_memory_ro()) memory pages.
>
>   (4) (Not in this patch.) If we add EXPORT_SYMBOL_GPL(security_add_hooks), dynamically
>       loadable LSMs are legally allowed.
>
>  include/linux/lsm_hooks.h |   31 +++++++---------
>  security/security.c       |   89 ++++++++++++++++++++++++++++++++++++++++++----
>  security/selinux/hooks.c  |   12 +++++-
>  3 files changed, 107 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index e29d4c6..00050a7 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1865,9 +1865,16 @@ struct security_hook_heads {
>   */
>  struct security_hook_list {
>  	struct list_head		list;
> -	struct list_head		*head;
>  	union security_list_options	hook;
> -	char				*lsm;
> +	const char			*lsm;
> +	unsigned int			offset;
> +};
> +
> +struct lsm_entry {
> +	struct list_head head;
> +	unsigned int order;
> +	unsigned int count;
> +	struct security_hook_list hooks[0];
>  };

I can't say that I'm buying the value of the additional
complexity here. Sure, you're protecting part of the data
all the time, but you're exposing part all the time, too.
For now I think that the solution James proposes makes the
most sense. In the hypothetical loadable modules case I
don't see real value in hardening bits of the data while
explicitly making the most important parts dynamic.

>  
>  /*
> @@ -1877,14 +1884,13 @@ struct security_hook_list {
>   * text involved.
>   */
>  #define LSM_HOOK_INIT(HEAD, HOOK) \
> -	{ .head = &security_hook_heads.HEAD, .hook = { .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 struct lsm_entry *security_add_hooks(const 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
> @@ -1898,15 +1904,8 @@ 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++)
> -		list_del_rcu(&hooks[i].list);
> -}
> -#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
> +extern void security_delete_hooks(struct lsm_entry *entry);
> +#endif
>  
>  extern int __init security_module_enable(const char *module);
>  extern void __init capability_add_hooks(void);
> diff --git a/security/security.c b/security/security.c
> index d0e07f2..dffe69b 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -32,6 +32,7 @@
>  /* Maximum number of letters for an LSM name string */
>  #define SECURITY_NAME_MAX	10
>  
> +static bool lsm_initialized;
>  char *lsm_names;
>  /* Boot-time LSM user choice */
>  static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
> @@ -47,6 +48,38 @@ static void __init do_security_initcalls(void)
>  	}
>  }
>  
> +static struct security_hook_heads security_hook_heads;
> +static LIST_HEAD(lsm_entry_list);
> +#ifdef CONFIG_DEBUG_RODATA
> +static inline void mark_security_hooks_ro(void)
> +{
> +	struct lsm_entry *tmp;
> +
> +	list_for_each_entry(tmp, &lsm_entry_list, head) {
> +		printk("Marking LSM hooks at %p read only.\n", tmp);
> +		set_memory_ro((unsigned long) tmp, 1 << tmp->order);
> +	}
> +}
> +
> +static inline void mark_security_hooks_rw(void)
> +{
> +	struct lsm_entry *tmp;
> +
> +	list_for_each_entry(tmp, &lsm_entry_list, head) {
> +		printk("Marking LSM hooks at %p read write.\n", tmp);
> +		set_memory_rw((unsigned long) tmp, 1 << tmp->order);
> +	}
> +}
> +#else
> +static inline void mark_security_hooks_ro(void)
> +{
> +}
> +
> +static inline void mark_security_hooks_rw(void)
> +{
> +}
> +#endif
> +
>  /**
>   * security_init - initializes the security framework
>   *
> @@ -68,6 +101,8 @@ int __init security_init(void)
>  	 */
>  	do_security_initcalls();
>  
> +	lsm_initialized = true;
> +	mark_security_hooks_ro();
>  	return 0;
>  }
>  
> @@ -79,7 +114,7 @@ static int __init choose_lsm(char *str)
>  }
>  __setup("security=", choose_lsm);
>  
> -static int lsm_append(char *new, char **result)
> +static int lsm_append(const char *new, char **result)
>  {
>  	char *cp;
>  
> @@ -122,18 +157,58 @@ 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)
> +struct lsm_entry *security_add_hooks(const struct security_hook_list *hooks,
> +				     int count, const char *lsm)
>  {
>  	int i;
> +	/*
> +	 * entry has to be an PAGE_ALIGNED multiple of PAGE_SIZE memory
> +	 * in order to pass to set_memory_ro()/set_memory_rw().
> +	 */
> +	const size_t size = sizeof(*hooks) * count + sizeof(struct lsm_entry);
> +	const unsigned int order = get_order(size);
> +	struct lsm_entry *entry = kmalloc_order(size, GFP_KERNEL | __GFP_ZERO,
> +						order);
>  
> +	if (!entry || !PAGE_ALIGNED(entry) || lsm_append(lsm, &lsm_names) < 0) {
> +		kfree(entry);
> +		goto out;
> +	}
> +	if (lsm_initialized)
> +		mark_security_hooks_rw();
> +	printk("Allocating LSM hooks for %s at %p\n", lsm, entry);
> +	entry->order = order;
> +	entry->count = count;
> +	memcpy(&entry->hooks[0], hooks, size - sizeof(struct lsm_entry));
>  	for (i = 0; i < count; i++) {
> -		hooks[i].lsm = lsm;
> -		list_add_tail_rcu(&hooks[i].list, hooks[i].head);
> +		entry->hooks[i].lsm = lsm;
> +		list_add_tail_rcu(&entry->hooks[i].list, (struct list_head *)
> +				  (((char *) &security_hook_heads)
> +				   + entry->hooks[i].offset));
>  	}
> -	if (lsm_append(lsm, &lsm_names) < 0)
> +	list_add_tail(&entry->head, &lsm_entry_list);
> +	if (lsm_initialized)
> +		mark_security_hooks_ro();
> +	return entry;
> + out:
> +	if (!lsm_initialized)
>  		panic("%s - Cannot get early memory.\n", __func__);
> +	return NULL;
> +}
> +
> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> +void security_delete_hooks(struct lsm_entry *entry)
> +{
> +	int i;
> +
> +	mark_security_hooks_rw();
> +	list_del_rcu(&entry->head);
> +	for (i = 0; i < entry->count; i++)
> +		list_del_rcu(&entry->hooks[i].list);
> +	/* Not calling kfree() in order to avoid races. */
> +	mark_security_hooks_ro();
>  }
> +#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
>  
>  /*
>   * Hook list operation macros.
> @@ -1622,7 +1697,7 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule,
>  }
>  #endif /* CONFIG_AUDIT */
>  
> -struct security_hook_heads security_hook_heads = {
> +static struct security_hook_heads security_hook_heads = {
>  	.binder_set_context_mgr =
>  		LIST_HEAD_INIT(security_hook_heads.binder_set_context_mgr),
>  	.binder_transaction =
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9bc12bc..4668590 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6319,6 +6319,10 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
>  #endif
>  };
>  
> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> +static struct lsm_entry *selinux_entry;
> +#endif
> +
>  static __init int selinux_init(void)
>  {
>  	if (!security_module_enable("selinux")) {
> @@ -6346,7 +6350,11 @@ static __init int selinux_init(void)
>  					    0, SLAB_PANIC, NULL);
>  	avc_init();
>  
> -	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> +	selinux_entry =
> +#endif
> +		security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks),
> +				   "selinux");
>  
>  	if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
>  		panic("SELinux: Unable to register AVC netcache callback\n");
> @@ -6475,7 +6483,7 @@ int selinux_disable(void)
>  	selinux_disabled = 1;
>  	selinux_enabled = 0;
>  
> -	security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
> +	security_delete_hooks(selinux_entry);
>  
>  	/* Try to destroy the avc node cache */
>  	avc_disable();
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>

[kernel-hardening] Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Casey Schaufler]

On 2/15/2017 6:42 AM, Tetsuo Handa wrote:
> James Morris wrote:
>> On Tue, 14 Feb 2017, Tetsuo Handa wrote:
>>
>>>> diff --git a/security/Kconfig b/security/Kconfig
>>>> index 118f454..f6f90c4 100644
>>>> --- a/security/Kconfig
>>>> +++ b/security/Kconfig
>>>> @@ -31,6 +31,11 @@ config SECURITY
>>>>  
>>>>  	  If you are unsure how to answer this question, answer N.
>>>>  
>>>> +config SECURITY_WRITABLE_HOOKS
>>>> +	depends on SECURITY
>>>> +	bool
>>>> +	default n
>>>> +
>>> This configuration option must not be set to N without big fat explanation
>>> about implications of setting this option to N.
>> It's not visible in the config menu, it's only there to support SELinux 
>> runtime disablement, otherwise it wouldn't even be an option.
>>
>>> Honestly, I still don't like this option, regardless of whether SELinux
>>> needs this option or not.
>>>
>> I agree, it would be better to just enable RO hardening without an option 
>> to disable it.
> Here is my version. printk() is only for testing purpose and will be removed
> in the final version.
>
>   (1) Use set_memory_ro() instead of __ro_after_init so that runtime disablement
>       of SELinux and runtime enablement of dynamically loadable LSMs can use
>       set_memory_rw().
>
>   (2) Replace "struct security_hook_list"->head with "struct security_hook_list"->offset
>       so that "struct security_hook_heads security_hook_heads;" can become a local variable
>       in security/security.c .
>
>   (3) (Not in this patch.) The "struct security_hook_list" variable in each LSM
>       module is considered as "__init" and "const" because the content is copied to
>       dynamically allocated (and protected via set_memory_ro()) memory pages.
>
>   (4) (Not in this patch.) If we add EXPORT_SYMBOL_GPL(security_add_hooks), dynamically
>       loadable LSMs are legally allowed.
>
>  include/linux/lsm_hooks.h |   31 +++++++---------
>  security/security.c       |   89 ++++++++++++++++++++++++++++++++++++++++++----
>  security/selinux/hooks.c  |   12 +++++-
>  3 files changed, 107 insertions(+), 25 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index e29d4c6..00050a7 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1865,9 +1865,16 @@ struct security_hook_heads {
>   */
>  struct security_hook_list {
>  	struct list_head		list;
> -	struct list_head		*head;
>  	union security_list_options	hook;
> -	char				*lsm;
> +	const char			*lsm;
> +	unsigned int			offset;
> +};
> +
> +struct lsm_entry {
> +	struct list_head head;
> +	unsigned int order;
> +	unsigned int count;
> +	struct security_hook_list hooks[0];
>  };

I can't say that I'm buying the value of the additional
complexity here. Sure, you're protecting part of the data
all the time, but you're exposing part all the time, too.
For now I think that the solution James proposes makes the
most sense. In the hypothetical loadable modules case I
don't see real value in hardening bits of the data while
explicitly making the most important parts dynamic.

>  
>  /*
> @@ -1877,14 +1884,13 @@ struct security_hook_list {
>   * text involved.
>   */
>  #define LSM_HOOK_INIT(HEAD, HOOK) \
> -	{ .head = &security_hook_heads.HEAD, .hook = { .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 struct lsm_entry *security_add_hooks(const 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
> @@ -1898,15 +1904,8 @@ 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++)
> -		list_del_rcu(&hooks[i].list);
> -}
> -#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
> +extern void security_delete_hooks(struct lsm_entry *entry);
> +#endif
>  
>  extern int __init security_module_enable(const char *module);
>  extern void __init capability_add_hooks(void);
> diff --git a/security/security.c b/security/security.c
> index d0e07f2..dffe69b 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -32,6 +32,7 @@
>  /* Maximum number of letters for an LSM name string */
>  #define SECURITY_NAME_MAX	10
>  
> +static bool lsm_initialized;
>  char *lsm_names;
>  /* Boot-time LSM user choice */
>  static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] =
> @@ -47,6 +48,38 @@ static void __init do_security_initcalls(void)
>  	}
>  }
>  
> +static struct security_hook_heads security_hook_heads;
> +static LIST_HEAD(lsm_entry_list);
> +#ifdef CONFIG_DEBUG_RODATA
> +static inline void mark_security_hooks_ro(void)
> +{
> +	struct lsm_entry *tmp;
> +
> +	list_for_each_entry(tmp, &lsm_entry_list, head) {
> +		printk("Marking LSM hooks at %p read only.\n", tmp);
> +		set_memory_ro((unsigned long) tmp, 1 << tmp->order);
> +	}
> +}
> +
> +static inline void mark_security_hooks_rw(void)
> +{
> +	struct lsm_entry *tmp;
> +
> +	list_for_each_entry(tmp, &lsm_entry_list, head) {
> +		printk("Marking LSM hooks at %p read write.\n", tmp);
> +		set_memory_rw((unsigned long) tmp, 1 << tmp->order);
> +	}
> +}
> +#else
> +static inline void mark_security_hooks_ro(void)
> +{
> +}
> +
> +static inline void mark_security_hooks_rw(void)
> +{
> +}
> +#endif
> +
>  /**
>   * security_init - initializes the security framework
>   *
> @@ -68,6 +101,8 @@ int __init security_init(void)
>  	 */
>  	do_security_initcalls();
>  
> +	lsm_initialized = true;
> +	mark_security_hooks_ro();
>  	return 0;
>  }
>  
> @@ -79,7 +114,7 @@ static int __init choose_lsm(char *str)
>  }
>  __setup("security=", choose_lsm);
>  
> -static int lsm_append(char *new, char **result)
> +static int lsm_append(const char *new, char **result)
>  {
>  	char *cp;
>  
> @@ -122,18 +157,58 @@ 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)
> +struct lsm_entry *security_add_hooks(const struct security_hook_list *hooks,
> +				     int count, const char *lsm)
>  {
>  	int i;
> +	/*
> +	 * entry has to be an PAGE_ALIGNED multiple of PAGE_SIZE memory
> +	 * in order to pass to set_memory_ro()/set_memory_rw().
> +	 */
> +	const size_t size = sizeof(*hooks) * count + sizeof(struct lsm_entry);
> +	const unsigned int order = get_order(size);
> +	struct lsm_entry *entry = kmalloc_order(size, GFP_KERNEL | __GFP_ZERO,
> +						order);
>  
> +	if (!entry || !PAGE_ALIGNED(entry) || lsm_append(lsm, &lsm_names) < 0) {
> +		kfree(entry);
> +		goto out;
> +	}
> +	if (lsm_initialized)
> +		mark_security_hooks_rw();
> +	printk("Allocating LSM hooks for %s at %p\n", lsm, entry);
> +	entry->order = order;
> +	entry->count = count;
> +	memcpy(&entry->hooks[0], hooks, size - sizeof(struct lsm_entry));
>  	for (i = 0; i < count; i++) {
> -		hooks[i].lsm = lsm;
> -		list_add_tail_rcu(&hooks[i].list, hooks[i].head);
> +		entry->hooks[i].lsm = lsm;
> +		list_add_tail_rcu(&entry->hooks[i].list, (struct list_head *)
> +				  (((char *) &security_hook_heads)
> +				   + entry->hooks[i].offset));
>  	}
> -	if (lsm_append(lsm, &lsm_names) < 0)
> +	list_add_tail(&entry->head, &lsm_entry_list);
> +	if (lsm_initialized)
> +		mark_security_hooks_ro();
> +	return entry;
> + out:
> +	if (!lsm_initialized)
>  		panic("%s - Cannot get early memory.\n", __func__);
> +	return NULL;
> +}
> +
> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> +void security_delete_hooks(struct lsm_entry *entry)
> +{
> +	int i;
> +
> +	mark_security_hooks_rw();
> +	list_del_rcu(&entry->head);
> +	for (i = 0; i < entry->count; i++)
> +		list_del_rcu(&entry->hooks[i].list);
> +	/* Not calling kfree() in order to avoid races. */
> +	mark_security_hooks_ro();
>  }
> +#endif /* CONFIG_SECURITY_SELINUX_DISABLE */
>  
>  /*
>   * Hook list operation macros.
> @@ -1622,7 +1697,7 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule,
>  }
>  #endif /* CONFIG_AUDIT */
>  
> -struct security_hook_heads security_hook_heads = {
> +static struct security_hook_heads security_hook_heads = {
>  	.binder_set_context_mgr =
>  		LIST_HEAD_INIT(security_hook_heads.binder_set_context_mgr),
>  	.binder_transaction =
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 9bc12bc..4668590 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6319,6 +6319,10 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
>  #endif
>  };
>  
> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> +static struct lsm_entry *selinux_entry;
> +#endif
> +
>  static __init int selinux_init(void)
>  {
>  	if (!security_module_enable("selinux")) {
> @@ -6346,7 +6350,11 @@ static __init int selinux_init(void)
>  					    0, SLAB_PANIC, NULL);
>  	avc_init();
>  
> -	security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks), "selinux");
> +#ifdef CONFIG_SECURITY_SELINUX_DISABLE
> +	selinux_entry =
> +#endif
> +		security_add_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks),
> +				   "selinux");
>  
>  	if (avc_add_callback(selinux_netcache_avc_callback, AVC_CALLBACK_RESET))
>  		panic("SELinux: Unable to register AVC netcache callback\n");
> @@ -6475,7 +6483,7 @@ int selinux_disable(void)
>  	selinux_disabled = 1;
>  	selinux_enabled = 0;
>  
> -	security_delete_hooks(selinux_hooks, ARRAY_SIZE(selinux_hooks));
> +	security_delete_hooks(selinux_entry);
>  
>  	/* Try to destroy the avc node cache */
>  	avc_disable();
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>

Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Tetsuo Handa]

Casey Schaufler wrote:
> I can't say that I'm buying the value of the additional
> complexity here. Sure, you're protecting part of the data
> all the time, but you're exposing part all the time, too.

Will you explain it in detail? As far as I know, __ro_after_init
annotation makes the kernel to call set_memory_ro() after __init
functions are completed, by gathering such variables into a section
so that set_memory_ro() can change page flags for a memory region
where it is PAGE_ALIGNED and the size is multiple of PAGE_SIZE bytes.

This "struct lsm_entry" is for gathering only LSM related variables which
would have been marked as __ro_after_init if CONFIG_SECURITY_WRITABLE_HOOKS=n
into a list of memory regions where they are PAGE_ALIGNED and the size is
multiple of PAGE_SIZE bytes, so that the kernel can call set_memory_ro() on
these memory regions even if CONFIG_SECURITY_SELINUX_DISABLE=y or allowing
LKM based LSMs.

> For now I think that the solution James proposes makes the
> most sense. In the hypothetical loadable modules case I
> don't see real value in hardening bits of the data while
> explicitly making the most important parts dynamic.

Best solution for environments where it is known to enforce only one specific
LSM module and no user choices and no LKM based LSMs (including antivirus or
alike) is to directly embed that LSM module into security/security.c .

CONFIG_SECURITY_WRITABLE_HOOKS=n depends on CONFIG_SECURITY_SELINUX_DISABLE=n
but many of distributor's kernels enable multiple LSM modules including
CONFIG_SECURITY_SELINUX_DISABLE=y. Also, people are using antivirus software
on distributor's kernels. At least one antivirus software (which allows
anonymous download of LKM source code) is using LSM hooks since Linux 2.6.32
instead of rewriting syscall tables. We are already allowing multiple concurrent
LSM modules (up to one fully armored module which uses "struct cred"->security
field or exclusive hooks like security_xfrm_state_pol_flow_match(), plus
unlimited number of lightweight modules which do not use "struct cred"->security
nor exclusive hooks) as long as they are built into the kernel. There is no
reason to keep LKM based LSM modules from antivirus software or alike away.
For such kernels, this "struct lsm_entry" allows calling set_memory_ro() on LSM
related variables which would have been marked as __ro_after_init if
CONFIG_SECURITY_WRITABLE_HOOKS=n.

If you are not happy with using kmalloc() for copying "struct security_hook_list"
in each LSM module, can we agree with padding "struct security_hook_list" in each
LSM module PAGE_ALIGNED and the size being multiple of PAGE_SIZE bytes?

[kernel-hardening] Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Tetsuo Handa]

Casey Schaufler wrote:
> I can't say that I'm buying the value of the additional
> complexity here. Sure, you're protecting part of the data
> all the time, but you're exposing part all the time, too.

Will you explain it in detail? As far as I know, __ro_after_init
annotation makes the kernel to call set_memory_ro() after __init
functions are completed, by gathering such variables into a section
so that set_memory_ro() can change page flags for a memory region
where it is PAGE_ALIGNED and the size is multiple of PAGE_SIZE bytes.

This "struct lsm_entry" is for gathering only LSM related variables which
would have been marked as __ro_after_init if CONFIG_SECURITY_WRITABLE_HOOKS=n
into a list of memory regions where they are PAGE_ALIGNED and the size is
multiple of PAGE_SIZE bytes, so that the kernel can call set_memory_ro() on
these memory regions even if CONFIG_SECURITY_SELINUX_DISABLE=y or allowing
LKM based LSMs.

> For now I think that the solution James proposes makes the
> most sense. In the hypothetical loadable modules case I
> don't see real value in hardening bits of the data while
> explicitly making the most important parts dynamic.

Best solution for environments where it is known to enforce only one specific
LSM module and no user choices and no LKM based LSMs (including antivirus or
alike) is to directly embed that LSM module into security/security.c .

CONFIG_SECURITY_WRITABLE_HOOKS=n depends on CONFIG_SECURITY_SELINUX_DISABLE=n
but many of distributor's kernels enable multiple LSM modules including
CONFIG_SECURITY_SELINUX_DISABLE=y. Also, people are using antivirus software
on distributor's kernels. At least one antivirus software (which allows
anonymous download of LKM source code) is using LSM hooks since Linux 2.6.32
instead of rewriting syscall tables. We are already allowing multiple concurrent
LSM modules (up to one fully armored module which uses "struct cred"->security
field or exclusive hooks like security_xfrm_state_pol_flow_match(), plus
unlimited number of lightweight modules which do not use "struct cred"->security
nor exclusive hooks) as long as they are built into the kernel. There is no
reason to keep LKM based LSM modules from antivirus software or alike away.
For such kernels, this "struct lsm_entry" allows calling set_memory_ro() on LSM
related variables which would have been marked as __ro_after_init if
CONFIG_SECURITY_WRITABLE_HOOKS=n.

If you are not happy with using kmalloc() for copying "struct security_hook_list"
in each LSM module, can we agree with padding "struct security_hook_list" in each
LSM module PAGE_ALIGNED and the size being multiple of PAGE_SIZE bytes?

Re: [kernel-hardening] Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Casey Schaufler]

On 2/16/2017 3:00 AM, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>> I can't say that I'm buying the value of the additional
>> complexity here. Sure, you're protecting part of the data
>> all the time, but you're exposing part all the time, too.
> Will you explain it in detail? As far as I know, __ro_after_init
> annotation makes the kernel to call set_memory_ro() after __init
> functions are completed, by gathering such variables into a section
> so that set_memory_ro() can change page flags for a memory region
> where it is PAGE_ALIGNED and the size is multiple of PAGE_SIZE bytes.
>
> This "struct lsm_entry" is for gathering only LSM related variables which
> would have been marked as __ro_after_init if CONFIG_SECURITY_WRITABLE_HOOKS=n
> into a list of memory regions where they are PAGE_ALIGNED and the size is
> multiple of PAGE_SIZE bytes, so that the kernel can call set_memory_ro() on
> these memory regions even if CONFIG_SECURITY_SELINUX_DISABLE=y or allowing
> LKM based LSMs.

All I'm saying is that it's simpler to mark the entire
structure than it is to mark parts.

>> For now I think that the solution James proposes makes the
>> most sense. In the hypothetical loadable modules case I
>> don't see real value in hardening bits of the data while
>> explicitly making the most important parts dynamic.
> Best solution for environments where it is known to enforce only one specific
> LSM module and no user choices and no LKM based LSMs (including antivirus or
> alike) is to directly embed that LSM module into security/security.c .

There are too few beers on the table in front of me
to start that discussion. :)

> CONFIG_SECURITY_WRITABLE_HOOKS=n depends on CONFIG_SECURITY_SELINUX_DISABLE=n
> but many of distributor's kernels enable multiple LSM modules including
> CONFIG_SECURITY_SELINUX_DISABLE=y. Also, people are using antivirus software
> on distributor's kernels.

I have to limit my comment on that to pointing out
that we can't make decisions based on code that has
never been proposed for the mainline.

>  At least one antivirus software (which allows
> anonymous download of LKM source code) is using LSM hooks since Linux 2.6.32
> instead of rewriting syscall tables. We are already allowing multiple concurrent
> LSM modules (up to one fully armored module which uses "struct cred"->security
> field or exclusive hooks like security_xfrm_state_pol_flow_match(), plus
> unlimited number of lightweight modules which do not use "struct cred"->security
> nor exclusive hooks) as long as they are built into the kernel. There is no
> reason to keep LKM based LSM modules from antivirus software or alike away.

We're not to the point where in-kernel modules are stacking fully.
Not everyone is on board for that, but hope springs eternal. Part
of the design criteria I'm working under is that it shouldn't
preclude loadable modules, and I still think that's doable. The
patch James proposed is completely compatible with this philosophy.
You can argue that it requires a loadable module configuration be
less "hardened", but the opponents of loadable modules say that is
inherent to loadable modules.

> For such kernels, this "struct lsm_entry" allows calling set_memory_ro() on LSM
> related variables which would have been marked as __ro_after_init if
> CONFIG_SECURITY_WRITABLE_HOOKS=n.
>
> If you are not happy with using kmalloc() for copying "struct security_hook_list"
> in each LSM module, can we agree with padding "struct security_hook_list" in each
> LSM module PAGE_ALIGNED and the size being multiple of PAGE_SIZE bytes?

I don't understand what that would help without
the rest of your changes.

Re: [kernel-hardening] Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Daniel Micay]

[-- Attachment #1: Type: text/plain, Size: 1489 bytes --]

> >  At least one antivirus software (which allows
> > anonymous download of LKM source code) is using LSM hooks since
> > Linux 2.6.32
> > instead of rewriting syscall tables. We are already allowing
> > multiple concurrent
> > LSM modules (up to one fully armored module which uses "struct
> > cred"->security
> > field or exclusive hooks like security_xfrm_state_pol_flow_match(),
> > plus
> > unlimited number of lightweight modules which do not use "struct
> > cred"->security
> > nor exclusive hooks) as long as they are built into the kernel.
> > There is no
> > reason to keep LKM based LSM modules from antivirus software or
> > alike away.
> 
> We're not to the point where in-kernel modules are stacking fully.
> Not everyone is on board for that, but hope springs eternal. Part
> of the design criteria I'm working under is that it shouldn't
> preclude loadable modules, and I still think that's doable. The
> patch James proposed is completely compatible with this philosophy.
> You can argue that it requires a loadable module configuration be
> less "hardened", but the opponents of loadable modules say that is
> inherent to loadable modules.

FWIW, the full infrastructure for read-only data from PaX includes a way
to make data temporary writable for a kernel thread. In PaX,
__ro_after_init was/is called __read_only and pax_open_kernel /
pax_close_kernel make it usable for rarely written data. That could
easily land before loadable LSMs.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 866 bytes --]

Re: [kernel-hardening] Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Tetsuo Handa]

Casey Schaufler wrote:
> On 2/16/2017 3:00 AM, Tetsuo Handa wrote:
> > Casey Schaufler wrote:
> >> I can't say that I'm buying the value of the additional
> >> complexity here. Sure, you're protecting part of the data
> >> all the time, but you're exposing part all the time, too.
> > Will you explain it in detail? As far as I know, __ro_after_init
> > annotation makes the kernel to call set_memory_ro() after __init
> > functions are completed, by gathering such variables into a section
> > so that set_memory_ro() can change page flags for a memory region
> > where it is PAGE_ALIGNED and the size is multiple of PAGE_SIZE bytes.
> >
> > This "struct lsm_entry" is for gathering only LSM related variables which
> > would have been marked as __ro_after_init if CONFIG_SECURITY_WRITABLE_HOOKS=n
> > into a list of memory regions where they are PAGE_ALIGNED and the size is
> > multiple of PAGE_SIZE bytes, so that the kernel can call set_memory_ro() on
> > these memory regions even if CONFIG_SECURITY_SELINUX_DISABLE=y or allowing
> > LKM based LSMs.
> 
> All I'm saying is that it's simpler to mark the entire
> structure than it is to mark parts.

I'm still unable to understand.
"struct lsm_entry" != "the entire structure" ?

> 
> >> For now I think that the solution James proposes makes the
> >> most sense. In the hypothetical loadable modules case I
> >> don't see real value in hardening bits of the data while
> >> explicitly making the most important parts dynamic.
> > Best solution for environments where it is known to enforce only one specific
> > LSM module and no user choices and no LKM based LSMs (including antivirus or
> > alike) is to directly embed that LSM module into security/security.c .
> 
> There are too few beers on the table in front of me
> to start that discussion. :)
> 
> > CONFIG_SECURITY_WRITABLE_HOOKS=n depends on CONFIG_SECURITY_SELINUX_DISABLE=n
> > but many of distributor's kernels enable multiple LSM modules including
> > CONFIG_SECURITY_SELINUX_DISABLE=y. Also, people are using antivirus software
> > on distributor's kernels.
> 
> I have to limit my comment on that to pointing out
> that we can't make decisions based on code that has
> never been proposed for the mainline.

If I recall correctly, LSM framework was considered as "should be used by only
rule based access restriction mechanisms (so-called MAC)". Hooks for e.g.
antivirus modules should use different interfaces (e.g. fsnotify_open())
which do not offer ability to reject access requests synchronously.
I think that this technical barrier was removed by commit b1d9e6b0646d0e5e "LSM:
Switch to lists of hooks". But there still remains non-technical barrier.

We think that we should not merge similar modules which provide same functionality.
What about antivirus modules? They offer same functionality (intercept and judge
access requests) but interface details vary depending on userspace daemon which
performs actual scan. They cannot be unified, but we won't agree with merging
all product's all implementations. Do we change our mind and encourage antivirus
modules developers to try proposing for the mainline?

> 
> >  At least one antivirus software (which allows
> > anonymous download of LKM source code) is using LSM hooks since Linux 2.6.32
> > instead of rewriting syscall tables. We are already allowing multiple concurrent
> > LSM modules (up to one fully armored module which uses "struct cred"->security
> > field or exclusive hooks like security_xfrm_state_pol_flow_match(), plus
> > unlimited number of lightweight modules which do not use "struct cred"->security
> > nor exclusive hooks) as long as they are built into the kernel. There is no
> > reason to keep LKM based LSM modules from antivirus software or alike away.
> 
> We're not to the point where in-kernel modules are stacking fully.
> Not everyone is on board for that, but hope springs eternal. Part
> of the design criteria I'm working under is that it shouldn't
> preclude loadable modules, and I still think that's doable. The
> patch James proposed is completely compatible with this philosophy.
> You can argue that it requires a loadable module configuration be
> less "hardened", but the opponents of loadable modules say that is
> inherent to loadable modules.

The patch James proposed looks like a placebo for me, for many of distributor
kernels will have to choose CONFIG_SECURITY_WRITABLE_HOOKS=y. If we offer a way to
call set_memory_ro()/set_memory_rw(), such distributor kernels will be able to
behave like CONFIG_SECURITY_WRITABLE_HOOKS=n.

> 
> > For such kernels, this "struct lsm_entry" allows calling set_memory_ro() on LSM
> > related variables which would have been marked as __ro_after_init if
> > CONFIG_SECURITY_WRITABLE_HOOKS=n.
> >
> > If you are not happy with using kmalloc() for copying "struct security_hook_list"
> > in each LSM module, can we agree with padding "struct security_hook_list" in each
> > LSM module PAGE_ALIGNED and the size being multiple of PAGE_SIZE bytes?
> 
> I don't understand what that would help without
> the rest of your changes.

Of course I mean with the rest of my changes in order to track list of LSMs.
pax_open_kernel() might be an interesting approach which will eliminate the need
of tracking list of LSMs.

Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Stephen Smalley]

On Wed, 2017-02-15 at 00:17 +1100, James Morris wrote:
> Subsequent patches will add RO hardening to LSM hooks, however,
> SELinux
> still needs to be able to perform runtime disablement after init to
> handle
> architectures where init-time disablement via boot parameters is not
> feasible.
> 
> Introduce a new kernel configuration parameter
> CONFIG_SECURITY_WRITABLE_HOOKS,
> and a helper macro __lsm_ro_after_init, to handle this case.
> 
> Signed-off-by: James Morris <james.l.morris@oracle.com>

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> ---
>  include/linux/lsm_hooks.h |    7 +++++++
>  security/Kconfig          |    5 +++++
>  security/selinux/Kconfig  |    6 ++++++
>  3 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index e29d4c6..c4b149f 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1908,6 +1908,13 @@ 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
> +#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/Kconfig b/security/Kconfig
> index 118f454..f6f90c4 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -31,6 +31,11 @@ config SECURITY
>  
>  	  If you are unsure how to answer this question, answer N.
>  
> +config SECURITY_WRITABLE_HOOKS
> +	depends on SECURITY
> +	bool
> +	default n
> +
>  config SECURITYFS
>  	bool "Enable the securityfs filesystem"
>  	help
> diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
> index ea7e3ef..8af7a69 100644
> --- a/security/selinux/Kconfig
> +++ b/security/selinux/Kconfig
> @@ -40,6 +40,7 @@ config SECURITY_SELINUX_BOOTPARAM_VALUE
>  config SECURITY_SELINUX_DISABLE
>  	bool "NSA SELinux runtime disable"
>  	depends on SECURITY_SELINUX
> +	select SECURITY_WRITABLE_HOOKS
>  	default n
>  	help
>  	  This option enables writing to a selinuxfs node 'disable',
> which
> @@ -50,6 +51,11 @@ config SECURITY_SELINUX_DISABLE
>  	  portability across platforms where boot parameters are
> difficult
>  	  to employ.
>  
> +	  NOTE: selecting this option will disable the
> '__ro_after_init'
> +	  kernel hardening feature for security hooks.   Please
> consider
> +	  using the selinux=0 boot parameter instead of enabling
> this
> +	  option.
> +
>  	  If you are unsure how to answer this question, answer N.
>  
>  config SECURITY_SELINUX_DEVELOP

[kernel-hardening] Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Stephen Smalley]

On Wed, 2017-02-15 at 00:17 +1100, James Morris wrote:
> Subsequent patches will add RO hardening to LSM hooks, however,
> SELinux
> still needs to be able to perform runtime disablement after init to
> handle
> architectures where init-time disablement via boot parameters is not
> feasible.
> 
> Introduce a new kernel configuration parameter
> CONFIG_SECURITY_WRITABLE_HOOKS,
> and a helper macro __lsm_ro_after_init, to handle this case.
> 
> Signed-off-by: James Morris <james.l.morris@oracle.com>

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> ---
>  include/linux/lsm_hooks.h |    7 +++++++
>  security/Kconfig          |    5 +++++
>  security/selinux/Kconfig  |    6 ++++++
>  3 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index e29d4c6..c4b149f 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1908,6 +1908,13 @@ 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
> +#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/Kconfig b/security/Kconfig
> index 118f454..f6f90c4 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -31,6 +31,11 @@ config SECURITY
>  
>  	  If you are unsure how to answer this question, answer N.
>  
> +config SECURITY_WRITABLE_HOOKS
> +	depends on SECURITY
> +	bool
> +	default n
> +
>  config SECURITYFS
>  	bool "Enable the securityfs filesystem"
>  	help
> diff --git a/security/selinux/Kconfig b/security/selinux/Kconfig
> index ea7e3ef..8af7a69 100644
> --- a/security/selinux/Kconfig
> +++ b/security/selinux/Kconfig
> @@ -40,6 +40,7 @@ config SECURITY_SELINUX_BOOTPARAM_VALUE
>  config SECURITY_SELINUX_DISABLE
>  	bool "NSA SELinux runtime disable"
>  	depends on SECURITY_SELINUX
> +	select SECURITY_WRITABLE_HOOKS
>  	default n
>  	help
>  	  This option enables writing to a selinuxfs node 'disable',
> which
> @@ -50,6 +51,11 @@ config SECURITY_SELINUX_DISABLE
>  	  portability across platforms where boot parameters are
> difficult
>  	  to employ.
>  
> +	  NOTE: selecting this option will disable the
> '__ro_after_init'
> +	  kernel hardening feature for security hooks.   Please
> consider
> +	  using the selinux=0 boot parameter instead of enabling
> this
> +	  option.
> +
>  	  If you are unsure how to answer this question, answer N.
>  
>  config SECURITY_SELINUX_DEVELOP

Re: [kernel-hardening] Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS

[Casey Schaufler]

On 2/17/2017 7:05 AM, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>> On 2/16/2017 3:00 AM, Tetsuo Handa wrote:
>>> Casey Schaufler wrote:
>>>> I can't say that I'm buying the value of the additional
>>>> complexity here. Sure, you're protecting part of the data
>>>> all the time, but you're exposing part all the time, too.
>>> Will you explain it in detail? As far as I know, __ro_after_init
>>> annotation makes the kernel to call set_memory_ro() after __init
>>> functions are completed, by gathering such variables into a section
>>> so that set_memory_ro() can change page flags for a memory region
>>> where it is PAGE_ALIGNED and the size is multiple of PAGE_SIZE bytes.
>>>
>>> This "struct lsm_entry" is for gathering only LSM related variables which
>>> would have been marked as __ro_after_init if CONFIG_SECURITY_WRITABLE_HOOKS=n
>>> into a list of memory regions where they are PAGE_ALIGNED and the size is
>>> multiple of PAGE_SIZE bytes, so that the kernel can call set_memory_ro() on
>>> these memory regions even if CONFIG_SECURITY_SELINUX_DISABLE=y or allowing
>>> LKM based LSMs.
>> All I'm saying is that it's simpler to mark the entire
>> structure than it is to mark parts.
> I'm still unable to understand.
> "struct lsm_entry" != "the entire structure" ?
>
>>>> For now I think that the solution James proposes makes the
>>>> most sense. In the hypothetical loadable modules case I
>>>> don't see real value in hardening bits of the data while
>>>> explicitly making the most important parts dynamic.
>>> Best solution for environments where it is known to enforce only one specific
>>> LSM module and no user choices and no LKM based LSMs (including antivirus or
>>> alike) is to directly embed that LSM module into security/security.c .
>> There are too few beers on the table in front of me
>> to start that discussion. :)
>>
>>> CONFIG_SECURITY_WRITABLE_HOOKS=n depends on CONFIG_SECURITY_SELINUX_DISABLE=n
>>> but many of distributor's kernels enable multiple LSM modules including
>>> CONFIG_SECURITY_SELINUX_DISABLE=y. Also, people are using antivirus software
>>> on distributor's kernels.
>> I have to limit my comment on that to pointing out
>> that we can't make decisions based on code that has
>> never been proposed for the mainline.
> If I recall correctly, LSM framework was considered as "should be used by only
> rule based access restriction mechanisms (so-called MAC)". Hooks for e.g.
> antivirus modules should use different interfaces (e.g. fsnotify_open())
> which do not offer ability to reject access requests synchronously.
> I think that this technical barrier was removed by commit b1d9e6b0646d0e5e "LSM:
> Switch to lists of hooks". But there still remains non-technical barrier.

The LSM infrastructure is 20 years old (or close too) and
as anyone who's raised a child can attest, what you expected
in the beginning is rarely what you end up with. We have some
really good implementations of MAC, but they have never been
the glint in the community's eye.


> We think that we should not merge similar modules which provide same functionality.
> What about antivirus modules? They offer same functionality (intercept and judge
> access requests) but interface details vary depending on userspace daemon which
> performs actual scan. They cannot be unified, but we won't agree with merging
> all product's all implementations. Do we change our mind and encourage antivirus
> modules developers to try proposing for the mainline?

Security means so many things to so many people
(There are people who think queuing up for an hour
to get a body scan is an element of security.)
that restricting the LSM infrastructure to MAC seems
a bit arrogant. I am *not* saying that I support
allowing security modules that are nothing more than
interfaces for proprietary, out of tree mechanisms.
Those are evil. I would encourage an antivirus (or
time based access control, or content based denial)
to use the LSM infrastructure if it is all going upstream,
where it belongs.

>>>  At least one antivirus software (which allows
>>> anonymous download of LKM source code) is using LSM hooks since Linux 2.6.32
>>> instead of rewriting syscall tables. We are already allowing multiple concurrent
>>> LSM modules (up to one fully armored module which uses "struct cred"->security
>>> field or exclusive hooks like security_xfrm_state_pol_flow_match(), plus
>>> unlimited number of lightweight modules which do not use "struct cred"->security
>>> nor exclusive hooks) as long as they are built into the kernel. There is no
>>> reason to keep LKM based LSM modules from antivirus software or alike away.
>> We're not to the point where in-kernel modules are stacking fully.
>> Not everyone is on board for that, but hope springs eternal. Part
>> of the design criteria I'm working under is that it shouldn't
>> preclude loadable modules, and I still think that's doable. The
>> patch James proposed is completely compatible with this philosophy.
>> You can argue that it requires a loadable module configuration be
>> less "hardened", but the opponents of loadable modules say that is
>> inherent to loadable modules.
> The patch James proposed looks like a placebo for me, for many of distributor
> kernels will have to choose CONFIG_SECURITY_WRITABLE_HOOKS=y. If we offer a way to
> call set_memory_ro()/set_memory_rw(), such distributor kernels will be able to
> behave like CONFIG_SECURITY_WRITABLE_HOOKS=n.
>
>>> For such kernels, this "struct lsm_entry" allows calling set_memory_ro() on LSM
>>> related variables which would have been marked as __ro_after_init if
>>> CONFIG_SECURITY_WRITABLE_HOOKS=n.
>>>
>>> If you are not happy with using kmalloc() for copying "struct security_hook_list"
>>> in each LSM module, can we agree with padding "struct security_hook_list" in each
>>> LSM module PAGE_ALIGNED and the size being multiple of PAGE_SIZE bytes?
>> I don't understand what that would help without
>> the rest of your changes.
> Of course I mean with the rest of my changes in order to track list of LSMs.
> pax_open_kernel() might be an interesting approach which will eliminate the need
> of tracking list of LSMs.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>