| Message ID | 1491734530-25002-2-git-send-email-tixxdz@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 4/9/2017 3:42 AM, Djalal Harouni wrote: > From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > > Since several modules are planning to use per "struct task_struct" blob, > we need a layer for isolating it. Therefore, this patch introduces per LSM > module per "struct task_struct" blob. > > It would be possible to remember location in security_hook_heads.task_alloc > list and undo up to the corresponding security_hook_heads.task_free list > when task_alloc hook failed. But this patch calls all task_free hooks > without checking whether the corresponding task_alloc hook was called > since most modules should be able to handle this correctly. > > How to calculate amount of needed bytes and allocate memory for initial > task is a chicken-or-egg syndrome. We need to know how many bytes needs > to be allocated for initial task's security blobs before security_init() > is called. But security_reserve_task_blob_index() which calculates amount > of needed bytes is called from security_init(). We will need to split > module registration into three steps. The first step is call > security_reserve_task_blob_index() on all modules which should be > activated. The second step is allocate memory for the initial task's > security blob. The third step is actually activate all modules which > should be activated. > > Signed-off-by: Djalal Harouni <tixxdz@gmail.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> While I appreciate your enthusiasm, I would really like to see a mechanism for managing all of the blobs as being potentially shared rather than just the task blob. With infrastructure blob management being the general case we could stack any set of existing modules that does not include both SELinux and Smack. (AppArmor is threatening to join that set, but we're still waiting on that) I think it's a bad idea to go ahead with one implementation for task blobs without taking care of the others. > --- > include/linux/lsm_hooks.h | 36 +++++++++++++++++++++++++++++++++++- > security/security.c | 37 ++++++++++++++++++++++++++++++++++++- > 2 files changed, 71 insertions(+), 2 deletions(-) > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 080f34e..ca19cdb 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -27,6 +27,7 @@ > #include <linux/security.h> > #include <linux/init.h> > #include <linux/rculist.h> > +#include <linux/sched.h> /* "struct task_struct" */ > > /** > * Security hooks for program execution operations. > @@ -536,7 +537,10 @@ > * @task_alloc: > * @task task being allocated. > * @clone_flags contains the flags indicating what should be shared. > - * Handle allocation of task-related resources. > + * Handle allocation of task-related resources. Note that task_free is > + * called even if task_alloc failed. This means that all task_free users > + * have to be prepared for task_free being called without corresponding > + * task_alloc call. > * Returns a zero on success, negative values on failure. > * @task_free: > * @task task about to be freed. > @@ -1947,4 +1951,34 @@ void __init loadpin_add_hooks(void); > static inline void loadpin_add_hooks(void) { }; > #endif > > +/* > + * Per "struct task_struct" security blob is managed using index numbers. > + * > + * Any user who wants to use per "struct task_struct" security blob reserves an > + * index number using security_reserve_task_blob_index() before calling > + * security_add_hooks(). > + * > + * security_reserve_task_blob_index() returns an index number which has to be > + * passed to task_security() by that user when fetching security blob of given > + * "struct task_struct" for that user. > + * > + * Security blob for newly allocated "struct task_struct" is allocated and > + * initialized with 0 inside security_task_alloc(), before calling each user's > + * task_alloc hook. Be careful with task_free hook, for security_task_free() > + * can be called when calling each user's task_alloc hook failed. > + * > + * Note that security_reserve_task_blob_index() uses "u16". It is not a good > + * idea to directly reserve large amount. Sum of reserved amount by all users > + * should be less than PAGE_SIZE bytes, for per "struct task_struct" security > + * blob is allocated for each "struct task_struct" using kcalloc(). > + */ > +extern u16 __init security_reserve_task_blob_index(const u16 size); > +static inline void *task_security(const struct task_struct *task, > + const u16 index) > +{ > + unsigned long *p = task->security; > + > + return p + index; > +} > + > #endif /* ! __LINUX_LSM_HOOKS_H */ > diff --git a/security/security.c b/security/security.c > index 549bddc..4dc6bca 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 u16 lsm_max_per_task_blob_index __ro_after_init; > struct security_hook_heads security_hook_heads __lsm_ro_after_init; > char *lsm_names; > /* Boot-time LSM user choice */ > @@ -75,6 +76,15 @@ int __init security_init(void) > */ > do_security_initcalls(); > > + /* > + * Allocate per "struct task_struct" blob for initial task. > + * Initialization is done later by each module which needs it. > + */ > + if (lsm_max_per_task_blob_index) > + current->security = kcalloc(lsm_max_per_task_blob_index, > + sizeof(unsigned long), > + GFP_KERNEL | __GFP_NOFAIL); > + > return 0; > } > > @@ -86,6 +96,15 @@ static int __init choose_lsm(char *str) > } > __setup("security=", choose_lsm); > > +u16 __init security_reserve_task_blob_index(const u16 size) > +{ > + const u16 index = lsm_max_per_task_blob_index; > + u16 requested = DIV_ROUND_UP(size, sizeof(unsigned long)); > + > + lsm_max_per_task_blob_index += requested; > + return index; > +} > + > static int lsm_append(char *new, char **result) > { > char *cp; > @@ -939,12 +958,28 @@ int security_task_create(unsigned long clone_flags) > > int security_task_alloc(struct task_struct *task, unsigned long clone_flags) > { > - return call_int_hook(task_alloc, 0, task, clone_flags); > + int rc; > + const unsigned long index = lsm_max_per_task_blob_index; > + > + if (index) { > + task->security = kcalloc(index, sizeof(unsigned long), > + GFP_KERNEL); > + if (unlikely(!task->security)) > + return -ENOMEM; > + } > + rc = call_int_hook(task_alloc, 0, task, clone_flags); > + if (unlikely(rc)) > + security_task_free(task); > + return rc; > } > > void security_task_free(struct task_struct *task) > { > call_void_hook(task_free, task); > + if (lsm_max_per_task_blob_index) { > + kfree(task->security); > + task->security = NULL; > + } > } > > int security_cred_alloc_blank(struct cred *cred, gfp_t gfp) -- 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
On Mon, Apr 10, 2017 at 5:50 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 4/9/2017 3:42 AM, Djalal Harouni wrote: >> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> >> Since several modules are planning to use per "struct task_struct" blob, >> we need a layer for isolating it. Therefore, this patch introduces per LSM >> module per "struct task_struct" blob. >> >> It would be possible to remember location in security_hook_heads.task_alloc >> list and undo up to the corresponding security_hook_heads.task_free list >> when task_alloc hook failed. But this patch calls all task_free hooks >> without checking whether the corresponding task_alloc hook was called >> since most modules should be able to handle this correctly. >> >> How to calculate amount of needed bytes and allocate memory for initial >> task is a chicken-or-egg syndrome. We need to know how many bytes needs >> to be allocated for initial task's security blobs before security_init() >> is called. But security_reserve_task_blob_index() which calculates amount >> of needed bytes is called from security_init(). We will need to split >> module registration into three steps. The first step is call >> security_reserve_task_blob_index() on all modules which should be >> activated. The second step is allocate memory for the initial task's >> security blob. The third step is actually activate all modules which >> should be activated. >> >> Signed-off-by: Djalal Harouni <tixxdz@gmail.com> >> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > > While I appreciate your enthusiasm, I would really like > to see a mechanism for managing all of the blobs as being > potentially shared rather than just the task blob. With > infrastructure blob management being the general case we > could stack any set of existing modules that does not > include both SELinux and Smack. (AppArmor is threatening > to join that set, but we're still waiting on that) I Yes! most of the other kernel maintainers I discussed the feature with did not even believe or knew that LSM stacking is possible! Some other kernel frameworks are being replaced with new ones (I'm not sure if the old ones were perfect!)... but what I'm trying to say is that we should make it easy for dynamic LSM plugins model so they can explore the interface, and maybe after some time the whole framework will even be replaced with a better one... Thanks to your effort and others we may achieve it, but I don't think that delaying features for a perfect implementation is the best way. During linuxcon 2016 Europe you said that there should be a new kind of LSMs, that's why I think we should make it easy for that to happen. > think it's a bad idea to go ahead with one implementation > for task blobs without taking care of the others. Ok. Sorry if I missed this, but could you please point me why this could be a bad idea ? As I understand it, these are internal details that could be replaced. My first implementation used resizable concurrent hashtables that are heavily used in the networking code and it worked, and I easily converted the module to use Tetsuo's approach since I didn't see any objection for it, and it continued to work. Maybe the point should be *which* LSM should use the task->security and how ? The ModAutoRestrict module is a simple LSM which could easily work with any provided solution. That being said, I could convert the module back and stick with rhashtables for now since it does not conflict with any module and it works well. I would like to avoid same history where Apparmor, Smack and TOMOYO all suffered to get mainlined, even Yama due to some requests... Then I can convert the module back to use the same LSM infrastructure if you maintainers think that how it should go, that's totally fine by me. Yama internally could use the same task blob but it is avoiding conflicts by using lists to manage its internal data, that's the same design with ModAutoRestrict and rhashtables. Thank you for the comment!
On 4/10/2017 11:30 AM, Djalal Harouni wrote: > On Mon, Apr 10, 2017 at 5:50 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >> On 4/9/2017 3:42 AM, Djalal Harouni wrote: >>> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >>> >>> Since several modules are planning to use per "struct task_struct" blob, >>> we need a layer for isolating it. Therefore, this patch introduces per LSM >>> module per "struct task_struct" blob. >>> >>> It would be possible to remember location in security_hook_heads.task_alloc >>> list and undo up to the corresponding security_hook_heads.task_free list >>> when task_alloc hook failed. But this patch calls all task_free hooks >>> without checking whether the corresponding task_alloc hook was called >>> since most modules should be able to handle this correctly. >>> >>> How to calculate amount of needed bytes and allocate memory for initial >>> task is a chicken-or-egg syndrome. We need to know how many bytes needs >>> to be allocated for initial task's security blobs before security_init() >>> is called. But security_reserve_task_blob_index() which calculates amount >>> of needed bytes is called from security_init(). We will need to split >>> module registration into three steps. The first step is call >>> security_reserve_task_blob_index() on all modules which should be >>> activated. The second step is allocate memory for the initial task's >>> security blob. The third step is actually activate all modules which >>> should be activated. >>> >>> Signed-off-by: Djalal Harouni <tixxdz@gmail.com> >>> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> >> While I appreciate your enthusiasm, I would really like >> to see a mechanism for managing all of the blobs as being >> potentially shared rather than just the task blob. With >> infrastructure blob management being the general case we >> could stack any set of existing modules that does not >> include both SELinux and Smack. (AppArmor is threatening >> to join that set, but we're still waiting on that) I > Yes! most of the other kernel maintainers I discussed the feature with > did not even believe or knew that LSM stacking is possible! Some other > kernel frameworks are being replaced with new ones (I'm not sure if > the old ones were perfect!)... but what I'm trying to say is that we > should make it easy for dynamic LSM plugins model so they can explore > the interface, and maybe after some time the whole framework will even > be replaced with a better one... I'm not committing to dynamic modules, but I am working to make sure I don't do anything to prevent someone else from doing it in the future. There are a good number of people who find the notion of dynamic security policy disturbing. > Thanks to your effort and others we may achieve it, but I don't think > that delaying features for a perfect implementation is the best way. > During linuxcon 2016 Europe you said that there should be a new kind > of LSMs, that's why I think we should make it easy for that to happen. I agree that it's better to use a work-around today then to let the shortcomings of the existing infrastructure stop you from getting your job done. > >> think it's a bad idea to go ahead with one implementation >> for task blobs without taking care of the others. > Ok. Sorry if I missed this, but could you please point me why this > could be a bad idea ? It's a whole lot easier to maintain one sort of blob management then a different kind for each blob. It would be lots cleaner to have a single call that tells the infrastructure how much space you need for all your blobs than a separate interface for each. > As I understand it, these are internal details that could be replaced. > My first implementation used resizable concurrent hashtables that are > heavily used in the networking code and it worked, and I easily > converted the module to use Tetsuo's approach since I didn't see any > objection for it, and it continued to work. Maybe the point should be > *which* LSM should use the task->security and how ? The > ModAutoRestrict module is a simple LSM which could easily work with > any provided solution. So I see. > That being said, I could convert the module back and stick with > rhashtables for now since it does not conflict with any module and it > works well. I would like to avoid same history where Apparmor, Smack > and TOMOYO all suffered to get mainlined, even Yama due to some > requests... Then I can convert the module back to use the same LSM > infrastructure if you maintainers think that how it should go, that's > totally fine by me. Yama internally could use the same task blob but > it is avoiding conflicts by using lists to manage its internal data, > that's the same design with ModAutoRestrict and rhashtables. I think that would be the prudent approach. There is still the possibility that blob sharing (or full stacking, if you prefer) won't be accepted any time soon. > Thank you for the comment! > -- 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
On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 4/10/2017 11:30 AM, Djalal Harouni wrote: >> On Mon, Apr 10, 2017 at 5:50 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>> On 4/9/2017 3:42 AM, Djalal Harouni wrote: >>>> From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> [...] >>> While I appreciate your enthusiasm, I would really like >>> to see a mechanism for managing all of the blobs as being >>> potentially shared rather than just the task blob. With >>> infrastructure blob management being the general case we >>> could stack any set of existing modules that does not >>> include both SELinux and Smack. (AppArmor is threatening >>> to join that set, but we're still waiting on that) I >> Yes! most of the other kernel maintainers I discussed the feature with >> did not even believe or knew that LSM stacking is possible! Some other >> kernel frameworks are being replaced with new ones (I'm not sure if >> the old ones were perfect!)... but what I'm trying to say is that we >> should make it easy for dynamic LSM plugins model so they can explore >> the interface, and maybe after some time the whole framework will even >> be replaced with a better one... > > I'm not committing to dynamic modules, but I am working > to make sure I don't do anything to prevent someone else > from doing it in the future. There are a good number of > people who find the notion of dynamic security policy > disturbing. I understand, though that with today's container recycling use cases some of it may make sense. Anyway you are doing the real work, you know better. >> Thanks to your effort and others we may achieve it, but I don't think >> that delaying features for a perfect implementation is the best way. >> During linuxcon 2016 Europe you said that there should be a new kind >> of LSMs, that's why I think we should make it easy for that to happen. > > I agree that it's better to use a work-around today then to > let the shortcomings of the existing infrastructure stop you > from getting your job done. Indeed! >> >>> think it's a bad idea to go ahead with one implementation >>> for task blobs without taking care of the others. >> Ok. Sorry if I missed this, but could you please point me why this >> could be a bad idea ? > > It's a whole lot easier to maintain one sort of blob > management then a different kind for each blob. It would > be lots cleaner to have a single call that tells the > infrastructure how much space you need for all your blobs > than a separate interface for each. I perfectly agree here. With Tetsuo patch it was easy to add a stackable LSM, with a consistent approach that would be even better. But as you note that should not be a blocker to upstream new stackable LSMs. >> As I understand it, these are internal details that could be replaced. >> My first implementation used resizable concurrent hashtables that are >> heavily used in the networking code and it worked, and I easily >> converted the module to use Tetsuo's approach since I didn't see any >> objection for it, and it continued to work. Maybe the point should be >> *which* LSM should use the task->security and how ? The >> ModAutoRestrict module is a simple LSM which could easily work with >> any provided solution. > > So I see. > >> That being said, I could convert the module back and stick with >> rhashtables for now since it does not conflict with any module and it >> works well. I would like to avoid same history where Apparmor, Smack >> and TOMOYO all suffered to get mainlined, even Yama due to some >> requests... Then I can convert the module back to use the same LSM >> infrastructure if you maintainers think that how it should go, that's >> totally fine by me. Yama internally could use the same task blob but >> it is avoiding conflicts by using lists to manage its internal data, >> that's the same design with ModAutoRestrict and rhashtables. > > I think that would be the prudent approach. There is still > the possibility that blob sharing (or full stacking, if you > prefer) won't be accepted any time soon. Ok Casey! I will wait for more feedback, and if other maintainers do not object, I will convert it back to rhashtables in next iterations making sure that it should be simple to convert later to a blob sharing mechanism. Thanks again for the comments!
On Mon, Apr 10, 2017 at 1:00 PM, Djalal Harouni <tixxdz@gmail.com> wrote: > On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >> I think that would be the prudent approach. There is still >> the possibility that blob sharing (or full stacking, if you >> prefer) won't be accepted any time soon. > > Ok Casey! I will wait for more feedback, and if other maintainers do > not object, I will convert it back to rhashtables in next iterations > making sure that it should be simple to convert later to a blob > sharing mechanism. Would it be possible just to add a single field to task_struct if this LSM is built in? I feel like rhashtables is a huge overhead when a single field is all that's needed. -Kees
On 4/10/2017 9:43 PM, Kees Cook wrote: > On Mon, Apr 10, 2017 at 1:00 PM, Djalal Harouni <tixxdz@gmail.com> wrote: >> On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>> I think that would be the prudent approach. There is still >>> the possibility that blob sharing (or full stacking, if you >>> prefer) won't be accepted any time soon. >> Ok Casey! I will wait for more feedback, and if other maintainers do >> not object, I will convert it back to rhashtables in next iterations >> making sure that it should be simple to convert later to a blob >> sharing mechanism. > Would it be possible just to add a single field to task_struct if this > LSM is built in? I feel like rhashtables is a huge overhead when a > single field is all that's needed. Special casing the task_struct based on which modules are compiled in would work, but I'm under the impression that there's a strong desire to keep to one pointer for security module information in the major structures. The code for generalizing shared blobs isn't that hard, and y'all have seen it many times. It would be perfectly safe to convert the task, cred, inode and such blobs to be infrastructure managed right now. That wouldn't mean that all the stacking issues (e.g. audit and networking) would be addressed, or that all combinations of modules would work (i.e. no SELinux+Smack) but it would clear the way for this case. And Yama could use a blob if it wanted to. > > -Kees > -- 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
On Tue, Apr 11, 2017 at 12:54 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 4/10/2017 9:43 PM, Kees Cook wrote: >> On Mon, Apr 10, 2017 at 1:00 PM, Djalal Harouni <tixxdz@gmail.com> wrote: >>> On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> I think that would be the prudent approach. There is still >>>> the possibility that blob sharing (or full stacking, if you >>>> prefer) won't be accepted any time soon. >>> Ok Casey! I will wait for more feedback, and if other maintainers do >>> not object, I will convert it back to rhashtables in next iterations >>> making sure that it should be simple to convert later to a blob >>> sharing mechanism. >> Would it be possible just to add a single field to task_struct if this >> LSM is built in? I feel like rhashtables is a huge overhead when a >> single field is all that's needed. > > Special casing the task_struct based on which modules > are compiled in would work, but I'm under the impression > that there's a strong desire to keep to one pointer for > security module information in the major structures. Right, I meant as far as keeping this patch set unblocked by the other one... -Kees
On Tue, Apr 11, 2017 at 9:54 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: > On 4/10/2017 9:43 PM, Kees Cook wrote: >> On Mon, Apr 10, 2017 at 1:00 PM, Djalal Harouni <tixxdz@gmail.com> wrote: >>> On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> I think that would be the prudent approach. There is still >>>> the possibility that blob sharing (or full stacking, if you >>>> prefer) won't be accepted any time soon. >>> Ok Casey! I will wait for more feedback, and if other maintainers do >>> not object, I will convert it back to rhashtables in next iterations >>> making sure that it should be simple to convert later to a blob >>> sharing mechanism. >> Would it be possible just to add a single field to task_struct if this >> LSM is built in? I feel like rhashtables is a huge overhead when a >> single field is all that's needed. > > Special casing the task_struct based on which modules > are compiled in would work, but I'm under the impression > that there's a strong desire to keep to one pointer for > security module information in the major structures. > > The code for generalizing shared blobs isn't that hard, > and y'all have seen it many times. It would be perfectly > safe to convert the task, cred, inode and such blobs to > be infrastructure managed right now. That wouldn't mean > that all the stacking issues (e.g. audit and networking) > would be addressed, or that all combinations of modules > would work (i.e. no SELinux+Smack) but it would clear > the way for this case. And Yama could use a blob if it > wanted to. I've been thinking about this again, so my patches did not convert creds, inodes etc, I don't have a use case for them now. My use case was for task->security and I re-used the simple approach. I can't offer a solution for other blobs since I'm not familiar with their context nor the different use cases. I checked TOMOYO and that was easy, but I can't check all of them. I agreed that I may use rhashtables but as Kees pointed out this may introduce overheads and extra memory, where the task->security for this ModAutoProtect LSM will only require an extra sizeof(unsigned long) per-task. Also again the problem is not in this proposed Module, the problem is in modules that can't be stacked together. I am bringing this, since maybe after we manage to merge this, and if Kees agree I may send another set of patches for Yama to enable the same per-task context, this enables containers but also allows later in systemd-logind sessions to set it where all inferiors inside the user sessions are protected by default, not only some apps or special desktops but for all. So I will hit the same problem again where to put it? You said that the code that generalize the blobs isn't that hard, but also in a previous response that it may not be accepted... so I will try to converge the task->security blob to be more like the infrastructure that you are proposing, then resubmit and maybe we will enable these modules that are stackable by default. Is this reasonable ? Thanks!
On Tue, Apr 11, 2017 at 6:43 AM, Kees Cook <keescook@chromium.org> wrote: > On Mon, Apr 10, 2017 at 1:00 PM, Djalal Harouni <tixxdz@gmail.com> wrote: >> On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>> I think that would be the prudent approach. There is still >>> the possibility that blob sharing (or full stacking, if you >>> prefer) won't be accepted any time soon. >> >> Ok Casey! I will wait for more feedback, and if other maintainers do >> not object, I will convert it back to rhashtables in next iterations >> making sure that it should be simple to convert later to a blob >> sharing mechanism. > > Would it be possible just to add a single field to task_struct if this > LSM is built in? I feel like rhashtables is a huge overhead when a > single field is all that's needed. Well, yes rhashtables can have an overhead especially when reclaiming memory back, I could not identify a way how to separate tables unless we use cgroups as an ID. Anyway this of course could be added in task_struct and updated to work like the capability security hooks rather than a proper LSM with its own name. But as noted in the other response, we may need task->security field for Yama anyway. I'm open to suggestion ? I may try to converge the task->security blob with what Casey is proposing and see! otherwise fallback to task_struct as a last resort! Thanks!
On 4/12/2017 9:22 AM, Djalal Harouni wrote: > On Tue, Apr 11, 2017 at 6:43 AM, Kees Cook <keescook@chromium.org> wrote: >> On Mon, Apr 10, 2017 at 1:00 PM, Djalal Harouni <tixxdz@gmail.com> wrote: >>> On Mon, Apr 10, 2017 at 9:26 PM, Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> I think that would be the prudent approach. There is still >>>> the possibility that blob sharing (or full stacking, if you >>>> prefer) won't be accepted any time soon. >>> Ok Casey! I will wait for more feedback, and if other maintainers do >>> not object, I will convert it back to rhashtables in next iterations >>> making sure that it should be simple to convert later to a blob >>> sharing mechanism. >> Would it be possible just to add a single field to task_struct if this >> LSM is built in? I feel like rhashtables is a huge overhead when a >> single field is all that's needed. > Well, yes rhashtables can have an overhead especially when reclaiming > memory back, I could not identify a way how to separate tables unless > we use cgroups as an ID. Anyway this of course could be added in > task_struct and updated to work like the capability security hooks > rather than a proper LSM with its own name. But as noted in the other > response, we may need task->security field for Yama anyway. I'm open > to suggestion ? I may try to converge the task->security blob with > what Casey is proposing and see! otherwise fallback to task_struct as > a last resort! > > Thanks! I can present a patch set based on my existing stacking work that includes the move from module based memory management to infrastructure memory management but pretty well stops there. There will be changes to Kconfig to allow stacking of everything except SELinux and Smack, because that's the only combination with other conficts. Well, there's /proc/attr, but I'd include the module specific subdirectories, too. That would allow landlock to come in and TOMOYO (and potentially YAMA) to use/share the task blob. I see this as taking down a barrier rather than otherwise pointless infrastructure change. -- 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
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 080f34e..ca19cdb 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -27,6 +27,7 @@ #include <linux/security.h> #include <linux/init.h> #include <linux/rculist.h> +#include <linux/sched.h> /* "struct task_struct" */ /** * Security hooks for program execution operations. @@ -536,7 +537,10 @@ * @task_alloc: * @task task being allocated. * @clone_flags contains the flags indicating what should be shared. - * Handle allocation of task-related resources. + * Handle allocation of task-related resources. Note that task_free is + * called even if task_alloc failed. This means that all task_free users + * have to be prepared for task_free being called without corresponding + * task_alloc call. * Returns a zero on success, negative values on failure. * @task_free: * @task task about to be freed. @@ -1947,4 +1951,34 @@ void __init loadpin_add_hooks(void); static inline void loadpin_add_hooks(void) { }; #endif +/* + * Per "struct task_struct" security blob is managed using index numbers. + * + * Any user who wants to use per "struct task_struct" security blob reserves an + * index number using security_reserve_task_blob_index() before calling + * security_add_hooks(). + * + * security_reserve_task_blob_index() returns an index number which has to be + * passed to task_security() by that user when fetching security blob of given + * "struct task_struct" for that user. + * + * Security blob for newly allocated "struct task_struct" is allocated and + * initialized with 0 inside security_task_alloc(), before calling each user's + * task_alloc hook. Be careful with task_free hook, for security_task_free() + * can be called when calling each user's task_alloc hook failed. + * + * Note that security_reserve_task_blob_index() uses "u16". It is not a good + * idea to directly reserve large amount. Sum of reserved amount by all users + * should be less than PAGE_SIZE bytes, for per "struct task_struct" security + * blob is allocated for each "struct task_struct" using kcalloc(). + */ +extern u16 __init security_reserve_task_blob_index(const u16 size); +static inline void *task_security(const struct task_struct *task, + const u16 index) +{ + unsigned long *p = task->security; + + return p + index; +} + #endif /* ! __LINUX_LSM_HOOKS_H */ diff --git a/security/security.c b/security/security.c index 549bddc..4dc6bca 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 u16 lsm_max_per_task_blob_index __ro_after_init; struct security_hook_heads security_hook_heads __lsm_ro_after_init; char *lsm_names; /* Boot-time LSM user choice */ @@ -75,6 +76,15 @@ int __init security_init(void) */ do_security_initcalls(); + /* + * Allocate per "struct task_struct" blob for initial task. + * Initialization is done later by each module which needs it. + */ + if (lsm_max_per_task_blob_index) + current->security = kcalloc(lsm_max_per_task_blob_index, + sizeof(unsigned long), + GFP_KERNEL | __GFP_NOFAIL); + return 0; } @@ -86,6 +96,15 @@ static int __init choose_lsm(char *str) } __setup("security=", choose_lsm); +u16 __init security_reserve_task_blob_index(const u16 size) +{ + const u16 index = lsm_max_per_task_blob_index; + u16 requested = DIV_ROUND_UP(size, sizeof(unsigned long)); + + lsm_max_per_task_blob_index += requested; + return index; +} + static int lsm_append(char *new, char **result) { char *cp; @@ -939,12 +958,28 @@ int security_task_create(unsigned long clone_flags) int security_task_alloc(struct task_struct *task, unsigned long clone_flags) { - return call_int_hook(task_alloc, 0, task, clone_flags); + int rc; + const unsigned long index = lsm_max_per_task_blob_index; + + if (index) { + task->security = kcalloc(index, sizeof(unsigned long), + GFP_KERNEL); + if (unlikely(!task->security)) + return -ENOMEM; + } + rc = call_int_hook(task_alloc, 0, task, clone_flags); + if (unlikely(rc)) + security_task_free(task); + return rc; } void security_task_free(struct task_struct *task) { call_void_hook(task_free, task); + if (lsm_max_per_task_blob_index) { + kfree(task->security); + task->security = NULL; + } } int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)