LSM: Revive security_task_alloc() hook.

Commit Message

Tetsuo Handa Jan. 6, 2017, 11:35 a.m. UTC

Casey Schaufler wrote:
> On 1/4/2017 3:00 AM, Tetsuo Handa wrote:
> > We switched from "struct task_struct"->security to "struct cred"->security
> > in Linux 2.6.29. But not all LSM modules were happy with that change.
> > TOMOYO LSM module is an example which want to use per "struct task_struct"
> > security blob, for TOMOYO's security context is defined based on "struct
> > task_struct" rather than "struct cred". AppArmor LSM module is another
> > example which want to use it, for AppArmor is currently abusing the cred
> > a little bit to store the change_hat and setexeccon info. Although
> > security_task_free() hook was revived in Linux 3.4 because Yama LSM module
> > wanted to release per "struct task_struct" security blob,
> > security_task_alloc() hook and "struct task_struct"->security field were
> > not revived. Nowadays, we are getting proposals of lightweight LSM modules
> > which want to use per "struct task_struct" security blob. PTAGS LSM module
> > and CaitSith LSM module which are currently under proposal for inclusion
> > also want to use it. Therefore, it will be time to revive
> > security_task_alloc() hook.
> 
> I think you've made a pretty clear argument.
> 

Thank you.

> > We are already allowing multiple concurrent LSM modules (up to one fully
> > armored module which uses "struct cred"->security field or exclusive hooks
> > like security_xfrm_state_pol_flow_match(), plus unlimited number of
> > lightweight modules which do not use "struct cred"->security nor exclusive
> > hooks) as long as they are built into the kernel. Since multiple LSM
> > modules might want to use "struct task_struct"->security field, we need to
> > somehow calculate and allocate enough size for that field. Casey is trying
> > to calculate and allocate enough size for "struct cred"->security field,
> > but that approach is not applicable for LKM based LSM modules. On the other
> > hand, lightweight LSM modules (e.g. Yama) do not always allocate "struct
> > task_struct"->security field. If we tolerate managing per "struct
> > task_struct" security blobs outside of "struct task_struct", we don't need
> > to calculate and allocate enough size for "struct task_struct"->security
> > field, we can save memory by using address of "struct task_struct" as
> > a hash key for searching corresponding security blobs, and as a result
> > we can also allow LKM based LSM modules. Therefore, this patch does not
> > revive "struct task_struct"->security field.
> 
> While I agree with your conclusion that it's unnecessary to
> revive the security field in the task structure now, I think
> we are going to want it in the not too distant future. We can
> leave that for the first module that uses it, or we can start
> the inevitable fight with the owner of the task structure here.
> I also believe that the infrastructure managed allocation model
> can accommodate dynamic loading. I have not proposed that yet
> as it does complicate things somewhat, and the slope is already
> steep enough.
> 
> As for hashes and IDs, I hates 'em to pieces.

OK. I changed to an infrastructure managed allocation model.
Updated patch is attached in the bottom.

> > 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 failed. But security_task_alloc() unlikely fails, Yama is
> > safe to call task_free even if security blob was not allocated, and LKM
> > based LSM modules will anyway have to be prepared for possibility of
> > calling task_free without corresponding task_alloc call. Therefore,
> > this patch calls security_task_free() even if security_task_alloc() failed.
> 
> Yes, that is an implication of having a free hook where the
> alloc hook might fail or might never have been called. In the
> infrastructure managed case it would require some level of
> bookkeeping to ensure that the module code can determine if
> the blob includes data for that module. I have a design for
> doing that. I don't plan to propose it this time around, but
> I am sure that I'm doing nothing to preclude it.

By embedding a boolean flag into "struct foo_blob" for module foo which
tells whether "struct foo_blob" is initialized, foo can do bookkeeping.
Thus, always calling security_task_free() will be fine.

On 1/4/2017 4:21 AM, Jose Bollo wrote:
>> + * @task_alloc:
>> + *      @task task being allocated.
>> + *      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. Since the address of @task is guaranteed to remain
>> + *      unchanged between task_alloc call and task_free call, task_free users
>> + *      can use the address of @task for checking whether task_free is called
>> + *      without corresponding task_alloc call.
> Is it possible to add a comment on the state of the task: is it fully
> initialised? parly only? not initialised at all?

What does the state of @task mean? If you meant the state of security blob of
@task, it is initialized with 0 in the updated patch attached in the bottom.
If you really meant the state of @task, it is "partially initialized" because
security_task_alloc() is called from copy_process() which duplicates current
thread's "struct task_struct" and modifies it as @task.

>> @@ -1479,6 +1489,7 @@
>>       int (*file_open)(struct file *file, const struct cred *cred);
>>  
>>       int (*task_create)(unsigned long clone_flags);
>> +     int (*task_alloc)(struct task_struct *task);
> I suggest to add the 'clone_flags' as below
>
>      int (*task_alloc)(
>                     struct task_struct *task,
>                     unsigned long clone_flags);
>
> It would allow to treat CLONE_THREAD and/or CLONE_NEW... in a specific
> way.

OK. I added it. Now, I'm tempted to eliminate security_task_create() call.

Creating a new thread is unlikely prohibited by security policy, for
fork()/execve()/exit() is fundamental of how processes are managed in Unix.
If a program is known to create a new thread, it is likely that permission
to create a new thread is given to that program. Therefore, a situation
where security_task_create() returns an error is likely that the program was
exploited and lost control. Even if SELinux failed to check permission to
create a thread at security_task_create(), SELinux can later check it at
security_task_alloc(). Since the new thread is not yet visible from the rest
of the system, nobody can do bad things using the new thread. What we waste
will be limited to some initialization steps such as dup_task_struct(),
copy_creds() and audit_alloc() in copy_process(). I think we can tolerate
these overhead for unlikely situation. What do SELinux people think?

Casey Schaufler wrote:
> On 1/4/2017 3:00 AM, Tetsuo Handa wrote:
> >  include/linux/lsm_hooks.h | 14 +++++++++++++-
> >  include/linux/security.h  |  6 ++++++
> >  kernel/fork.c             |  4 ++++
> >  security/security.c       | 15 +++++++++++++++
> >  4 files changed, 38 insertions(+), 1 deletion(-)
> 
> I would expect to have at least one module that uses
> the revived hook included.

OK, but not in this post. If SELinux can use security_task_alloc()
instead of security_task_create(), SELinux will become one of modules
that use the revived hook. ;-)

Casey Schaufler wrote:
> On 1/4/2017 4:21 AM, Jose Bollo wrote:
> > Hashing is an interesting approach for low bandwidth requests.
> >
> > I was more thinking on a struct within task like below:
> >
> > #ifdef CONFIG_SECURITY
> >   struct {
> > #    ifdef CONFIG_SECURITY_APPARMOR
> >         void *apparmor;
> > #    endif
> > #    ifdef CONFIG_SECURITY_PTAGS
> >         void *ptags;
> > #    endif
> >   } security;
> > #endif
> >
> > It potential defines 2 "security" fields: security.apparmo and security.ptags
> 
> This is not going to be popular with distributions like Ubuntu
> that compile in all security modules. You're usually going to
> have wasted space in the task structure doing this.

Yes, this can become a problem for those who don't need it.

When commit be6d3e56a6b9b3a4 ("introduce new LSM hooks where vfsmount is
available.") was merged into Linux 2.6.29 in order to allow TOMOYO (which
was merged into Linux 2.6.30) to use "struct vfsmount *" argument for
calculating an absolute pathname of the file, CONFIG_SECURITY_PATH was
introduced because these hooks are needed by only TOMOYO. Although AppArmor
(which needs these hooks) was merged into Linux 2.6.36, CONFIG_SECURITY_PATH
was not removed because these hooks are considered as overhead for SELinux
and SMACK. Not calling unnecessary hooks and not wasting memory by unused
modules are important for those who don't need it.

In the updated patch attached in the bottom, I used simple array of
"unsigned long" where index == 0 remembers size of array and index > 0
are used by LSM modules. Modules which need small bytes occupy multiple
index numbers for sizeof("struct xxx") bytes, whereas modules which
need large bytes occupy only one index number for sizeof("struct yyy *")
bytes and allocate sizeof("struct yyy") bytes separately.

My question is whether SELinux and SMACK can tolerate always calling
security_task_alloc() hook and defining "struct task_struct"->t_security field
(which costs only sizeof(unsigned long *) bytes). Since Fedora/RHEL supports
only SELinux, my customers still cannot calculate absolute pathname when using
AKARI (an LKM based LSM module similar to TOMOYO) due to CONFIG_SECURITY_PATH=n.
If security_task_alloc() hook and "struct task_struct"->t_security field are
considered as overhead and enclosed with "#ifdef CONFIG_SECURITY_TASK" ...
"#endif" clause, the same thing will happen. ;-(

Below is an updated patch. It does not include changes for how to allocate
memory for initial thread's security blobs. We need to know how many bytes
needs to be allocated for initial thread'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(). This is a chicken-or-egg syndrome.
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 thread's security blob.
The third step is actually activate all modules which should be activated.
The simplest way is to call registration hooks in security_init() twice, once
for the first step, once more for the third step.

>From 6eeb52d5b4f8ed22531ee8150808305b103cfe92 Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <[e-mail]>
Date: Fri, 6 Jan 2017 19:57:21 +0900
Subject: [PATCH] LSM: Revive security_task_alloc() hook and per "struct
 task_struct" security blob.

We switched from "struct task_struct"->security to "struct cred"->security
in Linux 2.6.29. But not all LSM modules were happy with that change.
TOMOYO LSM module is an example which want to use per "struct task_struct"
security blob, for TOMOYO's security context is defined based on "struct
task_struct" rather than "struct cred". AppArmor LSM module is another
example which want to use it, for AppArmor is currently abusing the cred
a little bit to store the change_hat and setexeccon info. Although
security_task_free() hook was revived in Linux 3.4 because Yama LSM module
wanted to release per "struct task_struct" security blob,
security_task_alloc() hook and "struct task_struct"->security field were
not revived. Nowadays, we are getting proposals of lightweight LSM modules
which want to use per "struct task_struct" security blob. PTAGS LSM module
and CaitSith LSM module which are currently under proposal for inclusion
also want to use it. Therefore, it will be time to revive
security_task_alloc() hook and "struct task_struct"->security field.

We are already allowing multiple concurrent LSM modules (up to one fully
armored module which uses "struct cred"->security field or exclusive hooks
like security_xfrm_state_pol_flow_match(), plus unlimited number of
lightweight modules which do not use "struct cred"->security nor exclusive
hooks) as long as they are built into the kernel. Since multiple LSM
modules might want to use "struct task_struct"->security field, we need to
somehow calculate and allocate enough size for that field. Since it is
also possible that none of activated LSM modules uses that field, it is
important that we do not waste too much. Therefore, this patch implements
variable length "struct task_struct"->security field using array of
"unsigned long". By using array of unsigned long, only sizeof(unsigned
long *) bytes in "struct task_struct" will be wasted when none of activated
LSM modules uses that field.

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 failed. But security_task_alloc() unlikely fails, Yama is
safe to call task_free even if security blob was not allocated, and LKM
based LSM modules will anyway have to be prepared for possibility of
calling task_free without corresponding task_alloc call. Therefore,
this patch calls security_task_free() even if security_task_alloc() failed.

Pointer to per "struct task_struct" security blobs can be fetched using
task_security() function. The "&& index && *p >= (unsigned long) index"
check in task_security() is for now only for catching buggy built-in LSM
modules who registers after non-initial threads are created, but it will
serve as a mechianism for LKM based LSM modules to detect task_free call
without corresponding task_alloc call when LKM based LSM modules becomes
legal (e.g. __init attribute is dropped and a simple lock for serializing
module registration is added).

Signed-off-by: Tetsuo Handa <[e-mail]>
Cc: John Johansen <[e-mail]>
Cc: Paul Moore <[e-mail]>
Cc: Stephen Smalley <[e-mail]>
Cc: Eric Paris <[e-mail]>
Cc: Casey Schaufler <[e-mail]>
Cc: Kees Cook <[e-mail]>
Cc: James Morris <[e-mail]>
Cc: Serge E. Hallyn <[e-mail]>
Cc: Jose Bollo <[e-mail]>
---
 include/linux/init_task.h |  7 +++++++
 include/linux/lsm_hooks.h | 42 +++++++++++++++++++++++++++++++++++++++++-
 include/linux/sched.h     |  3 +++
 include/linux/security.h  |  7 +++++++
 kernel/fork.c             |  4 ++++
 security/security.c       | 33 +++++++++++++++++++++++++++++++++
 6 files changed, 95 insertions(+), 1 deletion(-)

Comments

John Johansen Jan. 7, 2017, 1:45 a.m. UTC | #1

On 01/06/2017 03:35 AM, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>> On 1/4/2017 3:00 AM, Tetsuo Handa wrote:
>>> We switched from "struct task_struct"->security to "struct cred"->security
>>> in Linux 2.6.29. But not all LSM modules were happy with that change.
>>> TOMOYO LSM module is an example which want to use per "struct task_struct"
>>> security blob, for TOMOYO's security context is defined based on "struct
>>> task_struct" rather than "struct cred". AppArmor LSM module is another
>>> example which want to use it, for AppArmor is currently abusing the cred
>>> a little bit to store the change_hat and setexeccon info. Although
>>> security_task_free() hook was revived in Linux 3.4 because Yama LSM module
>>> wanted to release per "struct task_struct" security blob,
>>> security_task_alloc() hook and "struct task_struct"->security field were
>>> not revived. Nowadays, we are getting proposals of lightweight LSM modules
>>> which want to use per "struct task_struct" security blob. PTAGS LSM module
>>> and CaitSith LSM module which are currently under proposal for inclusion
>>> also want to use it. Therefore, it will be time to revive
>>> security_task_alloc() hook.
>>
>> I think you've made a pretty clear argument.
>>
> 
> Thank you.
> 
>>> We are already allowing multiple concurrent LSM modules (up to one fully
>>> armored module which uses "struct cred"->security field or exclusive hooks
>>> like security_xfrm_state_pol_flow_match(), plus unlimited number of
>>> lightweight modules which do not use "struct cred"->security nor exclusive
>>> hooks) as long as they are built into the kernel. Since multiple LSM
>>> modules might want to use "struct task_struct"->security field, we need to
>>> somehow calculate and allocate enough size for that field. Casey is trying
>>> to calculate and allocate enough size for "struct cred"->security field,
>>> but that approach is not applicable for LKM based LSM modules. On the other
>>> hand, lightweight LSM modules (e.g. Yama) do not always allocate "struct
>>> task_struct"->security field. If we tolerate managing per "struct
>>> task_struct" security blobs outside of "struct task_struct", we don't need
>>> to calculate and allocate enough size for "struct task_struct"->security
>>> field, we can save memory by using address of "struct task_struct" as
>>> a hash key for searching corresponding security blobs, and as a result
>>> we can also allow LKM based LSM modules. Therefore, this patch does not
>>> revive "struct task_struct"->security field.
>>
>> While I agree with your conclusion that it's unnecessary to
>> revive the security field in the task structure now, I think
>> we are going to want it in the not too distant future. We can
>> leave that for the first module that uses it, or we can start
>> the inevitable fight with the owner of the task structure here.
>> I also believe that the infrastructure managed allocation model
>> can accommodate dynamic loading. I have not proposed that yet
>> as it does complicate things somewhat, and the slope is already
>> steep enough.
>>
>> As for hashes and IDs, I hates 'em to pieces.
> 
> OK. I changed to an infrastructure managed allocation model.
> Updated patch is attached in the bottom.
> 
>>> 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 failed. But security_task_alloc() unlikely fails, Yama is
>>> safe to call task_free even if security blob was not allocated, and LKM
>>> based LSM modules will anyway have to be prepared for possibility of
>>> calling task_free without corresponding task_alloc call. Therefore,
>>> this patch calls security_task_free() even if security_task_alloc() failed.
>>
>> Yes, that is an implication of having a free hook where the
>> alloc hook might fail or might never have been called. In the
>> infrastructure managed case it would require some level of
>> bookkeeping to ensure that the module code can determine if
>> the blob includes data for that module. I have a design for
>> doing that. I don't plan to propose it this time around, but
>> I am sure that I'm doing nothing to preclude it.
> 
> By embedding a boolean flag into "struct foo_blob" for module foo which
> tells whether "struct foo_blob" is initialized, foo can do bookkeeping.
> Thus, always calling security_task_free() will be fine.
> 
> On 1/4/2017 4:21 AM, Jose Bollo wrote:
>>> + * @task_alloc:
>>> + *      @task task being allocated.
>>> + *      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. Since the address of @task is guaranteed to remain
>>> + *      unchanged between task_alloc call and task_free call, task_free users
>>> + *      can use the address of @task for checking whether task_free is called
>>> + *      without corresponding task_alloc call.
>> Is it possible to add a comment on the state of the task: is it fully
>> initialised? parly only? not initialised at all?
> 
> What does the state of @task mean? If you meant the state of security blob of
> @task, it is initialized with 0 in the updated patch attached in the bottom.
> If you really meant the state of @task, it is "partially initialized" because
> security_task_alloc() is called from copy_process() which duplicates current
> thread's "struct task_struct" and modifies it as @task.
> 
>>> @@ -1479,6 +1489,7 @@
>>>       int (*file_open)(struct file *file, const struct cred *cred);
>>>  
>>>       int (*task_create)(unsigned long clone_flags);
>>> +     int (*task_alloc)(struct task_struct *task);
>> I suggest to add the 'clone_flags' as below
>>
>>      int (*task_alloc)(
>>                     struct task_struct *task,
>>                     unsigned long clone_flags);
>>
>> It would allow to treat CLONE_THREAD and/or CLONE_NEW... in a specific
>> way.
> 
> OK. I added it. Now, I'm tempted to eliminate security_task_create() call.
> 
its certainly worth discussing, but it should be a follow on patch. Possibly
in the same series

> Creating a new thread is unlikely prohibited by security policy, for
> fork()/execve()/exit() is fundamental of how processes are managed in Unix.
> If a program is known to create a new thread, it is likely that permission
> to create a new thread is given to that program. Therefore, a situation
> where security_task_create() returns an error is likely that the program was
> exploited and lost control. Even if SELinux failed to check permission to
> create a thread at security_task_create(), SELinux can later check it at
> security_task_alloc(). Since the new thread is not yet visible from the rest
> of the system, nobody can do bad things using the new thread. What we waste
> will be limited to some initialization steps such as dup_task_struct(),
> copy_creds() and audit_alloc() in copy_process(). I think we can tolerate
> these overhead for unlikely situation. What do SELinux people think?
> 

> Casey Schaufler wrote:
>> On 1/4/2017 3:00 AM, Tetsuo Handa wrote:
>>>  include/linux/lsm_hooks.h | 14 +++++++++++++-
>>>  include/linux/security.h  |  6 ++++++
>>>  kernel/fork.c             |  4 ++++
>>>  security/security.c       | 15 +++++++++++++++
>>>  4 files changed, 38 insertions(+), 1 deletion(-)
>>
>> I would expect to have at least one module that uses
>> the revived hook included.
> 
> OK, but not in this post. If SELinux can use security_task_alloc()
> instead of security_task_create(), SELinux will become one of modules
> that use the revived hook. ;-)
> 
> Casey Schaufler wrote:
>> On 1/4/2017 4:21 AM, Jose Bollo wrote:
>>> Hashing is an interesting approach for low bandwidth requests.
>>>
>>> I was more thinking on a struct within task like below:
>>>
>>> #ifdef CONFIG_SECURITY
>>>   struct {
>>> #    ifdef CONFIG_SECURITY_APPARMOR
>>>         void *apparmor;
>>> #    endif
>>> #    ifdef CONFIG_SECURITY_PTAGS
>>>         void *ptags;
>>> #    endif
>>>   } security;
>>> #endif
>>>
>>> It potential defines 2 "security" fields: security.apparmo and security.ptags
>>
>> This is not going to be popular with distributions like Ubuntu
>> that compile in all security modules. You're usually going to
>> have wasted space in the task structure doing this.
> 
> Yes, this can become a problem for those who don't need it.
> 
> When commit be6d3e56a6b9b3a4 ("introduce new LSM hooks where vfsmount is
> available.") was merged into Linux 2.6.29 in order to allow TOMOYO (which
> was merged into Linux 2.6.30) to use "struct vfsmount *" argument for
> calculating an absolute pathname of the file, CONFIG_SECURITY_PATH was
> introduced because these hooks are needed by only TOMOYO. Although AppArmor
> (which needs these hooks) was merged into Linux 2.6.36, CONFIG_SECURITY_PATH
> was not removed because these hooks are considered as overhead for SELinux
> and SMACK. Not calling unnecessary hooks and not wasting memory by unused
> modules are important for those who don't need it.
> 
> In the updated patch attached in the bottom, I used simple array of
> "unsigned long" where index == 0 remembers size of array and index > 0
> are used by LSM modules. Modules which need small bytes occupy multiple
> index numbers for sizeof("struct xxx") bytes, whereas modules which
> need large bytes occupy only one index number for sizeof("struct yyy *")
> bytes and allocate sizeof("struct yyy") bytes separately.
> 
> My question is whether SELinux and SMACK can tolerate always calling
> security_task_alloc() hook and defining "struct task_struct"->t_security field
> (which costs only sizeof(unsigned long *) bytes). Since Fedora/RHEL supports
> only SELinux, my customers still cannot calculate absolute pathname when using
> AKARI (an LKM based LSM module similar to TOMOYO) due to CONFIG_SECURITY_PATH=n.
> If security_task_alloc() hook and "struct task_struct"->t_security field are
> considered as overhead and enclosed with "#ifdef CONFIG_SECURITY_TASK" ...
> "#endif" clause, the same thing will happen. ;-(
> 
> Below is an updated patch. It does not include changes for how to allocate
> memory for initial thread's security blobs. We need to know how many bytes
> needs to be allocated for initial thread'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(). This is a chicken-or-egg syndrome.
> 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 thread's security blob.
> The third step is actually activate all modules which should be activated.
> The simplest way is to call registration hooks in security_init() twice, once
> for the first step, once more for the third step.
> 
>>From 6eeb52d5b4f8ed22531ee8150808305b103cfe92 Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <[e-mail]>
> Date: Fri, 6 Jan 2017 19:57:21 +0900
> Subject: [PATCH] LSM: Revive security_task_alloc() hook and per "struct
>  task_struct" security blob.
> 
> We switched from "struct task_struct"->security to "struct cred"->security
> in Linux 2.6.29. But not all LSM modules were happy with that change.
> TOMOYO LSM module is an example which want to use per "struct task_struct"
> security blob, for TOMOYO's security context is defined based on "struct
> task_struct" rather than "struct cred". AppArmor LSM module is another
> example which want to use it, for AppArmor is currently abusing the cred
> a little bit to store the change_hat and setexeccon info. Although
> security_task_free() hook was revived in Linux 3.4 because Yama LSM module
> wanted to release per "struct task_struct" security blob,
> security_task_alloc() hook and "struct task_struct"->security field were
> not revived. Nowadays, we are getting proposals of lightweight LSM modules
> which want to use per "struct task_struct" security blob. PTAGS LSM module
> and CaitSith LSM module which are currently under proposal for inclusion
> also want to use it. Therefore, it will be time to revive
> security_task_alloc() hook and "struct task_struct"->security field.
> 
> We are already allowing multiple concurrent LSM modules (up to one fully
> armored module which uses "struct cred"->security field or exclusive hooks
> like security_xfrm_state_pol_flow_match(), plus unlimited number of
> lightweight modules which do not use "struct cred"->security nor exclusive
> hooks) as long as they are built into the kernel. Since multiple LSM
> modules might want to use "struct task_struct"->security field, we need to
> somehow calculate and allocate enough size for that field. Since it is
> also possible that none of activated LSM modules uses that field, it is
> important that we do not waste too much. Therefore, this patch implements
> variable length "struct task_struct"->security field using array of
> "unsigned long". By using array of unsigned long, only sizeof(unsigned
> long *) bytes in "struct task_struct" will be wasted when none of activated
> LSM modules uses that field.
> 

I get why you want the multiple LSM module access, but I think for a first
pass it would be better to just take the same approach as cred->security
and let a single LSM own it.

That way the disussion about getting the t_security field and how to
share it can be separated, and those who are only managing/interested
in the task_struct management portion don't need to be bored with the
LSM infrastructure part.

> 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 failed. But security_task_alloc() unlikely fails, Yama is
> safe to call task_free even if security blob was not allocated, and LKM
> based LSM modules will anyway have to be prepared for possibility of
> calling task_free without corresponding task_alloc call. Therefore,
> this patch calls security_task_free() even if security_task_alloc() failed.
> 
> Pointer to per "struct task_struct" security blobs can be fetched using
> task_security() function. The "&& index && *p >= (unsigned long) index"
> check in task_security() is for now only for catching buggy built-in LSM
> modules who registers after non-initial threads are created, but it will
> serve as a mechianism for LKM based LSM modules to detect task_free call
> without corresponding task_alloc call when LKM based LSM modules becomes
> legal (e.g. __init attribute is dropped and a simple lock for serializing
> module registration is added).
> 
LKM based LSM modules is a completely separate discussion as they are
currently not supported, and any mention of them should be removed from
the patch.

Discussion of them can be added when/if they become supported

> Signed-off-by: Tetsuo Handa <[e-mail]>
> Cc: John Johansen <[e-mail]>
> Cc: Paul Moore <[e-mail]>
> Cc: Stephen Smalley <[e-mail]>
> Cc: Eric Paris <[e-mail]>
> Cc: Casey Schaufler <[e-mail]>
> Cc: Kees Cook <[e-mail]>
> Cc: James Morris <[e-mail]>
> Cc: Serge E. Hallyn <[e-mail]>
> Cc: Jose Bollo <[e-mail]>
> ---
>  include/linux/init_task.h |  7 +++++++
>  include/linux/lsm_hooks.h | 42 +++++++++++++++++++++++++++++++++++++++++-
>  include/linux/sched.h     |  3 +++
>  include/linux/security.h  |  7 +++++++
>  kernel/fork.c             |  4 ++++
>  security/security.c       | 33 +++++++++++++++++++++++++++++++++
>  6 files changed, 95 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> index 325f649..9bd0c83 100644
> --- a/include/linux/init_task.h
> +++ b/include/linux/init_task.h
> @@ -193,6 +193,12 @@
>  # define INIT_TASK_TI(tsk)
>  #endif
>  
> +#ifdef CONFIG_SECURITY
> +#define INIT_TASK_SECURITY .t_security = NULL,
> +#else
> +#define INIT_TASK_SECURITY
> +#endif
> +
>  /*
>   *  INIT_TASK is used to set up the first task table, touch at
>   * your own risk!. Base=0, limit=0x1fffff (=2MB)
> @@ -271,6 +277,7 @@
>  	INIT_VTIME(tsk)							\
>  	INIT_NUMA_BALANCING(tsk)					\
>  	INIT_KASAN(tsk)							\
> +	INIT_TASK_SECURITY						\
>  }
>  
>  
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 0dde959..daabf73 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -534,8 +534,16 @@
>   *	manual page for definitions of the @clone_flags.
>   *	@clone_flags contains the flags indicating what should be shared.
>   *	Return 0 if permission is granted.
> + * @task_alloc:
> + *      @task task being allocated.
> + *      @clone_flags: contains the flags indicating what should be shared.

drop the trailing : from @clone_flags to match the doc style

> + *      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 being freed
> + *	@task task about to be freed.
>   *	Handle release of task-related resources. (Note that this can be called
>   *	from interrupt context.)
>   * @cred_alloc_blank:
> @@ -1479,6 +1487,7 @@
>  	int (*file_open)(struct file *file, const struct cred *cred);
>  
>  	int (*task_create)(unsigned long clone_flags);
> +	int (*task_alloc)(struct task_struct *task, unsigned long clone_flags);
>  	void (*task_free)(struct task_struct *task);
>  	int (*cred_alloc_blank)(struct cred *cred, gfp_t gfp);
>  	void (*cred_free)(struct cred *cred);
> @@ -1744,6 +1753,7 @@ struct security_hook_heads {
>  	struct list_head file_receive;
>  	struct list_head file_open;
>  	struct list_head task_create;
> +	struct list_head task_alloc;
>  	struct list_head task_free;
>  	struct list_head cred_alloc_blank;
>  	struct list_head cred_free;
> @@ -1933,4 +1943,34 @@ static inline void __init yama_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 before calling security_add_hooks(). If reservation succeeds,
> + * security_reserve_task_blob_index() returns a postive number which has to be
> + * passed to task_security() by that user when fetching security blob of given
> + * "struct task_struct" for that user. If reservation fails,
> + * security_reserve_task_blob_index() returns 0.
> + *
> + * 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 task_security() can
> + * return NULL if security_task_free() is called due to kcalloc() failure in
> + * security_task_alloc().
> + *
> + * 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->t_security;
> +
> +	return p && index && *p >= (unsigned long) index ? p + index : NULL;
> +}
> +
>  #endif /* ! __LINUX_LSM_HOOKS_H */
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 1531c48..7441d06 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1988,6 +1988,9 @@ struct task_struct {
>  	/* A live task holds one reference. */
>  	atomic_t stack_refcount;
>  #endif
> +#ifdef CONFIG_SECURITY
> +	unsigned long *t_security;
> +#endif
>  /* CPU-specific state of this task */
>  	struct thread_struct thread;
>  /*
> diff --git a/include/linux/security.h b/include/linux/security.h
> index f4ebac1..9135f03 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -305,6 +305,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
>  int security_file_receive(struct file *file);
>  int security_file_open(struct file *file, const struct cred *cred);
>  int security_task_create(unsigned long clone_flags);
> +int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
>  void security_task_free(struct task_struct *task);
>  int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
>  void security_cred_free(struct cred *cred);
> @@ -857,6 +858,12 @@ static inline int security_task_create(unsigned long clone_flags)
>  	return 0;
>  }
>  
> +static inline int security_task_alloc(struct task_struct *task,
> +				      unsigned long clone_flags)
> +{
> +	return 0;
> +}
> +
>  static inline void security_task_free(struct task_struct *task)
>  { }
>  
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 11c5c8a..492d638 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1643,6 +1643,9 @@ static __latent_entropy struct task_struct *copy_process(
>  		goto bad_fork_cleanup_perf;
>  	/* copy all the process information */
>  	shm_init_task(p);
> +	retval = security_task_alloc(p, clone_flags);
> +	if (retval)
> +		goto bad_fork_cleanup_audit;
>  	retval = copy_semundo(clone_flags, p);
>  	if (retval)
>  		goto bad_fork_cleanup_audit;
> @@ -1862,6 +1865,7 @@ static __latent_entropy struct task_struct *copy_process(
>  	exit_sem(p);
>  bad_fork_cleanup_audit:
>  	audit_free(p);
> +	security_task_free(p);
>  bad_fork_cleanup_perf:
>  	perf_event_free_task(p);
>  bad_fork_cleanup_policy:
> diff --git a/security/security.c b/security/security.c
> index 32052f5..9519bcc 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -78,6 +78,20 @@ static int __init choose_lsm(char *str)
>  }
>  __setup("security=", choose_lsm);
>  
> +static u16 lsm_max_per_task_blob_index;
> +
> +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));
> +
> +	if (requested) {
> +		lsm_max_per_task_blob_index += requested;
> +		return index + 1;
> +	}
> +	return 0;
> +}
> +
>  /**
>   * security_module_enable - Load given security module on boot ?
>   * @module: the name of the module
> @@ -892,9 +906,27 @@ int security_task_create(unsigned long clone_flags)
>  	return call_int_hook(task_create, 0, clone_flags);
>  }
>  
> +int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
> +{
> +	const unsigned long index = lsm_max_per_task_blob_index;
> +
> +	if (index) {
> +		task->t_security = kcalloc(index + 1, sizeof(unsigned long),
> +					   GFP_KERNEL);
> +		if (!task->t_security)
> +			return -ENOMEM;
> +		*task->t_security = index + 1;
> +	} else {
> +		task->t_security = NULL;
> +	}
> +	return call_int_hook(task_alloc, 0, task, clone_flags);
> +}
> +
>  void security_task_free(struct task_struct *task)
>  {
>  	call_void_hook(task_free, task);
> +	if (lsm_max_per_task_blob_index)
> +		kfree(task->t_security);
>  }
>  
>  int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
> @@ -1731,6 +1763,7 @@ struct security_hook_heads security_hook_heads = {
>  	.file_receive =	LIST_HEAD_INIT(security_hook_heads.file_receive),
>  	.file_open =	LIST_HEAD_INIT(security_hook_heads.file_open),
>  	.task_create =	LIST_HEAD_INIT(security_hook_heads.task_create),
> +	.task_alloc =	LIST_HEAD_INIT(security_hook_heads.task_alloc),
>  	.task_free =	LIST_HEAD_INIT(security_hook_heads.task_free),
>  	.cred_alloc_blank =
>  		LIST_HEAD_INIT(security_hook_heads.cred_alloc_blank),
> 

So after a quick scan I didn't see anything else. My issue really is
I think you are trying to do too much, and I would rather see this broken
down into some separate patches.

This being the minimal to get the task_alloc and t_security fields
reintroduced, and another to deal with LSM infrastructure questions,
though by the sounds of it, maybe the second one should wait until
we see what Casey has

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index 325f649..9bd0c83 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -193,6 +193,12 @@ 
 # define INIT_TASK_TI(tsk)
 #endif
 
+#ifdef CONFIG_SECURITY
+#define INIT_TASK_SECURITY .t_security = NULL,
+#else
+#define INIT_TASK_SECURITY
+#endif
+
 /*
  *  INIT_TASK is used to set up the first task table, touch at
  * your own risk!. Base=0, limit=0x1fffff (=2MB)
@@ -271,6 +277,7 @@ 
 	INIT_VTIME(tsk)							\
 	INIT_NUMA_BALANCING(tsk)					\
 	INIT_KASAN(tsk)							\
+	INIT_TASK_SECURITY						\
 }
 
 
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 0dde959..daabf73 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -534,8 +534,16 @@ 
  *	manual page for definitions of the @clone_flags.
  *	@clone_flags contains the flags indicating what should be shared.
  *	Return 0 if permission is granted.
+ * @task_alloc:
+ *      @task task being allocated.
+ *      @clone_flags: contains the flags indicating what should be shared.
+ *      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 being freed
+ *	@task task about to be freed.
  *	Handle release of task-related resources. (Note that this can be called
  *	from interrupt context.)
  * @cred_alloc_blank:
@@ -1479,6 +1487,7 @@ 
 	int (*file_open)(struct file *file, const struct cred *cred);
 
 	int (*task_create)(unsigned long clone_flags);
+	int (*task_alloc)(struct task_struct *task, unsigned long clone_flags);
 	void (*task_free)(struct task_struct *task);
 	int (*cred_alloc_blank)(struct cred *cred, gfp_t gfp);
 	void (*cred_free)(struct cred *cred);
@@ -1744,6 +1753,7 @@  struct security_hook_heads {
 	struct list_head file_receive;
 	struct list_head file_open;
 	struct list_head task_create;
+	struct list_head task_alloc;
 	struct list_head task_free;
 	struct list_head cred_alloc_blank;
 	struct list_head cred_free;
@@ -1933,4 +1943,34 @@  static inline void __init yama_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 before calling security_add_hooks(). If reservation succeeds,
+ * security_reserve_task_blob_index() returns a postive number which has to be
+ * passed to task_security() by that user when fetching security blob of given
+ * "struct task_struct" for that user. If reservation fails,
+ * security_reserve_task_blob_index() returns 0.
+ *
+ * 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 task_security() can
+ * return NULL if security_task_free() is called due to kcalloc() failure in
+ * security_task_alloc().
+ *
+ * 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->t_security;
+
+	return p && index && *p >= (unsigned long) index ? p + index : NULL;
+}
+
 #endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 1531c48..7441d06 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1988,6 +1988,9 @@  struct task_struct {
 	/* A live task holds one reference. */
 	atomic_t stack_refcount;
 #endif
+#ifdef CONFIG_SECURITY
+	unsigned long *t_security;
+#endif
 /* CPU-specific state of this task */
 	struct thread_struct thread;
 /*
diff --git a/include/linux/security.h b/include/linux/security.h
index f4ebac1..9135f03 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -305,6 +305,7 @@  int security_file_send_sigiotask(struct task_struct *tsk,
 int security_file_receive(struct file *file);
 int security_file_open(struct file *file, const struct cred *cred);
 int security_task_create(unsigned long clone_flags);
+int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
 void security_task_free(struct task_struct *task);
 int security_cred_alloc_blank(struct cred *cred, gfp_t gfp);
 void security_cred_free(struct cred *cred);
@@ -857,6 +858,12 @@  static inline int security_task_create(unsigned long clone_flags)
 	return 0;
 }
 
+static inline int security_task_alloc(struct task_struct *task,
+				      unsigned long clone_flags)
+{
+	return 0;
+}
+
 static inline void security_task_free(struct task_struct *task)
 { }
 
diff --git a/kernel/fork.c b/kernel/fork.c
index 11c5c8a..492d638 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1643,6 +1643,9 @@  static __latent_entropy struct task_struct *copy_process(
 		goto bad_fork_cleanup_perf;
 	/* copy all the process information */
 	shm_init_task(p);
+	retval = security_task_alloc(p, clone_flags);
+	if (retval)
+		goto bad_fork_cleanup_audit;
 	retval = copy_semundo(clone_flags, p);
 	if (retval)
 		goto bad_fork_cleanup_audit;
@@ -1862,6 +1865,7 @@  static __latent_entropy struct task_struct *copy_process(
 	exit_sem(p);
 bad_fork_cleanup_audit:
 	audit_free(p);
+	security_task_free(p);
 bad_fork_cleanup_perf:
 	perf_event_free_task(p);
 bad_fork_cleanup_policy:
diff --git a/security/security.c b/security/security.c
index 32052f5..9519bcc 100644
--- a/security/security.c
+++ b/security/security.c
@@ -78,6 +78,20 @@  static int __init choose_lsm(char *str)
 }
 __setup("security=", choose_lsm);
 
+static u16 lsm_max_per_task_blob_index;
+
+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));
+
+	if (requested) {
+		lsm_max_per_task_blob_index += requested;
+		return index + 1;
+	}
+	return 0;
+}
+
 /**
  * security_module_enable - Load given security module on boot ?
  * @module: the name of the module
@@ -892,9 +906,27 @@  int security_task_create(unsigned long clone_flags)
 	return call_int_hook(task_create, 0, clone_flags);
 }
 
+int security_task_alloc(struct task_struct *task, unsigned long clone_flags)
+{
+	const unsigned long index = lsm_max_per_task_blob_index;
+
+	if (index) {
+		task->t_security = kcalloc(index + 1, sizeof(unsigned long),
+					   GFP_KERNEL);
+		if (!task->t_security)
+			return -ENOMEM;
+		*task->t_security = index + 1;
+	} else {
+		task->t_security = NULL;
+	}
+	return call_int_hook(task_alloc, 0, task, clone_flags);
+}
+
 void security_task_free(struct task_struct *task)
 {
 	call_void_hook(task_free, task);
+	if (lsm_max_per_task_blob_index)
+		kfree(task->t_security);
 }
 
 int security_cred_alloc_blank(struct cred *cred, gfp_t gfp)
@@ -1731,6 +1763,7 @@  struct security_hook_heads security_hook_heads = {
 	.file_receive =	LIST_HEAD_INIT(security_hook_heads.file_receive),
 	.file_open =	LIST_HEAD_INIT(security_hook_heads.file_open),
 	.task_create =	LIST_HEAD_INIT(security_hook_heads.task_create),
+	.task_alloc =	LIST_HEAD_INIT(security_hook_heads.task_alloc),
 	.task_free =	LIST_HEAD_INIT(security_hook_heads.task_free),
 	.cred_alloc_blank =
 		LIST_HEAD_INIT(security_hook_heads.cred_alloc_blank),