Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS
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_headlist;
> - struct list_head*head;
> union security_list_options hook;
> - char*lsm;
> + const char *lsm;
> + unsigned intoffset;
> +};
> +
> +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 = _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([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_MAX10
>
> +static bool
Re: [RFC v2 PATCH 1/2] security: introduce CONFIG_SECURITY_WRITABLE_HOOKS
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_headlist;
- struct list_head*head;
union security_list_options hook;
- char*lsm;
+ const char *lsm;
+ unsigned intoffset;
+};
+
+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 = _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([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, _entry_list, head) {
+ printk("Marking LSM hooks at %p read only.\n", tmp);
+ set_memory_ro((unsigned long) tmp,
