| Message ID | e9bfb2d5-d987-55ce-4011-9b32ff745d36@schaufler-ca.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | LSM: Module stacking for SARA and Landlock | expand |
On Fri, Sep 21, 2018 at 4:59 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > v4: Finer granularity in the patches and other > cleanups suggested by Kees Cook. > Removed dead code created by the removal of SELinux > credential blob poisoning. Thanks for the splitting, this really does make it easier to review (at least for me). I think this looks really good, though obviously I'd like to refactor it slightly on top of my series. :) One additional thought I had was about the blobs allocations: some are separate kmem caches, and some are kmalloc. I'm thinking it might make sense to use separate kmem caches for two reasons: - they're going to always be the same size and are regularly allocated/freed, so it may offer a performance benefit. - they're explicitly not supposed to be exposed to userspace, so hardened usercopy would protect them if they were not kmalloc()ed. I'm excited about getting this landed! -Kees
On 9/21/2018 8:02 PM, Kees Cook wrote: > On Fri, Sep 21, 2018 at 4:59 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >> v4: Finer granularity in the patches and other >> cleanups suggested by Kees Cook. >> Removed dead code created by the removal of SELinux >> credential blob poisoning. > Thanks for the splitting, this really does make it easier to review > (at least for me). I think this looks really good, though obviously > I'd like to refactor it slightly on top of my series. :) Whichever goes on top is fine with me. What's one more patch set merge, after all? > One additional thought I had was about the blobs allocations: some are > separate kmem caches, and some are kmalloc. I'm thinking it might make > sense to use separate kmem caches for two reasons: I had seriously considered doing that. I can't see any reason not to. It's something that could be done at any time, and with all the other things that had to change it just didn't get in. > - they're going to always be the same size and are regularly > allocated/freed, so it may offer a performance benefit. > > - they're explicitly not supposed to be exposed to userspace, so > hardened usercopy would protect them if they were not kmalloc()ed. > > I'm excited about getting this landed! Soon. Real soon. I hope. I would very much like for someone from the SELinux camp to chime in, especially on the selinux_is_enabled() removal. On a somewhat related note, I will be out for the first three weeks of October, returning just in time for the Linux Security Summit in Edinburgh. My connectivity will be severely limited. I don't expect to accomplish anything while I'm out.
On Sat, Sep 22, 2018 at 9:38 AM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 9/21/2018 8:02 PM, Kees Cook wrote: >> On Fri, Sep 21, 2018 at 4:59 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>> v4: Finer granularity in the patches and other >>> cleanups suggested by Kees Cook. >>> Removed dead code created by the removal of SELinux >>> credential blob poisoning. >> Thanks for the splitting, this really does make it easier to review >> (at least for me). I think this looks really good, though obviously >> I'd like to refactor it slightly on top of my series. :) > > Whichever goes on top is fine with me. What's one > more patch set merge, after all? > >> One additional thought I had was about the blobs allocations: some are >> separate kmem caches, and some are kmalloc. I'm thinking it might make >> sense to use separate kmem caches for two reasons: > > I had seriously considered doing that. I can't see any reason > not to. It's something that could be done at any time, and with > all the other things that had to change it just didn't get in. Yup; that is an easy future change. Not needed now! > >> - they're going to always be the same size and are regularly >> allocated/freed, so it may offer a performance benefit. >> >> - they're explicitly not supposed to be exposed to userspace, so >> hardened usercopy would protect them if they were not kmalloc()ed. >> >> I'm excited about getting this landed! > > Soon. Real soon. I hope. I would very much like for > someone from the SELinux camp to chime in, especially on > the selinux_is_enabled() removal. Agreed. > On a somewhat related note, I will be out for the first three > weeks of October, returning just in time for the Linux Security > Summit in Edinburgh. My connectivity will be severely limited. > I don't expect to accomplish anything while I'm out. If you're okay with it, I can help with changes while you're out -- I want to try to rebase it on my tree and see how it looks anyway. :) -Kees
On 2018/09/23 11:43, Kees Cook wrote: >>> I'm excited about getting this landed! >> >> Soon. Real soon. I hope. I would very much like for >> someone from the SELinux camp to chime in, especially on >> the selinux_is_enabled() removal. > > Agreed. > This patchset from Casey lands before the patchset from Kees, doesn't it? OK, a few comments (if I didn't overlook something). lsm_early_cred()/lsm_early_task() are called from only __init functions. lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c . lsm_early_inode() should be avoided because it is not appropriate to call panic() when lsm_early_inode() is called after __init phase. Since all free hooks are called when one of init hooks failed, each free hook needs to check whether init hook was called. An example is inode_free_security() in security/selinux/hooks.c (but not addressed in this patch). This patchset might fatally prevent LKM-based LSM modules, for LKM-based LSMs cannot count on lsm_*_alloc() because size for lsm_*_alloc() cannot be updated upon loading LKM-based LSMs. If security_file_free() is called regardless of whether lsm_file_cache is defined, LKM-based LSMs can be loaded using current behavior (apart from the fact that legitimate interface for appending to security_hook_heads is currently missing). How do you plan to handle LKM-based LSMs? include/linux/lsm_hooks.h | 6 ++---- security/security.c | 31 ++++++------------------------- security/smack/smack_lsm.c | 8 +++++++- 3 files changed, 15 insertions(+), 30 deletions(-) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 7e8b32f..8014614 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -2095,13 +2095,11 @@ static inline void __init yama_add_hooks(void) { } static inline void loadpin_add_hooks(void) { }; #endif -extern int lsm_cred_alloc(struct cred *cred, gfp_t gfp); extern int lsm_inode_alloc(struct inode *inode); #ifdef CONFIG_SECURITY -void lsm_early_cred(struct cred *cred); -void lsm_early_inode(struct inode *inode); -void lsm_early_task(struct task_struct *task); +void __init lsm_early_cred(struct cred *cred); +void __init lsm_early_task(struct task_struct *task); #endif #endif /* ! __LINUX_LSM_HOOKS_H */ diff --git a/security/security.c b/security/security.c index e7c85060..341e8df 100644 --- a/security/security.c +++ b/security/security.c @@ -267,7 +267,7 @@ int unregister_lsm_notifier(struct notifier_block *nb) * * Returns 0, or -ENOMEM if memory can't be allocated. */ -int lsm_cred_alloc(struct cred *cred, gfp_t gfp) +static int lsm_cred_alloc(struct cred *cred, gfp_t gfp) { if (blob_sizes.lbs_cred == 0) { cred->security = NULL; @@ -286,7 +286,7 @@ int lsm_cred_alloc(struct cred *cred, gfp_t gfp) * * Allocate the cred blob for all the modules if it's not already there */ -void lsm_early_cred(struct cred *cred) +void __init lsm_early_cred(struct cred *cred) { int rc; @@ -344,7 +344,7 @@ void __init security_add_blobs(struct lsm_blob_sizes *needed) * * Returns 0, or -ENOMEM if memory can't be allocated. */ -int lsm_file_alloc(struct file *file) +static int lsm_file_alloc(struct file *file) { if (!lsm_file_cache) { file->f_security = NULL; @@ -379,25 +379,6 @@ int lsm_inode_alloc(struct inode *inode) } /** - * lsm_early_inode - during initialization allocate a composite inode blob - * @inode: the inode that needs a blob - * - * Allocate the inode blob for all the modules if it's not already there - */ -void lsm_early_inode(struct inode *inode) -{ - int rc; - - if (inode == NULL) - panic("%s: NULL inode.\n", __func__); - if (inode->i_security != NULL) - return; - rc = lsm_inode_alloc(inode); - if (rc) - panic("%s: Early inode alloc failed.\n", __func__); -} - -/** * lsm_task_alloc - allocate a composite task blob * @task: the task that needs a blob * @@ -466,7 +447,7 @@ int lsm_msg_msg_alloc(struct msg_msg *mp) * * Allocate the task blob for all the modules if it's not already there */ -void lsm_early_task(struct task_struct *task) +void __init lsm_early_task(struct task_struct *task) { int rc; @@ -1202,11 +1183,11 @@ void security_file_free(struct file *file) { void *blob; + call_void_hook(file_free_security, file); + if (!lsm_file_cache) return; - call_void_hook(file_free_security, file); - blob = file->f_security; file->f_security = NULL; kmem_cache_free(lsm_file_cache, blob); diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 7843004..b0b4045 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -750,6 +750,13 @@ static int smack_set_mnt_opts(struct super_block *sb, if (sp->smk_flags & SMK_SB_INITIALIZED) return 0; + if (inode->i_security == NULL) { + int rc = lsm_inode_alloc(inode); + + if (rc) + return rc; + } + if (!smack_privileged(CAP_MAC_ADMIN)) { /* * Unprivileged mounts don't get to specify Smack values. @@ -818,7 +825,6 @@ static int smack_set_mnt_opts(struct super_block *sb, /* * Initialize the root inode. */ - lsm_early_inode(inode); init_inode_smack(inode, sp->smk_root); if (transmute) {
On 9/23/2018 8:59 AM, Tetsuo Handa wrote: > On 2018/09/23 11:43, Kees Cook wrote: >>>> I'm excited about getting this landed! >>> Soon. Real soon. I hope. I would very much like for >>> someone from the SELinux camp to chime in, especially on >>> the selinux_is_enabled() removal. >> Agreed. >> > This patchset from Casey lands before the patchset from Kees, doesn't it? That is up for negotiation. We may end up combining them. > OK, a few comments (if I didn't overlook something). > > lsm_early_cred()/lsm_early_task() are called from only __init functions. True. > lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c . Also true. > lsm_early_inode() should be avoided because it is not appropriate to > call panic() when lsm_early_inode() is called after __init phase. You're correct. In fact, lsm_early_inode() isn't needed at all until multiple inode using modules are supported. > Since all free hooks are called when one of init hooks failed, each > free hook needs to check whether init hook was called. An example is > inode_free_security() in security/selinux/hooks.c (but not addressed in > this patch). I *think* that selinux_inode_free_security() is safe in this case because the blob will be zeroed, hence isec->list will be NULL. > This patchset might fatally prevent LKM-based LSM modules, for LKM-based > LSMs cannot count on lsm_*_alloc() because size for lsm_*_alloc() cannot > be updated upon loading LKM-based LSMs. LKM based security modules will require dynamically sized blobs. These can be added to the scheme used here. Each blob would get a header identifying the modules for which it contains data. When an LKM is registered if has to declare it's blob space requirements and gets back the offsets. All alloc operations have to put their marks in the header. All LKM blob users have to check that the blob they are looking at has the required data. module_cred(struct cred *cred) { return cred->security + module_blob_sizes.lbs_cred; } becomes module_cred(struct cred *cred) { if (blob_includes(module_id)) return cred->security + module_blob_sizes.lbs_cred; return NULL; } and the calling code needs to accept a NULL return. Blobs can never get smaller because readjusting the offsets isn't going to work, so unloading an LKM security module isn't going to be as complete as you might like. There may be a way around this if you unload all the LKM modules, but that's a special case and there may be dragon lurking in the mist. > If security_file_free() is called > regardless of whether lsm_file_cache is defined, LKM-based LSMs can be > loaded using current behavior (apart from the fact that legitimate > interface for appending to security_hook_heads is currently missing). > How do you plan to handle LKM-based LSMs? My position all along has been that I don't plan to handle LKM based LSMs, but that I won't do anything to prevent someone else from adding them later. I believe that I've done that. Several designs, including a separate list for dynamically loaded modules have been proposed. I think some of those would work. > include/linux/lsm_hooks.h | 6 ++---- > security/security.c | 31 ++++++------------------------- > security/smack/smack_lsm.c | 8 +++++++- > 3 files changed, 15 insertions(+), 30 deletions(-) > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 7e8b32f..8014614 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -2095,13 +2095,11 @@ static inline void __init yama_add_hooks(void) { } > static inline void loadpin_add_hooks(void) { }; > #endif > > -extern int lsm_cred_alloc(struct cred *cred, gfp_t gfp); > extern int lsm_inode_alloc(struct inode *inode); > > #ifdef CONFIG_SECURITY > -void lsm_early_cred(struct cred *cred); > -void lsm_early_inode(struct inode *inode); > -void lsm_early_task(struct task_struct *task); > +void __init lsm_early_cred(struct cred *cred); > +void __init lsm_early_task(struct task_struct *task); > #endif > > #endif /* ! __LINUX_LSM_HOOKS_H */ > diff --git a/security/security.c b/security/security.c > index e7c85060..341e8df 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -267,7 +267,7 @@ int unregister_lsm_notifier(struct notifier_block *nb) > * > * Returns 0, or -ENOMEM if memory can't be allocated. > */ > -int lsm_cred_alloc(struct cred *cred, gfp_t gfp) > +static int lsm_cred_alloc(struct cred *cred, gfp_t gfp) > { > if (blob_sizes.lbs_cred == 0) { > cred->security = NULL; > @@ -286,7 +286,7 @@ int lsm_cred_alloc(struct cred *cred, gfp_t gfp) > * > * Allocate the cred blob for all the modules if it's not already there > */ > -void lsm_early_cred(struct cred *cred) > +void __init lsm_early_cred(struct cred *cred) > { > int rc; > > @@ -344,7 +344,7 @@ void __init security_add_blobs(struct lsm_blob_sizes *needed) > * > * Returns 0, or -ENOMEM if memory can't be allocated. > */ > -int lsm_file_alloc(struct file *file) > +static int lsm_file_alloc(struct file *file) > { > if (!lsm_file_cache) { > file->f_security = NULL; > @@ -379,25 +379,6 @@ int lsm_inode_alloc(struct inode *inode) > } > > /** > - * lsm_early_inode - during initialization allocate a composite inode blob > - * @inode: the inode that needs a blob > - * > - * Allocate the inode blob for all the modules if it's not already there > - */ > -void lsm_early_inode(struct inode *inode) > -{ > - int rc; > - > - if (inode == NULL) > - panic("%s: NULL inode.\n", __func__); > - if (inode->i_security != NULL) > - return; > - rc = lsm_inode_alloc(inode); > - if (rc) > - panic("%s: Early inode alloc failed.\n", __func__); > -} > - > -/** > * lsm_task_alloc - allocate a composite task blob > * @task: the task that needs a blob > * > @@ -466,7 +447,7 @@ int lsm_msg_msg_alloc(struct msg_msg *mp) > * > * Allocate the task blob for all the modules if it's not already there > */ > -void lsm_early_task(struct task_struct *task) > +void __init lsm_early_task(struct task_struct *task) > { > int rc; > > @@ -1202,11 +1183,11 @@ void security_file_free(struct file *file) > { > void *blob; > > + call_void_hook(file_free_security, file); > + > if (!lsm_file_cache) > return; > > - call_void_hook(file_free_security, file); > - Why does this make sense? If the lsm_file_cache isn't initialized you can't have allocated any file blobs, no module can have initialized a file blob, hence there can be nothing for the module to do. > blob = file->f_security; > file->f_security = NULL; > kmem_cache_free(lsm_file_cache, blob); > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index 7843004..b0b4045 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -750,6 +750,13 @@ static int smack_set_mnt_opts(struct super_block *sb, > if (sp->smk_flags & SMK_SB_INITIALIZED) > return 0; > > + if (inode->i_security == NULL) { > + int rc = lsm_inode_alloc(inode); > + > + if (rc) > + return rc; > + } > + > if (!smack_privileged(CAP_MAC_ADMIN)) { > /* > * Unprivileged mounts don't get to specify Smack values. > @@ -818,7 +825,6 @@ static int smack_set_mnt_opts(struct super_block *sb, > /* > * Initialize the root inode. > */ > - lsm_early_inode(inode); > init_inode_smack(inode, sp->smk_root); > > if (transmute) {
On 2018/09/24 2:09, Casey Schaufler wrote: >> Since all free hooks are called when one of init hooks failed, each >> free hook needs to check whether init hook was called. An example is >> inode_free_security() in security/selinux/hooks.c (but not addressed in >> this patch). > > I *think* that selinux_inode_free_security() is safe in this > case because the blob will be zeroed, hence isec->list will > be NULL. > OK. >> This patchset might fatally prevent LKM-based LSM modules, for LKM-based >> LSMs cannot count on lsm_*_alloc() because size for lsm_*_alloc() cannot >> be updated upon loading LKM-based LSMs. > > LKM based security modules will require dynamically sized blobs. > These can be added to the scheme used here. Each blob would get a > header identifying the modules for which it contains data. When an > LKM is registered if has to declare it's blob space requirements > and gets back the offsets. All alloc operations have to put their > marks in the header. All LKM blob users have to check that the blob > they are looking at has the required data. > > module_cred(struct cred *cred) { > return cred->security + module_blob_sizes.lbs_cred; > } > > becomes > > module_cred(struct cred *cred) { > if (blob_includes(module_id)) > return cred->security + module_blob_sizes.lbs_cred; > return NULL; > } > > and the calling code needs to accept a NULL return. Not all of LKM-based LSMs use security blobs. And some of LKM-based LSMs might use security blobs for only a few objects. For example, AKARI uses inode security blob for remembering whether source address/port of an accept()ed socket was already checked, only during accept() operation and first socket operation on the accept()ed socket. Thus, there is no need to waste memory by assigning blobs for all inode objects. > Blobs can never get smaller because readjusting the offsets > isn't going to work, so unloading an LKM security module isn't > going to be as complete as you might like. There may be a way > around this if you unload all the LKM modules, but that's a > special case and there may be dragon lurking in the mist. If LKM-based LSMs who want to use security blobs have to check for NULL return, they might choose "not using infrastructure managed security blobs" and "using locally hashed blobs associated with object's address" (like AKARI does). > >> If security_file_free() is called >> regardless of whether lsm_file_cache is defined, LKM-based LSMs can be >> loaded using current behavior (apart from the fact that legitimate >> interface for appending to security_hook_heads is currently missing). >> How do you plan to handle LKM-based LSMs? > > My position all along has been that I don't plan to handle LKM > based LSMs, but that I won't do anything to prevent someone else > from adding them later. I believe that I've done that. Several > designs, including a separate list for dynamically loaded modules > have been proposed. I think some of those would work. Though AKARI is not using security_file_free(), some of LKM-based LSMs might want to use it. If file_free_security hook is called unconditionally, such LKM-based LSMs can be registered/unregistered, without worrying about inability to shrink sizes for blobs. >> @@ -1202,11 +1183,11 @@ void security_file_free(struct file *file) >> { >> void *blob; >> >> + call_void_hook(file_free_security, file); >> + >> if (!lsm_file_cache) >> return; >> >> - call_void_hook(file_free_security, file); >> - > > Why does this make sense? If the lsm_file_cache isn't > initialized you can't have allocated any file blobs, > no module can have initialized a file blob, hence there > can be nothing for the module to do. > For modules (not limited to LKM-based LSMs) which want to use file blobs for only a few objects and avoid wasting memory by allocating file blobs to all file objects. Infrastructure based blob management fits well for LSM modules which want to assign blobs to all objects (like SELinux). But forcing infrastructure based blob management can become a huge waste of memory for LSM modules which want to assign blobs to only a few objects. Unconditionally calling file_free_security hook (as with other hooks) preserves a room for allowing the latter type of LSM modules without using infrastructure based blob management.
On 09/23/2018 01:09 PM, Casey Schaufler wrote: > On 9/23/2018 8:59 AM, Tetsuo Handa wrote: >> On 2018/09/23 11:43, Kees Cook wrote: >>>>> I'm excited about getting this landed! >>>> Soon. Real soon. I hope. I would very much like for >>>> someone from the SELinux camp to chime in, especially on >>>> the selinux_is_enabled() removal. >>> Agreed. >>> >> This patchset from Casey lands before the patchset from Kees, doesn't it? > > That is up for negotiation. We may end up combining them. > >> OK, a few comments (if I didn't overlook something). >> >> lsm_early_cred()/lsm_early_task() are called from only __init functions. > > True. > >> lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c . > > Also true. > >> lsm_early_inode() should be avoided because it is not appropriate to >> call panic() when lsm_early_inode() is called after __init phase. > > You're correct. In fact, lsm_early_inode() isn't needed at all > until multiple inode using modules are supported. > >> Since all free hooks are called when one of init hooks failed, each >> free hook needs to check whether init hook was called. An example is >> inode_free_security() in security/selinux/hooks.c (but not addressed in >> this patch). > > I *think* that selinux_inode_free_security() is safe in this > case because the blob will be zeroed, hence isec->list will > be NULL. That's not safe - look more closely at what list_empty_careful() tests, and then think about what happens when list_del_init() gets called on that isec->list. selinux_inode_free_security() presumes that selinux_inode_alloc_security() has been called already. If you are breaking that assumption, you have to fix it. Is there a reason you can't make inode_alloc_security() return void since you moved the allocation to the framework? Unfortunate that inode_init_security name is already in use for another purpose since essentially you have reduced these hooks to initialization only. > >> This patchset might fatally prevent LKM-based LSM modules, for LKM-based >> LSMs cannot count on lsm_*_alloc() because size for lsm_*_alloc() cannot >> be updated upon loading LKM-based LSMs. > > LKM based security modules will require dynamically sized blobs. > These can be added to the scheme used here. Each blob would get a > header identifying the modules for which it contains data. When an > LKM is registered if has to declare it's blob space requirements > and gets back the offsets. All alloc operations have to put their > marks in the header. All LKM blob users have to check that the blob > they are looking at has the required data. > > module_cred(struct cred *cred) { > return cred->security + module_blob_sizes.lbs_cred; > } > > becomes > > module_cred(struct cred *cred) { > if (blob_includes(module_id)) > return cred->security + module_blob_sizes.lbs_cred; > return NULL; > } > > and the calling code needs to accept a NULL return. > Blobs can never get smaller because readjusting the offsets > isn't going to work, so unloading an LKM security module isn't > going to be as complete as you might like. There may be a way > around this if you unload all the LKM modules, but that's a > special case and there may be dragon lurking in the mist. > >> If security_file_free() is called >> regardless of whether lsm_file_cache is defined, LKM-based LSMs can be >> loaded using current behavior (apart from the fact that legitimate >> interface for appending to security_hook_heads is currently missing). >> How do you plan to handle LKM-based LSMs? > > My position all along has been that I don't plan to handle LKM > based LSMs, but that I won't do anything to prevent someone else > from adding them later. I believe that I've done that. Several > designs, including a separate list for dynamically loaded modules > have been proposed. I think some of those would work. > >> include/linux/lsm_hooks.h | 6 ++---- >> security/security.c | 31 ++++++------------------------- >> security/smack/smack_lsm.c | 8 +++++++- >> 3 files changed, 15 insertions(+), 30 deletions(-) >> >> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >> index 7e8b32f..8014614 100644 >> --- a/include/linux/lsm_hooks.h >> +++ b/include/linux/lsm_hooks.h >> @@ -2095,13 +2095,11 @@ static inline void __init yama_add_hooks(void) { } >> static inline void loadpin_add_hooks(void) { }; >> #endif >> >> -extern int lsm_cred_alloc(struct cred *cred, gfp_t gfp); >> extern int lsm_inode_alloc(struct inode *inode); >> >> #ifdef CONFIG_SECURITY >> -void lsm_early_cred(struct cred *cred); >> -void lsm_early_inode(struct inode *inode); >> -void lsm_early_task(struct task_struct *task); >> +void __init lsm_early_cred(struct cred *cred); >> +void __init lsm_early_task(struct task_struct *task); >> #endif >> >> #endif /* ! __LINUX_LSM_HOOKS_H */ >> diff --git a/security/security.c b/security/security.c >> index e7c85060..341e8df 100644 >> --- a/security/security.c >> +++ b/security/security.c >> @@ -267,7 +267,7 @@ int unregister_lsm_notifier(struct notifier_block *nb) >> * >> * Returns 0, or -ENOMEM if memory can't be allocated. >> */ >> -int lsm_cred_alloc(struct cred *cred, gfp_t gfp) >> +static int lsm_cred_alloc(struct cred *cred, gfp_t gfp) >> { >> if (blob_sizes.lbs_cred == 0) { >> cred->security = NULL; >> @@ -286,7 +286,7 @@ int lsm_cred_alloc(struct cred *cred, gfp_t gfp) >> * >> * Allocate the cred blob for all the modules if it's not already there >> */ >> -void lsm_early_cred(struct cred *cred) >> +void __init lsm_early_cred(struct cred *cred) >> { >> int rc; >> >> @@ -344,7 +344,7 @@ void __init security_add_blobs(struct lsm_blob_sizes *needed) >> * >> * Returns 0, or -ENOMEM if memory can't be allocated. >> */ >> -int lsm_file_alloc(struct file *file) >> +static int lsm_file_alloc(struct file *file) >> { >> if (!lsm_file_cache) { >> file->f_security = NULL; >> @@ -379,25 +379,6 @@ int lsm_inode_alloc(struct inode *inode) >> } >> >> /** >> - * lsm_early_inode - during initialization allocate a composite inode blob >> - * @inode: the inode that needs a blob >> - * >> - * Allocate the inode blob for all the modules if it's not already there >> - */ >> -void lsm_early_inode(struct inode *inode) >> -{ >> - int rc; >> - >> - if (inode == NULL) >> - panic("%s: NULL inode.\n", __func__); >> - if (inode->i_security != NULL) >> - return; >> - rc = lsm_inode_alloc(inode); >> - if (rc) >> - panic("%s: Early inode alloc failed.\n", __func__); >> -} >> - >> -/** >> * lsm_task_alloc - allocate a composite task blob >> * @task: the task that needs a blob >> * >> @@ -466,7 +447,7 @@ int lsm_msg_msg_alloc(struct msg_msg *mp) >> * >> * Allocate the task blob for all the modules if it's not already there >> */ >> -void lsm_early_task(struct task_struct *task) >> +void __init lsm_early_task(struct task_struct *task) >> { >> int rc; >> >> @@ -1202,11 +1183,11 @@ void security_file_free(struct file *file) >> { >> void *blob; >> >> + call_void_hook(file_free_security, file); >> + >> if (!lsm_file_cache) >> return; >> >> - call_void_hook(file_free_security, file); >> - > > Why does this make sense? If the lsm_file_cache isn't > initialized you can't have allocated any file blobs, > no module can have initialized a file blob, hence there > can be nothing for the module to do. > >> blob = file->f_security; >> file->f_security = NULL; >> kmem_cache_free(lsm_file_cache, blob); >> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >> index 7843004..b0b4045 100644 >> --- a/security/smack/smack_lsm.c >> +++ b/security/smack/smack_lsm.c >> @@ -750,6 +750,13 @@ static int smack_set_mnt_opts(struct super_block *sb, >> if (sp->smk_flags & SMK_SB_INITIALIZED) >> return 0; >> >> + if (inode->i_security == NULL) { >> + int rc = lsm_inode_alloc(inode); >> + >> + if (rc) >> + return rc; >> + } >> + >> if (!smack_privileged(CAP_MAC_ADMIN)) { >> /* >> * Unprivileged mounts don't get to specify Smack values. >> @@ -818,7 +825,6 @@ static int smack_set_mnt_opts(struct super_block *sb, >> /* >> * Initialize the root inode. >> */ >> - lsm_early_inode(inode); >> init_inode_smack(inode, sp->smk_root); >> >> if (transmute) { >
On 9/24/2018 8:01 AM, Stephen Smalley wrote: > On 09/23/2018 01:09 PM, Casey Schaufler wrote: >> On 9/23/2018 8:59 AM, Tetsuo Handa wrote: >>> On 2018/09/23 11:43, Kees Cook wrote: >>>>>> I'm excited about getting this landed! >>>>> Soon. Real soon. I hope. I would very much like for >>>>> someone from the SELinux camp to chime in, especially on >>>>> the selinux_is_enabled() removal. >>>> Agreed. >>>> >>> This patchset from Casey lands before the patchset from Kees, doesn't it? >> >> That is up for negotiation. We may end up combining them. >> >>> OK, a few comments (if I didn't overlook something). >>> >>> lsm_early_cred()/lsm_early_task() are called from only __init functions. >> >> True. >> >>> lsm_cred_alloc()/lsm_file_alloc() are called from only security/security.c . >> >> Also true. >> >>> lsm_early_inode() should be avoided because it is not appropriate to >>> call panic() when lsm_early_inode() is called after __init phase. >> >> You're correct. In fact, lsm_early_inode() isn't needed at all >> until multiple inode using modules are supported. >> >>> Since all free hooks are called when one of init hooks failed, each >>> free hook needs to check whether init hook was called. An example is >>> inode_free_security() in security/selinux/hooks.c (but not addressed in >>> this patch). >> >> I *think* that selinux_inode_free_security() is safe in this >> case because the blob will be zeroed, hence isec->list will >> be NULL. > > That's not safe - look more closely at what list_empty_careful() tests, and then think about what happens when list_del_init() gets called on that isec->list. selinux_inode_free_security() presumes that selinux_inode_alloc_security() has been called already. If you are breaking that assumption, you have to fix it. Yup. I misread the macro my first time around. Easy fix. > Is there a reason you can't make inode_alloc_security() return void since you moved the allocation to the framework? No reason with any of the existing modules, But I could see someone doing unnatural things during allocation that might result in a failure. > Unfortunate that inode_init_security name is already in use for another purpose since essentially you have reduced these hooks to initialization only. I considered that but decided that it makes more sense for the module hook names to match the infrastructure name. Having security_inode_alloc() call selinux_inode_setup_security() starts to get confusing.
On 9/23/2018 6:53 PM, Tetsuo Handa wrote: > On 2018/09/24 2:09, Casey Schaufler wrote: >>> Since all free hooks are called when one of init hooks failed, each >>> free hook needs to check whether init hook was called. An example is >>> inode_free_security() in security/selinux/hooks.c (but not addressed in >>> this patch). >> I *think* that selinux_inode_free_security() is safe in this >> case because the blob will be zeroed, hence isec->list will >> be NULL. >> > OK. > >>> This patchset might fatally prevent LKM-based LSM modules, for LKM-based >>> LSMs cannot count on lsm_*_alloc() because size for lsm_*_alloc() cannot >>> be updated upon loading LKM-based LSMs. >> LKM based security modules will require dynamically sized blobs. >> These can be added to the scheme used here. Each blob would get a >> header identifying the modules for which it contains data. When an >> LKM is registered if has to declare it's blob space requirements >> and gets back the offsets. All alloc operations have to put their >> marks in the header. All LKM blob users have to check that the blob >> they are looking at has the required data. >> >> module_cred(struct cred *cred) { >> return cred->security + module_blob_sizes.lbs_cred; >> } >> >> becomes >> >> module_cred(struct cred *cred) { >> if (blob_includes(module_id)) >> return cred->security + module_blob_sizes.lbs_cred; >> return NULL; >> } >> >> and the calling code needs to accept a NULL return. > Not all of LKM-based LSMs use security blobs. And some of LKM-based LSMs > might use security blobs for only a few objects. For example, AKARI uses > inode security blob for remembering whether source address/port of an > accept()ed socket was already checked, only during accept() operation and > first socket operation on the accept()ed socket. Thus, there is no need > to waste memory by assigning blobs for all inode objects. The first question is why use an inode blob? Shouldn't you be using a socket blob for this socket based information? If you only want information part of the time you can declare a pointer sized blob and manage what hangs off that as you will. I personally think that the added complexity of conditional blob management is more pain than it's worth, but if you want a really big blob, but only on occasion, I could see doing it. >> Blobs can never get smaller because readjusting the offsets >> isn't going to work, so unloading an LKM security module isn't >> going to be as complete as you might like. There may be a way >> around this if you unload all the LKM modules, but that's a >> special case and there may be dragon lurking in the mist. > If LKM-based LSMs who want to use security blobs have to check for > NULL return, they might choose "not using infrastructure managed > security blobs" and "using locally hashed blobs associated with > object's address" (like AKARI does). I can't see how a check for NULL could possibly be a bigger hassle than doing your own locally hashed blobs. > >>> If security_file_free() is called >>> regardless of whether lsm_file_cache is defined, LKM-based LSMs can be >>> loaded using current behavior (apart from the fact that legitimate >>> interface for appending to security_hook_heads is currently missing). >>> How do you plan to handle LKM-based LSMs? >> My position all along has been that I don't plan to handle LKM >> based LSMs, but that I won't do anything to prevent someone else >> from adding them later. I believe that I've done that. Several >> designs, including a separate list for dynamically loaded modules >> have been proposed. I think some of those would work. > Though AKARI is not using security_file_free(), some of LKM-based LSMs > might want to use it. If file_free_security hook is called unconditionally, > such LKM-based LSMs can be registered/unregistered, without worrying about > inability to shrink sizes for blobs. The infrastructure wouldn't call unregistered hooks, so any module that allocates additional memory attached to a blob is going to have to deal with freeing that when it unregisters. Aside from that unregistration should be a (not so) small matter of locking. > >>> @@ -1202,11 +1183,11 @@ void security_file_free(struct file *file) >>> { >>> void *blob; >>> >>> + call_void_hook(file_free_security, file); >>> + >>> if (!lsm_file_cache) >>> return; >>> >>> - call_void_hook(file_free_security, file); >>> - >> Why does this make sense? If the lsm_file_cache isn't >> initialized you can't have allocated any file blobs, >> no module can have initialized a file blob, hence there >> can be nothing for the module to do. >> > For modules (not limited to LKM-based LSMs) which want to use > file blobs for only a few objects and avoid wasting memory by > allocating file blobs to all file objects. > > Infrastructure based blob management fits well for LSM modules > which want to assign blobs to all objects (like SELinux). But > forcing infrastructure based blob management can become a huge > waste of memory for LSM modules which want to assign blobs to > only a few objects. Unconditionally calling file_free_security > hook (as with other hooks) preserves a room for allowing the > latter type of LSM modules without using infrastructure based > blob management. There is a hypothetical issue here, but that would require abuse of the infrastructure. Having a file_free_security hook that doesn't free a security blob allocated by file_alloc_security may coincidentaly be useful, but that's not the intent of the hook.
On 2018/09/25 1:15, Casey Schaufler wrote: >>>> Since all free hooks are called when one of init hooks failed, each >>>> free hook needs to check whether init hook was called. An example is >>>> inode_free_security() in security/selinux/hooks.c (but not addressed in >>>> this patch). >>> >>> I *think* that selinux_inode_free_security() is safe in this >>> case because the blob will be zeroed, hence isec->list will >>> be NULL. >> >> That's not safe - look more closely at what list_empty_careful() tests, and then think about what happens when list_del_init() gets called on that isec->list. selinux_inode_free_security() presumes that selinux_inode_alloc_security() has been called already. If you are breaking that assumption, you have to fix it. > > Yup. I misread the macro my first time around. Easy fix. Oh, I didn't notice that it is doing !list_empty_careful() than list_empty_careful(). Unsafe indeed. But easy to fix. > >> Is there a reason you can't make inode_alloc_security() return void since you moved the allocation to the framework? > > No reason with any of the existing modules, But I could see someone > doing unnatural things during allocation that might result in a > failure. Currently upstreamed LSM modules and AKARI would be OK. But I can't guarantee it for future / not-yet-upstreamed LSM modules.
On 2018/09/25 2:16, Casey Schaufler wrote: >> Not all of LKM-based LSMs use security blobs. And some of LKM-based LSMs >> might use security blobs for only a few objects. For example, AKARI uses >> inode security blob for remembering whether source address/port of an >> accept()ed socket was already checked, only during accept() operation and >> first socket operation on the accept()ed socket. Thus, there is no need >> to waste memory by assigning blobs for all inode objects. > > The first question is why use an inode blob? Shouldn't you > be using a socket blob for this socket based information? Indeed. AKARI can as well use security_sk_free() using address of "struct sock" as a key. > > If you only want information part of the time you can declare > a pointer sized blob and manage what hangs off that as you will. > I personally think that the added complexity of conditional > blob management is more pain than it's worth, but if you want > a really big blob, but only on occasion, I could see doing it. LKM based LSMs are too late for updating blob_sizes.* fields. Even if they could, they after all have to somehow check whether corresponding init hook was called. That's checking for NULL. >> >>>> @@ -1202,11 +1183,11 @@ void security_file_free(struct file *file) >>>> { >>>> void *blob; >>>> >>>> + call_void_hook(file_free_security, file); >>>> + >>>> if (!lsm_file_cache) >>>> return; >>>> >>>> - call_void_hook(file_free_security, file); >>>> - >>> Why does this make sense? If the lsm_file_cache isn't >>> initialized you can't have allocated any file blobs, >>> no module can have initialized a file blob, hence there >>> can be nothing for the module to do. >>> >> For modules (not limited to LKM-based LSMs) which want to use >> file blobs for only a few objects and avoid wasting memory by >> allocating file blobs to all file objects. >> >> Infrastructure based blob management fits well for LSM modules >> which want to assign blobs to all objects (like SELinux). But >> forcing infrastructure based blob management can become a huge >> waste of memory for LSM modules which want to assign blobs to >> only a few objects. Unconditionally calling file_free_security >> hook (as with other hooks) preserves a room for allowing the >> latter type of LSM modules without using infrastructure based >> blob management. > > There is a hypothetical issue here, but that would require abuse > of the infrastructure. Having a file_free_security hook that doesn't > free a security blob allocated by file_alloc_security may coincidentaly > be useful, but that's not the intent of the hook. > The free hook might be used for freeing resources which were not allocated by alloc hook. Yama is using task_free hook without task_alloc hook. Someone might want to use file_free hook without file_alloc hook.
On 9/24/2018 10:53 AM, Tetsuo Handa wrote: > On 2018/09/25 2:16, Casey Schaufler wrote: >>> Not all of LKM-based LSMs use security blobs. And some of LKM-based LSMs >>> might use security blobs for only a few objects. For example, AKARI uses >>> inode security blob for remembering whether source address/port of an >>> accept()ed socket was already checked, only during accept() operation and >>> first socket operation on the accept()ed socket. Thus, there is no need >>> to waste memory by assigning blobs for all inode objects. >> The first question is why use an inode blob? Shouldn't you >> be using a socket blob for this socket based information? > Indeed. AKARI can as well use security_sk_free() using address of > "struct sock" as a key. > >> If you only want information part of the time you can declare >> a pointer sized blob and manage what hangs off that as you will. >> I personally think that the added complexity of conditional >> blob management is more pain than it's worth, but if you want >> a really big blob, but only on occasion, I could see doing it. > LKM based LSMs are too late for updating blob_sizes.* fields. That is true with the code in this patch set. As I mentioned, changing the blob handling to include a header with real use information would be required. > Even if they could, they after all have to somehow check whether > corresponding init hook was called. That's checking for NULL. Right. >>>>> @@ -1202,11 +1183,11 @@ void security_file_free(struct file *file) >>>>> { >>>>> void *blob; >>>>> >>>>> + call_void_hook(file_free_security, file); >>>>> + >>>>> if (!lsm_file_cache) >>>>> return; >>>>> >>>>> - call_void_hook(file_free_security, file); >>>>> - >>>> Why does this make sense? If the lsm_file_cache isn't >>>> initialized you can't have allocated any file blobs, >>>> no module can have initialized a file blob, hence there >>>> can be nothing for the module to do. >>>> >>> For modules (not limited to LKM-based LSMs) which want to use >>> file blobs for only a few objects and avoid wasting memory by >>> allocating file blobs to all file objects. >>> >>> Infrastructure based blob management fits well for LSM modules >>> which want to assign blobs to all objects (like SELinux). But >>> forcing infrastructure based blob management can become a huge >>> waste of memory for LSM modules which want to assign blobs to >>> only a few objects. Unconditionally calling file_free_security >>> hook (as with other hooks) preserves a room for allowing the >>> latter type of LSM modules without using infrastructure based >>> blob management. >> There is a hypothetical issue here, but that would require abuse >> of the infrastructure. Having a file_free_security hook that doesn't >> free a security blob allocated by file_alloc_security may coincidentaly >> be useful, but that's not the intent of the hook. >> > The free hook might be used for freeing resources which were not allocated > by alloc hook. Yama is using task_free hook without task_alloc hook. > Someone might want to use file_free hook without file_alloc hook. OK, you're correct. Checking for an initialized kmem_cache isn't appropriate.
On Sun, 23 Sep 2018, Casey Schaufler wrote: > > How do you plan to handle LKM-based LSMs? > > My position all along has been that I don't plan to handle LKM > based LSMs, but that I won't do anything to prevent someone else > from adding them later. I believe that I've done that. Several > designs, including a separate list for dynamically loaded modules > have been proposed. I think some of those would work. Dynamically loadable LSMs are a bad idea, per several previous discussions. As a general design concept, kernel security mechanisms should be invoked during boot, so we can reason about the overall state of the system at a given point. In any case, we do not need to take dynamic LSMs into account at this stage. We don't build infrastructure for non-existent features.
v4: Finer granularity in the patches and other cleanups suggested by Kees Cook. Removed dead code created by the removal of SELinux credential blob poisoning. v3: Add ipc blob for SARA and task blob for Landlock. Removing the SELinux cred blob pointer poisoning results selinux_is_enabled() being unused, so it and all it's overhead has been removed. Broke up the cred infrastructure patch. v2: Reduce the patchset to what is required to support the proposed SARA and LandLock security modules The SARA security module is intended to be used in conjunction with other security modules. It requires state to be maintained for the credential, which in turn requires a mechanism for sharing the credential security blob. It also uses the ipc security blob. The module also requires mechanism for user space manipulation of the credential information, hence an additional subdirectory in /proc/.../attr. The LandLock security module provides user configurable policy in the secmark mechanism. It requires data in the credential, file, inode and task security blobs. For this to be used along side the existing "major" security modules mechanism for sharing these blobs are provided. A side effect of providing sharing of the crendential security blob is that the TOMOYO module can be used at the same time as the other "major" modules. The mechanism for configuring which security modules are enabled has to change when stacking in enabled. Any module that uses just the security blobs that are shared can be selected. Additionally, one other "major" module can be selected. The security module stacking issues around networking and IPC are not addressed here as they are beyond what is required for TOMOYO, SARA and LandLock. git://github.com/cschaufler/lsm-stacking.git#stacking-4.19-rc2-saralock-v4 Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- Documentation/admin-guide/LSM/index.rst | 23 +- fs/proc/base.c | 64 ++++- fs/proc/internal.h | 1 + include/linux/cred.h | 1 - include/linux/lsm_hooks.h | 24 +- include/linux/security.h | 15 +- include/linux/selinux.h | 35 --- kernel/cred.c | 13 - security/Kconfig | 92 +++++++ security/apparmor/domain.c | 2 +- security/apparmor/include/cred.h | 24 +- security/apparmor/include/file.h | 9 +- security/apparmor/include/lib.h | 4 + security/apparmor/include/task.h | 18 +- security/apparmor/lsm.c | 68 +++-- security/apparmor/task.c | 6 +- security/security.c | 438 ++++++++++++++++++++++++++++++-- security/selinux/Makefile | 2 +- security/selinux/exports.c | 23 -- security/selinux/hooks.c | 333 +++++++----------------- security/selinux/include/audit.h | 3 - security/selinux/include/objsec.h | 48 +++- security/selinux/selinuxfs.c | 4 +- security/selinux/ss/services.c | 1 - security/selinux/xfrm.c | 4 +- security/smack/smack.h | 55 +++- security/smack/smack_access.c | 4 +- security/smack/smack_lsm.c | 315 ++++++++--------------- security/smack/smackfs.c | 18 +- security/tomoyo/common.h | 26 +- security/tomoyo/domain.c | 4 +- security/tomoyo/securityfs_if.c | 15 +- security/tomoyo/tomoyo.c | 57 ++++- 33 files changed, 1098 insertions(+), 651 deletions(-)