| Message ID | 20170605192216.21596-5-igor.stoppa@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 6/5/2017 12:22 PM, Igor Stoppa wrote: > This patch shows how it is possible to take advantage of pmalloc: > instead of using the build-time option __lsm_ro_after_init, to decide if > it is possible to keep the hooks modifiable, now this becomes a > boot-time decision, based on the kernel command line. > > This patch relies on: > > "Convert security_hook_heads into explicit array of struct list_head" > Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > > to break free from the static constraint imposed by the previous > hardening model, based on __ro_after_init. > > Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com> > CC: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > init/main.c | 2 ++ > security/security.c | 29 ++++++++++++++++++++++++++--- > 2 files changed, 28 insertions(+), 3 deletions(-) > > diff --git a/init/main.c b/init/main.c > index f866510..7850887 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -485,6 +485,7 @@ static void __init mm_init(void) > ioremap_huge_init(); > } > > +extern int __init pmalloc_init(void); > asmlinkage __visible void __init start_kernel(void) > { > char *command_line; > @@ -653,6 +654,7 @@ asmlinkage __visible void __init start_kernel(void) > proc_caches_init(); > buffer_init(); > key_init(); > + pmalloc_init(); > security_init(); > dbg_late_init(); > vfs_caches_init(); > diff --git a/security/security.c b/security/security.c > index c492f68..4285545 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -26,6 +26,7 @@ > #include <linux/personality.h> > #include <linux/backing-dev.h> > #include <linux/string.h> > +#include <linux/pmalloc.h> > #include <net/flow.h> > > #define MAX_LSM_EVM_XATTR 2 > @@ -33,8 +34,17 @@ > /* Maximum number of letters for an LSM name string */ > #define SECURITY_NAME_MAX 10 > > -static struct list_head hook_heads[LSM_MAX_HOOK_INDEX] > - __lsm_ro_after_init; > +static int security_debug; > + > +static __init int set_security_debug(char *str) > +{ > + get_option(&str, &security_debug); > + return 0; > +} > +early_param("security_debug", set_security_debug); I don't care for calling this "security debug". Making the lists writable after init isn't about development, it's about (Tetsuo's desire for) dynamic module loading. I would prefer "dynamic_module_lists" our something else more descriptive. > + > +static struct list_head *hook_heads; > +static struct pmalloc_pool *sec_pool; > char *lsm_names; > /* Boot-time LSM user choice */ > static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] = > @@ -59,6 +69,13 @@ int __init security_init(void) > { > enum security_hook_index i; > > + sec_pool = pmalloc_create_pool("security"); > + if (!sec_pool) > + goto error_pool; Excessive gotoing - return -ENOMEM instead. > + hook_heads = pmalloc(sizeof(struct list_head) * LSM_MAX_HOOK_INDEX, > + sec_pool); > + if (!hook_heads) > + goto error_heads; This is the only case where you'd destroy the pool, so the goto is unnecessary. Put the pmalloc_destroy_pool(sec_pool); return -ENOMEM; under the if here. > for (i = 0; i < LSM_MAX_HOOK_INDEX; i++) > INIT_LIST_HEAD(&hook_heads[i]); > pr_info("Security Framework initialized\n"); > @@ -74,8 +91,14 @@ int __init security_init(void) > * Load all the remaining security modules. > */ > do_security_initcalls(); > - > + if (!security_debug) > + pmalloc_protect_pool(sec_pool); > return 0; > + > +error_heads: > + pmalloc_destroy_pool(sec_pool); > +error_pool: > + return -ENOMEM; > } > > /* Save user chosen LSM */
Casey Schaufler wrote: > > @@ -33,8 +34,17 @@ > > /* Maximum number of letters for an LSM name string */ > > #define SECURITY_NAME_MAX 10 > > > > -static struct list_head hook_heads[LSM_MAX_HOOK_INDEX] > > - __lsm_ro_after_init; > > +static int security_debug; > > + > > +static __init int set_security_debug(char *str) > > +{ > > + get_option(&str, &security_debug); > > + return 0; > > +} > > +early_param("security_debug", set_security_debug); > > I don't care for calling this "security debug". Making > the lists writable after init isn't about development, > it's about (Tetsuo's desire for) dynamic module loading. > I would prefer "dynamic_module_lists" our something else > more descriptive. Maybe dynamic_lsm ? > > > + > > +static struct list_head *hook_heads; > > +static struct pmalloc_pool *sec_pool; > > char *lsm_names; > > /* Boot-time LSM user choice */ > > static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] = > > @@ -59,6 +69,13 @@ int __init security_init(void) > > { > > enum security_hook_index i; > > > > + sec_pool = pmalloc_create_pool("security"); > > + if (!sec_pool) > > + goto error_pool; > > Excessive gotoing - return -ENOMEM instead. But does it make sense to continue? hook_heads == NULL and we will oops as soon as call_void_hook() or call_int_hook() is called for the first time.
On 05/06/17 23:50, Tetsuo Handa wrote: > Casey Schaufler wrote: [...] >> I don't care for calling this "security debug". Making >> the lists writable after init isn't about development, >> it's about (Tetsuo's desire for) dynamic module loading. >> I would prefer "dynamic_module_lists" our something else >> more descriptive. > > Maybe dynamic_lsm ? ok, apologies for misunderstanding, I'll fix it. I am not sure I understood what exactly the use case is: -1) loading off-tree modules -2) loading and unloading modules -3) something else ? I'm asking this because I now wonder if I should provide means for protecting the heads later on (which still can make sense for case 1). Or if it's expected that things will stay fluid and this dynamic loading is matched by unloading, therefore the heads must stay writable (case 2) [...] >>> + if (!sec_pool) >>> + goto error_pool; >> >> Excessive gotoing - return -ENOMEM instead. > > But does it make sense to continue? > hook_heads == NULL and we will oops as soon as > call_void_hook() or call_int_hook() is called for the first time. Shouldn't the caller check for result? -ENOMEM gives it a chance to do so. I can replace the goto. --- igor
Igor Stoppa wrote: > On 05/06/17 23:50, Tetsuo Handa wrote: > > Casey Schaufler wrote: > > [...] > > >> I don't care for calling this "security debug". Making > >> the lists writable after init isn't about development, > >> it's about (Tetsuo's desire for) dynamic module loading. > >> I would prefer "dynamic_module_lists" our something else > >> more descriptive. > > > > Maybe dynamic_lsm ? > > ok, apologies for misunderstanding, I'll fix it. > > I am not sure I understood what exactly the use case is: > -1) loading off-tree modules Does off-tree mean out-of-tree? If yes, this case is not correct. "Loading modules which are not compiled as built-in" is correct. My use case is to allow users to use LSM modules as loadable kernel modules which distributors do not compile as built-in. > -2) loading and unloading modules Unloading LSM modules is dangerous. Only SELinux allows unloading at the risk of triggering an oops. If we insert delay while removing list elements, we can easily observe oops due to free function being called without corresponding allocation function. > -3) something else ? Nothing else, as far as I know. > > I'm asking this because I now wonder if I should provide means for > protecting the heads later on (which still can make sense for case 1). > > Or if it's expected that things will stay fluid and this dynamic loading > is matched by unloading, therefore the heads must stay writable (case 2) > > [...] > > >>> + if (!sec_pool) > >>> + goto error_pool; > >> > >> Excessive gotoing - return -ENOMEM instead. > > > > But does it make sense to continue? > > hook_heads == NULL and we will oops as soon as > > call_void_hook() or call_int_hook() is called for the first time. > > Shouldn't the caller check for result? -ENOMEM gives it a chance to do > so. I can replace the goto. security_init() is called from start_kernel() in init/main.c , and errors are silently ignored. Thus, I don't think returning error to the caller makes sense.
On 06/06/17 13:54, Tetsuo Handa wrote: [...] > "Loading modules which are not compiled as built-in" is correct. > My use case is to allow users to use LSM modules as loadable kernel > modules which distributors do not compile as built-in. Ok, so I suppose someone should eventually lock down the header, after the additional modules are loaded. Who decides when enough is enough, meaning that all the needed modules are loaded? Should I provide an interface to user-space? A sysfs entry? [...] > Unloading LSM modules is dangerous. Only SELinux allows unloading > at the risk of triggering an oops. If we insert delay while removing > list elements, we can easily observe oops due to free function being > called without corresponding allocation function. Ok. But even in this case, the sys proposal would still work. It would just stay unused. -- igor
Igor Stoppa wrote: > Who decides when enough is enough, meaning that all the needed modules > are loaded? > Should I provide an interface to user-space? A sysfs entry? No such interface is needed. Just an API for applying set_memory_rw() and set_memory_ro() on LSM hooks is enough. security_add_hooks() can call set_memory_rw() before adding hooks and call set_memory_ro() after adding hooks. Ditto for security_delete_hooks() for SELinux's unregistration.
On 06/06/17 14:42, Tetsuo Handa wrote: > Igor Stoppa wrote: >> Who decides when enough is enough, meaning that all the needed modules >> are loaded? >> Should I provide an interface to user-space? A sysfs entry? > > No such interface is needed. Just an API for applying set_memory_rw() > and set_memory_ro() on LSM hooks is enough. > > security_add_hooks() can call set_memory_rw() before adding hooks and > call set_memory_ro() after adding hooks. Ditto for security_delete_hooks() > for SELinux's unregistration. I think this should be considered part of the 2nd phase "write seldom", as we agreed with Kees Cook. Right now the goal was to provide the basic API for: - create pool - get memory from pool - lock the pool - destroy the pool And, behind the scene, verify that a memory range falls into Pmalloc pages. Then would come the "write seldom" part. The reason for this is that a proper implementation of write seldom should, imho, make writable only those pages that really need to be modified. Possibly also add some verification on the call stack about who is requesting the unlocking. Therefore I would feel more comfortable in splitting the work into 2 part. For the case at hand, would it work if there was a non-API call that you could use until the API is properly expanded? -- igor
Igor Stoppa wrote: > For the case at hand, would it work if there was a non-API call that you > could use until the API is properly expanded? Kernel command line switching (i.e. this patch) is fine for my use cases. SELinux folks might want -static int security_debug; +static int security_debug = IS_ENABLED(CONFIG_SECURITY_SELINUX_DISABLE); so that those who are using SELINUX=disabled in /etc/selinux/config won't get oops upon boot by default. If "unlock the pool" were available, SELINUX=enforcing users would be happy. Maybe two modes for rw/ro transition helps. oneway rw -> ro transition mode: can't be made rw again by calling "unlock the pool" API twoway rw <-> ro transition mode: can be made rw again by calling "unlock the pool" API
On 06/06/17 17:36, Tetsuo Handa wrote: > Igor Stoppa wrote: >> For the case at hand, would it work if there was a non-API call that you >> could use until the API is properly expanded? > > Kernel command line switching (i.e. this patch) is fine for my use cases. > > SELinux folks might want > > -static int security_debug; > +static int security_debug = IS_ENABLED(CONFIG_SECURITY_SELINUX_DISABLE); ok, thanks, I will add this > so that those who are using SELINUX=disabled in /etc/selinux/config won't > get oops upon boot by default. If "unlock the pool" were available, > SELINUX=enforcing users would be happy. Maybe two modes for rw/ro transition helps. > > oneway rw -> ro transition mode: can't be made rw again by calling "unlock the pool" API > twoway rw <-> ro transition mode: can be made rw again by calling "unlock the pool" API This was in the first cut of the API, but I was told that it would require further rework, to make it ok for upstream, so we agreed to do first the lockdown/destroy only part and the the rewrite. Is there really a valid use case for unloading SE Linux? Or any other security module. -- igor
On 6/6/2017 7:51 AM, Igor Stoppa wrote: > On 06/06/17 17:36, Tetsuo Handa wrote: >> Igor Stoppa wrote: >>> For the case at hand, would it work if there was a non-API call that you >>> could use until the API is properly expanded? >> Kernel command line switching (i.e. this patch) is fine for my use cases. >> >> SELinux folks might want >> >> -static int security_debug; >> +static int security_debug = IS_ENABLED(CONFIG_SECURITY_SELINUX_DISABLE); > ok, thanks, I will add this > >> so that those who are using SELINUX=disabled in /etc/selinux/config won't >> get oops upon boot by default. If "unlock the pool" were available, >> SELINUX=enforcing users would be happy. Maybe two modes for rw/ro transition helps. >> >> oneway rw -> ro transition mode: can't be made rw again by calling "unlock the pool" API >> twoway rw <-> ro transition mode: can be made rw again by calling "unlock the pool" API > This was in the first cut of the API, but I was told that it would > require further rework, to make it ok for upstream, so we agreed to do > first the lockdown/destroy only part and the the rewrite. > > Is there really a valid use case for unloading SE Linux? It's used today in the Redhat distros. There is talk of removing it. You can only unload SELinux before policy is loaded, which is sort of saying that you have your system misconfigured but can't figure out how to fix it. You might be able to convince Paul Moore to accelerate the removal of this feature for this worthy cause. > Or any other security module. I suppose that you could argue that if a security module had been in place for 2 years on a system and had never once denied anyone access it should be removed. That's a reasonable use case description, but I doubt you'd encounter it in the real world. Another possibility is a security module that is used during container setup and once the system goes into full operation is no longer needed. Personally, I don't see either of these cases as compelling. "systemctl restart xyzzyd".
diff --git a/init/main.c b/init/main.c index f866510..7850887 100644 --- a/init/main.c +++ b/init/main.c @@ -485,6 +485,7 @@ static void __init mm_init(void) ioremap_huge_init(); } +extern int __init pmalloc_init(void); asmlinkage __visible void __init start_kernel(void) { char *command_line; @@ -653,6 +654,7 @@ asmlinkage __visible void __init start_kernel(void) proc_caches_init(); buffer_init(); key_init(); + pmalloc_init(); security_init(); dbg_late_init(); vfs_caches_init(); diff --git a/security/security.c b/security/security.c index c492f68..4285545 100644 --- a/security/security.c +++ b/security/security.c @@ -26,6 +26,7 @@ #include <linux/personality.h> #include <linux/backing-dev.h> #include <linux/string.h> +#include <linux/pmalloc.h> #include <net/flow.h> #define MAX_LSM_EVM_XATTR 2 @@ -33,8 +34,17 @@ /* Maximum number of letters for an LSM name string */ #define SECURITY_NAME_MAX 10 -static struct list_head hook_heads[LSM_MAX_HOOK_INDEX] - __lsm_ro_after_init; +static int security_debug; + +static __init int set_security_debug(char *str) +{ + get_option(&str, &security_debug); + return 0; +} +early_param("security_debug", set_security_debug); + +static struct list_head *hook_heads; +static struct pmalloc_pool *sec_pool; char *lsm_names; /* Boot-time LSM user choice */ static __initdata char chosen_lsm[SECURITY_NAME_MAX + 1] = @@ -59,6 +69,13 @@ int __init security_init(void) { enum security_hook_index i; + sec_pool = pmalloc_create_pool("security"); + if (!sec_pool) + goto error_pool; + hook_heads = pmalloc(sizeof(struct list_head) * LSM_MAX_HOOK_INDEX, + sec_pool); + if (!hook_heads) + goto error_heads; for (i = 0; i < LSM_MAX_HOOK_INDEX; i++) INIT_LIST_HEAD(&hook_heads[i]); pr_info("Security Framework initialized\n"); @@ -74,8 +91,14 @@ int __init security_init(void) * Load all the remaining security modules. */ do_security_initcalls(); - + if (!security_debug) + pmalloc_protect_pool(sec_pool); return 0; + +error_heads: + pmalloc_destroy_pool(sec_pool); +error_pool: + return -ENOMEM; } /* Save user chosen LSM */
This patch shows how it is possible to take advantage of pmalloc: instead of using the build-time option __lsm_ro_after_init, to decide if it is possible to keep the hooks modifiable, now this becomes a boot-time decision, based on the kernel command line. This patch relies on: "Convert security_hook_heads into explicit array of struct list_head" Author: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> to break free from the static constraint imposed by the previous hardening model, based on __ro_after_init. Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com> CC: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- init/main.c | 2 ++ security/security.c | 29 ++++++++++++++++++++++++++--- 2 files changed, 28 insertions(+), 3 deletions(-)