diff mbox

[RFC,v2,1/3] LSM: Allow per LSM module per "struct task_struct" blob.

Message ID 1491734530-25002-2-git-send-email-tixxdz@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Djalal Harouni April 9, 2017, 10:42 a.m. UTC
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>
---
 include/linux/lsm_hooks.h | 36 +++++++++++++++++++++++++++++++++++-
 security/security.c       | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 71 insertions(+), 2 deletions(-)

Comments

Casey Schaufler April 10, 2017, 3:50 p.m. UTC | #1
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
Djalal Harouni April 10, 2017, 6:30 p.m. UTC | #2
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!
Casey Schaufler April 10, 2017, 7:26 p.m. UTC | #3
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
Djalal Harouni April 10, 2017, 8 p.m. UTC | #4
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!
Kees Cook April 11, 2017, 4:43 a.m. UTC | #5
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
Casey Schaufler April 11, 2017, 7:54 p.m. UTC | #6
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
Kees Cook April 11, 2017, 7:57 p.m. UTC | #7
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
Djalal Harouni April 12, 2017, 4:08 p.m. UTC | #8
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!
Djalal Harouni April 12, 2017, 4:22 p.m. UTC | #9
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!
Casey Schaufler April 12, 2017, 8:41 p.m. UTC | #10
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 mbox

Patch

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)