LSM: Revive security_task_alloc() hook.

Commit Message

Tetsuo Handa Jan. 10, 2017, 10:58 a.m. UTC

Casey Schaufler wrote:
> 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.

John Johansen wrote:
> 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

I wanted to show how it will not be difficult to share per
"struct task_struct" security blob. Anyway, below is updated patch.

>From 387dabf2b2dde2e415d154249122e113f061345d Mon Sep 17 00:00:00 2001
From: Tetsuo Handa <[e-mail]>
Date: Tue, 10 Jan 2017 19:48:17 +0900
Subject: [PATCH 1/2] 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. But this patch does not
implement variable length "struct task_struct"->security field which will
become needed when multiple LSM modules want to use "struct task_struct"->
security field. Although it won't be difficult to implement variable length
"struct task_struct"->security field, let's think about it after we merged
this patch.

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 | 9 ++++++++-
 include/linux/sched.h     | 3 +++
 include/linux/security.h  | 7 +++++++
 kernel/fork.c             | 7 ++++++-
 security/security.c       | 6 ++++++
 6 files changed, 37 insertions(+), 2 deletions(-)

Comments

John Johansen Feb. 1, 2017, 11:15 a.m. UTC | #1

On 01/10/2017 02:58 AM, Tetsuo Handa wrote:
> Casey Schaufler wrote:
>> 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.
> 
> John Johansen wrote:
>> 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
> 
> I wanted to show how it will not be difficult to share per
> "struct task_struct" security blob. Anyway, below is updated patch.
> 
>>From 387dabf2b2dde2e415d154249122e113f061345d Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <[e-mail]>
> Date: Tue, 10 Jan 2017 19:48:17 +0900
> Subject: [PATCH 1/2] 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. But this patch does not
> implement variable length "struct task_struct"->security field which will
> become needed when multiple LSM modules want to use "struct task_struct"->
> security field. Although it won't be difficult to implement variable length
> "struct task_struct"->security field, let's think about it after we merged
> this patch.
> 
> 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]>

Sorry this took so long, it looks good to me, and I have done some builds
and tests with apparmor using it. The apparmor patch to make use of this
follows as a reply.

Acked-by: John Johansen <[e-mail]>




> ---
>  include/linux/init_task.h | 7 +++++++
>  include/linux/lsm_hooks.h | 9 ++++++++-
>  include/linux/sched.h     | 3 +++
>  include/linux/security.h  | 7 +++++++
>  kernel/fork.c             | 7 ++++++-
>  security/security.c       | 6 ++++++
>  6 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> index 325f649..6680e57 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 .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 9cf50ad..22f78e6 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -533,8 +533,13 @@
>   *	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.
> + *      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:
> @@ -1478,6 +1483,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 +1750,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;
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 4d19052..c733036 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
> +	void *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 c2125e9..ee1685f 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..ad55915 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1643,9 +1643,12 @@ static __latent_entropy struct task_struct *copy_process(
>  		goto bad_fork_cleanup_perf;
>  	/* copy all the process information */
>  	shm_init_task(p);
> -	retval = copy_semundo(clone_flags, 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_security;
>  	retval = copy_files(clone_flags, p);
>  	if (retval)
>  		goto bad_fork_cleanup_semundo;
> @@ -1860,6 +1863,8 @@ static __latent_entropy struct task_struct *copy_process(
>  	exit_files(p); /* blocking */
>  bad_fork_cleanup_semundo:
>  	exit_sem(p);
> +bad_fork_cleanup_security:
> +	security_task_free(p);
>  bad_fork_cleanup_audit:
>  	audit_free(p);
>  bad_fork_cleanup_perf:
> diff --git a/security/security.c b/security/security.c
> index f825304..246f844 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -892,6 +892,11 @@ 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)
> +{
> +	return call_int_hook(task_alloc, 0, task, clone_flags);
> +}
> +
>  void security_task_free(struct task_struct *task)
>  {
>  	call_void_hook(task_free, task);
> @@ -1731,6 +1736,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),
> 

--
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

James Morris Feb. 2, 2017, 4:02 a.m. UTC | #2

On Wed, 1 Feb 2017, John Johansen wrote:

> Sorry this took so long, it looks good to me, and I have done some builds
> and tests with apparmor using it. The apparmor patch to make use of this
> follows as a reply.
> 
> Acked-by: John Johansen <[e-mail]>

We're too late in the -rc cycle to take these for 4.11.  Please keep 
testing them and some more review/acks would also be good.


- James

John Johansen Feb. 2, 2017, 4:16 a.m. UTC | #3

On 02/01/2017 08:02 PM, James Morris wrote:
> On Wed, 1 Feb 2017, John Johansen wrote:
> 
>> Sorry this took so long, it looks good to me, and I have done some builds
>> and tests with apparmor using it. The apparmor patch to make use of this
>> follows as a reply.
>>
>> Acked-by: John Johansen <[e-mail]>
> 
> We're too late in the -rc cycle to take these for 4.11.  Please keep 
> testing them and some more review/acks would also be good.
> 
Certainly, I didn't expect this to land in 4.11. I would really like
get some feed back/review/ack from the owner of the task struct.

--
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

Tetsuo Handa Feb. 8, 2017, 1:21 p.m. UTC | #4

John Johansen wrote:
> On 02/01/2017 08:02 PM, James Morris wrote:
> > On Wed, 1 Feb 2017, John Johansen wrote:
> > 
> >> Sorry this took so long, it looks good to me, and I have done some builds
> >> and tests with apparmor using it. The apparmor patch to make use of this
> >> follows as a reply.
> >>
> >> Acked-by: John Johansen <[e-mail]>
> > 
> > We're too late in the -rc cycle to take these for 4.11.  Please keep 
> > testing them and some more review/acks would also be good.
> > 
> Certainly, I didn't expect this to land in 4.11. I would really like
> get some feed back/review/ack from the owner of the task struct.

What does "the owner of the task struct" mean? "struct task_struct" is
an object where fields are added/removed by individual subsystem. I think
there is no explicit owner to ask feed back/review/ack.

We added CONFIG_SECURITY_PATH in 2.6.29 and its user in 2.6.30.
I think applying this patch without users in 4.11-rc1 for trial is better.

TOMOYO can test this patch in 4.11-rcX if this patch is applied now.
AppArmor can test this patch in AppArmor's tree.
SELinux can apply security_task_create() => security_task_alloc() change
in SELinux's tree.
Casey can update "LSM: Stacking for major security modules - resend"
with this change included.
ptags and Timgad can evaluate this patch in their trees.

We can update this patch in 4.12-rc1 if it turned out that this patch
is insufficient for somebody.
--
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

Djalal Harouni Feb. 9, 2017, 9:31 a.m. UTC | #5

On Tue, Jan 10, 2017 at 11:58 AM, Tetsuo Handa
<[e-mail]> wrote:
> Casey Schaufler wrote:
>> 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.
>
> John Johansen wrote:
>> 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
>
> I wanted to show how it will not be difficult to share per
> "struct task_struct" security blob. Anyway, below is updated patch.
>
> >From 387dabf2b2dde2e415d154249122e113f061345d Mon Sep 17 00:00:00 2001
> From: Tetsuo Handa <[e-mail]>
> Date: Tue, 10 Jan 2017 19:48:17 +0900
> Subject: [PATCH 1/2] 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. But this patch does not
> implement variable length "struct task_struct"->security field which will
> become needed when multiple LSM modules want to use "struct task_struct"->
> security field. Although it won't be difficult to implement variable length
> "struct task_struct"->security field, let's think about it after we merged
> this patch.
>
> 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 | 9 ++++++++-
>  include/linux/sched.h     | 3 +++
>  include/linux/security.h  | 7 +++++++
>  kernel/fork.c             | 7 ++++++-
>  security/security.c       | 6 ++++++
>  6 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> index 325f649..6680e57 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 .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 9cf50ad..22f78e6 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -533,8 +533,13 @@
>   *     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.
> + *      Returns a zero on success, negative values on failure.


I'm not sure if this is the last version of the patch. However in
previous versions you had:
" @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."

Which gives some context about task_free, this doc is useful to have,
maybe add it later.


>   * @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:
> @@ -1478,6 +1483,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 +1750,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;
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 4d19052..c733036 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
> +       void *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 c2125e9..ee1685f 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..ad55915 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1643,9 +1643,12 @@ static __latent_entropy struct task_struct *copy_process(
>                 goto bad_fork_cleanup_perf;
>         /* copy all the process information */
>         shm_init_task(p);
> -       retval = copy_semundo(clone_flags, p);
> +       retval = security_task_alloc(p, clone_flags);
>         if (retval)
>                 goto bad_fork_cleanup_audit;

I tested this with Timgad module [1], and it works, for my case even
if security_task_free() is called I handle it... I would say may be it
would be a bit easy to handle if security_task_free() was not called
from both: when 1) copy_process() may fail or 2) from interrupt
context which means copy_process() succeeded which translates we have
2 states: 1) allocated but not active, and the later: 2) active or was
active... this may also avoid taking unnecessary write locks or
trigger workqueues to clean objects. However I'm fine with this too,
if you think that this should be there, the lsm has to be smarter


[1] http://www.openwall.com/lists/kernel-hardening/2017/02/02/21

Djalal Harouni Feb. 9, 2017, 9:41 a.m. UTC | #6

On Wed, Feb 8, 2017 at 2:21 PM, Tetsuo Handa
<[e-mail]> wrote:
> John Johansen wrote:
>> On 02/01/2017 08:02 PM, James Morris wrote:
>> > On Wed, 1 Feb 2017, John Johansen wrote:
>> >
>> >> Sorry this took so long, it looks good to me, and I have done some builds
>> >> and tests with apparmor using it. The apparmor patch to make use of this
>> >> follows as a reply.
>> >>
>> >> Acked-by: John Johansen <[e-mail]>
>> >
>> > We're too late in the -rc cycle to take these for 4.11.  Please keep
>> > testing them and some more review/acks would also be good.
>> >
>> Certainly, I didn't expect this to land in 4.11. I would really like
>> get some feed back/review/ack from the owner of the task struct.
>
> What does "the owner of the task struct" mean? "struct task_struct" is
> an object where fields are added/removed by individual subsystem. I think
> there is no explicit owner to ask feed back/review/ack.
>
> We added CONFIG_SECURITY_PATH in 2.6.29 and its user in 2.6.30.
> I think applying this patch without users in 4.11-rc1 for trial is better.
>
> TOMOYO can test this patch in 4.11-rcX if this patch is applied now.
> AppArmor can test this patch in AppArmor's tree.
> SELinux can apply security_task_create() => security_task_alloc() change
> in SELinux's tree.
> Casey can update "LSM: Stacking for major security modules - resend"
> with this change included.
> ptags and Timgad can evaluate this patch in their trees.
>
> We can update this patch in 4.12-rc1 if it turned out that this patch
> is insufficient for somebody.

That would work for me as I really wasted time since I didn't get the
full LSM stacking context (Thanks Casey for the work).
I ended up introducing more or less the same hook that's being discussed here.

Please Testuo could you point me to a branch where I can follow the
development regarding this hook ? Thank you!

Thanks!

> --
> 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

Tetsuo Handa Feb. 9, 2017, 11:30 a.m. UTC | #7

Djalal Harouni wrote:
> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> > index 9cf50ad..22f78e6 100644
> > --- a/include/linux/lsm_hooks.h
> > +++ b/include/linux/lsm_hooks.h
> > @@ -533,8 +533,13 @@
> >   *     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.
> > + *      Returns a zero on success, negative values on failure.
> 
> 
> I'm not sure if this is the last version of the patch. However in
> previous versions you had:

This is the last version of the patch.

> " @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."
> 
> Which gives some context about task_free, this doc is useful to have,
> maybe add it later.

This comment was dropped because I was suggested to start without stacking
which will entail rollback operation if one of users returned an error.

The simplest way to rollback is to call all task_free hooks regardless of
the location where task_alloc hook failed. I added this comment as a hint for
rolling back safely in order to make sure that all task_alloc users are
prepared for task_free hook being called without corresponding task_alloc call
before we encounter a user who is not prepared for such limitation.

> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 11c5c8a..ad55915 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -1643,9 +1643,12 @@ static __latent_entropy struct task_struct *copy_process(
> >                 goto bad_fork_cleanup_perf;
> >         /* copy all the process information */
> >         shm_init_task(p);
> > -       retval = copy_semundo(clone_flags, p);
> > +       retval = security_task_alloc(p, clone_flags);
> >         if (retval)
> >                 goto bad_fork_cleanup_audit;
> 
> I tested this with Timgad module [1], and it works, for my case even
> if security_task_free() is called I handle it...

Good.

>                                                  I would say may be it
> would be a bit easy to handle if security_task_free() was not called
> from both: when 1) copy_process() may fail or 2) from interrupt
> context which means copy_process() succeeded which translates we have
> 2 states: 1) allocated but not active, and the later: 2) active or was
> active... this may also avoid taking unnecessary write locks or
> trigger workqueues to clean objects. However I'm fine with this too,
> if you think that this should be there, the lsm has to be smarter

I didn't catch what you are worrying about. I think that since situations
where copy_process() fails after

  if (nr_threads >= max_threads)
      goto bad_fork_cleanup_count;

check are rare, we don't need to worry about cost of taking unnecessary
write locks or trigger workqueues to clean objects.

> 
> 
> [1] http://www.openwall.com/lists/kernel-hardening/2017/02/02/21
> 
--
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

Tetsuo Handa Feb. 9, 2017, 11:30 a.m. UTC | #8

Djalal Harouni wrote:
> On Wed, Feb 8, 2017 at 2:21 PM, Tetsuo Handa
> <[e-mail]> wrote:
> > John Johansen wrote:
> >> On 02/01/2017 08:02 PM, James Morris wrote:
> >> > On Wed, 1 Feb 2017, John Johansen wrote:
> >> >
> >> >> Sorry this took so long, it looks good to me, and I have done some builds
> >> >> and tests with apparmor using it. The apparmor patch to make use of this
> >> >> follows as a reply.
> >> >>
> >> >> Acked-by: John Johansen <[e-mail]>
> >> >
> >> > We're too late in the -rc cycle to take these for 4.11.  Please keep
> >> > testing them and some more review/acks would also be good.
> >> >
> >> Certainly, I didn't expect this to land in 4.11. I would really like
> >> get some feed back/review/ack from the owner of the task struct.
> >
> > What does "the owner of the task struct" mean? "struct task_struct" is
> > an object where fields are added/removed by individual subsystem. I think
> > there is no explicit owner to ask feed back/review/ack.
> >
> > We added CONFIG_SECURITY_PATH in 2.6.29 and its user in 2.6.30.
> > I think applying this patch without users in 4.11-rc1 for trial is better.
> >
> > TOMOYO can test this patch in 4.11-rcX if this patch is applied now.
> > AppArmor can test this patch in AppArmor's tree.
> > SELinux can apply security_task_create() => security_task_alloc() change
> > in SELinux's tree.
> > Casey can update "LSM: Stacking for major security modules - resend"
> > with this change included.
> > ptags and Timgad can evaluate this patch in their trees.
> >
> > We can update this patch in 4.12-rc1 if it turned out that this patch
> > is insufficient for somebody.
> 
> That would work for me as I really wasted time since I didn't get the
> full LSM stacking context (Thanks Casey for the work).
> I ended up introducing more or less the same hook that's being discussed here.
> 
> Please Testuo could you point me to a branch where I can follow the
> development regarding this hook ? Thank you!

If "this hook" == security_task_alloc(), there is no git tree for this hook.
I do "git commit" using linux-security.git#next , "git format-patches -1",
"git send-email" and "git reset --hard HEAD~1".

If "this hook" == full LSM stacking, it is Casey's smack-next tree at
https://github.com/cschaufler/smack-next.git#stacking-4.10-rc3 .
--
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

Djalal Harouni Feb. 9, 2017, 12:38 p.m. UTC | #9

On Thu, Feb 9, 2017 at 12:30 PM, Tetsuo Handa
<[e-mail]> wrote:
> Djalal Harouni wrote:
>> > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>> > index 9cf50ad..22f78e6 100644
>> > --- a/include/linux/lsm_hooks.h
>> > +++ b/include/linux/lsm_hooks.h
>> > @@ -533,8 +533,13 @@
>> >   *     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.
>> > + *      Returns a zero on success, negative values on failure.
>>
>>
>> I'm not sure if this is the last version of the patch. However in
>> previous versions you had:
>
> This is the last version of the patch.
>
>> " @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."
>>
>> Which gives some context about task_free, this doc is useful to have,
>> maybe add it later.
>
> This comment was dropped because I was suggested to start without stacking
> which will entail rollback operation if one of users returned an error.
>
> The simplest way to rollback is to call all task_free hooks regardless of
> the location where task_alloc hook failed. I added this comment as a hint for
> rolling back safely in order to make sure that all task_alloc users are
> prepared for task_free hook being called without corresponding task_alloc call
> before we encounter a user who is not prepared for such limitation.

I see ok, thanks

>> > diff --git a/kernel/fork.c b/kernel/fork.c
>> > index 11c5c8a..ad55915 100644
>> > --- a/kernel/fork.c
>> > +++ b/kernel/fork.c
>> > @@ -1643,9 +1643,12 @@ static __latent_entropy struct task_struct *copy_process(
>> >                 goto bad_fork_cleanup_perf;
>> >         /* copy all the process information */
>> >         shm_init_task(p);
>> > -       retval = copy_semundo(clone_flags, p);
>> > +       retval = security_task_alloc(p, clone_flags);
>> >         if (retval)
>> >                 goto bad_fork_cleanup_audit;
>>
>> I tested this with Timgad module [1], and it works, for my case even
>> if security_task_free() is called I handle it...
>
> Good.
>
>>                                                  I would say may be it
>> would be a bit easy to handle if security_task_free() was not called
>> from both: when 1) copy_process() may fail or 2) from interrupt
>> context which means copy_process() succeeded which translates we have
>> 2 states: 1) allocated but not active, and the later: 2) active or was
>> active... this may also avoid taking unnecessary write locks or
>> trigger workqueues to clean objects. However I'm fine with this too,
>> if you think that this should be there, the lsm has to be smarter
>
> I didn't catch what you are worrying about. I think that since situations
> where copy_process() fails after
>
>   if (nr_threads >= max_threads)
>       goto bad_fork_cleanup_count;
>
> check are rare, we don't need to worry about cost of taking unnecessary
> write locks or trigger workqueues to clean objects.

Alright.

Thanks!

>>
>>
>> [1] http://www.openwall.com/lists/kernel-hardening/2017/02/02/21
>>

Djalal Harouni Feb. 9, 2017, 12:48 p.m. UTC | #10

On Thu, Feb 9, 2017 at 12:30 PM, Tetsuo Handa
<[e-mail]> wrote:
> Djalal Harouni wrote:
>> On Wed, Feb 8, 2017 at 2:21 PM, Tetsuo Handa
>> <[e-mail]> wrote:
>> > John Johansen wrote:
>> >> On 02/01/2017 08:02 PM, James Morris wrote:
>> >> > On Wed, 1 Feb 2017, John Johansen wrote:
>> >> >
>> >> >> Sorry this took so long, it looks good to me, and I have done some builds
>> >> >> and tests with apparmor using it. The apparmor patch to make use of this
>> >> >> follows as a reply.
>> >> >>
>> >> >> Acked-by: John Johansen <[e-mail]>
>> >> >
>> >> > We're too late in the -rc cycle to take these for 4.11.  Please keep
>> >> > testing them and some more review/acks would also be good.
>> >> >
>> >> Certainly, I didn't expect this to land in 4.11. I would really like
>> >> get some feed back/review/ack from the owner of the task struct.
>> >
>> > What does "the owner of the task struct" mean? "struct task_struct" is
>> > an object where fields are added/removed by individual subsystem. I think
>> > there is no explicit owner to ask feed back/review/ack.
>> >
>> > We added CONFIG_SECURITY_PATH in 2.6.29 and its user in 2.6.30.
>> > I think applying this patch without users in 4.11-rc1 for trial is better.
>> >
>> > TOMOYO can test this patch in 4.11-rcX if this patch is applied now.
>> > AppArmor can test this patch in AppArmor's tree.
>> > SELinux can apply security_task_create() => security_task_alloc() change
>> > in SELinux's tree.
>> > Casey can update "LSM: Stacking for major security modules - resend"
>> > with this change included.
>> > ptags and Timgad can evaluate this patch in their trees.
>> >
>> > We can update this patch in 4.12-rc1 if it turned out that this patch
>> > is insufficient for somebody.
>>
>> That would work for me as I really wasted time since I didn't get the
>> full LSM stacking context (Thanks Casey for the work).
>> I ended up introducing more or less the same hook that's being discussed here.
>>
>> Please Testuo could you point me to a branch where I can follow the
>> development regarding this hook ? Thank you!
>
> If "this hook" == security_task_alloc(), there is no git tree for this hook.
> I do "git commit" using linux-security.git#next , "git format-patches -1",
> "git send-email" and "git reset --hard HEAD~1".

I was referring to only this hook, per task too. I tried to stack my
module as yama do it.

> If "this hook" == full LSM stacking, it is Casey's smack-next tree at
> https://github.com/cschaufler/smack-next.git#stacking-4.10-rc3 .

Ok thanks will have a look too.

For the security_task_alloc() hook:  Tested-by: Djalal Harouni
<[e-mail]>

John Johansen Feb. 14, 2017, 10:49 a.m. UTC | #11

On 02/08/2017 05:21 AM, Tetsuo Handa wrote:
> John Johansen wrote:
>> On 02/01/2017 08:02 PM, James Morris wrote:
>>> On Wed, 1 Feb 2017, John Johansen wrote:
>>>
>>>> Sorry this took so long, it looks good to me, and I have done some builds
>>>> and tests with apparmor using it. The apparmor patch to make use of this
>>>> follows as a reply.
>>>>
>>>> Acked-by: John Johansen <[e-mail]>
>>>
>>> We're too late in the -rc cycle to take these for 4.11.  Please keep 
>>> testing them and some more review/acks would also be good.
>>>
>> Certainly, I didn't expect this to land in 4.11. I would really like
>> get some feed back/review/ack from the owner of the task struct.
> 
> What does "the owner of the task struct" mean? "struct task_struct" is
> an object where fields are added/removed by individual subsystem. I think
> there is no explicit owner to ask feed back/review/ack.
> 
I merely meant someone who mucks around in task_struct and sched.h more
than we do

> We added CONFIG_SECURITY_PATH in 2.6.29 and its user in 2.6.30.
> I think applying this patch without users in 4.11-rc1 for trial is better.
> 
its certainly sooner, I am not opposed I just didn't expect for it to
go into 4.11, and expected people would want more time to look at and
play with it.

apparmor wise I still could do another patch to help clean things up
some more.

> TOMOYO can test this patch in 4.11-rcX if this patch is applied now.
> AppArmor can test this patch in AppArmor's tree.
> SELinux can apply security_task_create() => security_task_alloc() change
> in SELinux's tree.
> Casey can update "LSM: Stacking for major security modules - resend"
> with this change included.
> ptags and Timgad can evaluate this patch in their trees.
> 
> We can update this patch in 4.12-rc1 if it turned out that this patch
> is insufficient for somebody.
> 

--
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

José Bollo Feb. 14, 2017, 12:42 p.m. UTC | #12

On Wed, 8 Feb 2017 22:21:14 +0900
Tetsuo Handa <[e-mail]> wrote:

> John Johansen wrote:
> > On 02/01/2017 08:02 PM, James Morris wrote:  
> > > On Wed, 1 Feb 2017, John Johansen wrote:
> > >   
> > >> Sorry this took so long, it looks good to me, and I have done
> > >> some builds and tests with apparmor using it. The apparmor patch
> > >> to make use of this follows as a reply.
> > >>
> > >> Acked-by: John Johansen <[e-mail]>  
> > > 
> > > We're too late in the -rc cycle to take these for 4.11.  Please
> > > keep testing them and some more review/acks would also be good.
> > >   
> > Certainly, I didn't expect this to land in 4.11. I would really like
> > get some feed back/review/ack from the owner of the task struct.  
> 
> What does "the owner of the task struct" mean? "struct task_struct" is
> an object where fields are added/removed by individual subsystem. I
> think there is no explicit owner to ask feed back/review/ack.
> 
> We added CONFIG_SECURITY_PATH in 2.6.29 and its user in 2.6.30.
> I think applying this patch without users in 4.11-rc1 for trial is
> better.

agreed

> TOMOYO can test this patch in 4.11-rcX if this patch is applied now.
> AppArmor can test this patch in AppArmor's tree.
> SELinux can apply security_task_create() => security_task_alloc()
> change in SELinux's tree.
> Casey can update "LSM: Stacking for major security modules - resend"
> with this change included.
> ptags and Timgad can evaluate this patch in their trees.
> 
> We can update this patch in 4.12-rc1 if it turned out that this patch
> is insufficient for somebody.

agreed

I believe that there is a general agreement on the behaviour and the
spirit of the patch. So why not to go ahead?

a+j

--
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

Djalal Harouni Feb. 14, 2017, 12:46 p.m. UTC | #13

On Tue, Feb 14, 2017 at 11:49 AM, John Johansen
<[e-mail]> wrote:
> On 02/08/2017 05:21 AM, Tetsuo Handa wrote:
>> John Johansen wrote:
>>> On 02/01/2017 08:02 PM, James Morris wrote:
>>>> On Wed, 1 Feb 2017, John Johansen wrote:
>>>>
>>>>> Sorry this took so long, it looks good to me, and I have done some builds
>>>>> and tests with apparmor using it. The apparmor patch to make use of this
>>>>> follows as a reply.
>>>>>
>>>>> Acked-by: John Johansen <[e-mail]>
>>>>
>>>> We're too late in the -rc cycle to take these for 4.11.  Please keep
>>>> testing them and some more review/acks would also be good.
>>>>
>>> Certainly, I didn't expect this to land in 4.11. I would really like
>>> get some feed back/review/ack from the owner of the task struct.
>>
>> What does "the owner of the task struct" mean? "struct task_struct" is
>> an object where fields are added/removed by individual subsystem. I think
>> there is no explicit owner to ask feed back/review/ack.
>>
> I merely meant someone who mucks around in task_struct and sched.h more
> than we do
>
>> We added CONFIG_SECURITY_PATH in 2.6.29 and its user in 2.6.30.
>> I think applying this patch without users in 4.11-rc1 for trial is better.
>>
> its certainly sooner, I am not opposed I just didn't expect for it to
> go into 4.11, and expected people would want more time to look at and
> play with it.
>
> apparmor wise I still could do another patch to help clean things up
> some more.
>
I would have suggested at least to try to add the
security_task_alloc() hook without the security bit inside task_struct
(since that was tested), but sure you guys know better.

Thanks!

John Johansen Feb. 14, 2017, 1:17 p.m. UTC | #14

On 02/14/2017 04:46 AM, Djalal Harouni wrote:
> On Tue, Feb 14, 2017 at 11:49 AM, John Johansen
> <[e-mail]> wrote:
>> On 02/08/2017 05:21 AM, Tetsuo Handa wrote:
>>> John Johansen wrote:
>>>> On 02/01/2017 08:02 PM, James Morris wrote:
>>>>> On Wed, 1 Feb 2017, John Johansen wrote:
>>>>>
>>>>>> Sorry this took so long, it looks good to me, and I have done some builds
>>>>>> and tests with apparmor using it. The apparmor patch to make use of this
>>>>>> follows as a reply.
>>>>>>
>>>>>> Acked-by: John Johansen <[e-mail]>
>>>>>
>>>>> We're too late in the -rc cycle to take these for 4.11.  Please keep
>>>>> testing them and some more review/acks would also be good.
>>>>>
>>>> Certainly, I didn't expect this to land in 4.11. I would really like
>>>> get some feed back/review/ack from the owner of the task struct.
>>>
>>> What does "the owner of the task struct" mean? "struct task_struct" is
>>> an object where fields are added/removed by individual subsystem. I think
>>> there is no explicit owner to ask feed back/review/ack.
>>>
>> I merely meant someone who mucks around in task_struct and sched.h more
>> than we do
>>
>>> We added CONFIG_SECURITY_PATH in 2.6.29 and its user in 2.6.30.
>>> I think applying this patch without users in 4.11-rc1 for trial is better.
>>>
>> its certainly sooner, I am not opposed I just didn't expect for it to
>> go into 4.11, and expected people would want more time to look at and
>> play with it.
>>
>> apparmor wise I still could do another patch to help clean things up
>> some more.
>>
> I would have suggested at least to try to add the
> security_task_alloc() hook without the security bit inside task_struct
> (since that was tested), but sure you guys know better.
> 
locally I have the patch is split up into a series of smaller transforms.
I just figured those detailed steps were uninteresting for upstream and
harder to carry as part of this series.

For apparmor's current use just stubbing out the security_task_alloc()
hook without the security bits inside the task_struct doesn't make
a strong case for the hook because it would just be an empty stub.
And I wanted to make sure we provided a full use of the hook and
security field, along with some actually testing to support the proposed
change.

If you are interested in the incremental dev steps I can certainly make
them available.

--
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

Djalal Harouni Feb. 14, 2017, 5:18 p.m. UTC | #15

On Tue, Feb 14, 2017 at 2:17 PM, John Johansen
<[e-mail]> wrote:
> On 02/14/2017 04:46 AM, Djalal Harouni wrote:
>> On Tue, Feb 14, 2017 at 11:49 AM, John Johansen
>> <[e-mail]> wrote:
>>> On 02/08/2017 05:21 AM, Tetsuo Handa wrote:
>>>> John Johansen wrote:
>>>>> On 02/01/2017 08:02 PM, James Morris wrote:
>>>>>> On Wed, 1 Feb 2017, John Johansen wrote:
>>>>>>
>>>>>>> Sorry this took so long, it looks good to me, and I have done some builds
>>>>>>> and tests with apparmor using it. The apparmor patch to make use of this
>>>>>>> follows as a reply.
>>>>>>>
>>>>>>> Acked-by: John Johansen <[e-mail]>
>>>>>>
>>>>>> We're too late in the -rc cycle to take these for 4.11.  Please keep
>>>>>> testing them and some more review/acks would also be good.
>>>>>>
>>>>> Certainly, I didn't expect this to land in 4.11. I would really like
>>>>> get some feed back/review/ack from the owner of the task struct.
>>>>
>>>> What does "the owner of the task struct" mean? "struct task_struct" is
>>>> an object where fields are added/removed by individual subsystem. I think
>>>> there is no explicit owner to ask feed back/review/ack.
>>>>
>>> I merely meant someone who mucks around in task_struct and sched.h more
>>> than we do
>>>
>>>> We added CONFIG_SECURITY_PATH in 2.6.29 and its user in 2.6.30.
>>>> I think applying this patch without users in 4.11-rc1 for trial is better.
>>>>
>>> its certainly sooner, I am not opposed I just didn't expect for it to
>>> go into 4.11, and expected people would want more time to look at and
>>> play with it.
>>>
>>> apparmor wise I still could do another patch to help clean things up
>>> some more.
>>>
>> I would have suggested at least to try to add the
>> security_task_alloc() hook without the security bit inside task_struct
>> (since that was tested), but sure you guys know better.
>>
> locally I have the patch is split up into a series of smaller transforms.
> I just figured those detailed steps were uninteresting for upstream and
> harder to carry as part of this series.
>
> For apparmor's current use just stubbing out the security_task_alloc()
> hook without the security bits inside the task_struct doesn't make
> a strong case for the hook because it would just be an empty stub.
> And I wanted to make sure we provided a full use of the hook and
> security field, along with some actually testing to support the proposed
> change.
I understand, ok!

>
> If you are interested in the incremental dev steps I can certainly make
> them available.
>
Ok that may help sure,  I can follow what has already be done, what
are the plans and avoid some mistakes. Thank you!

Are you aware of any other public archive of linux-security-module
list ?  (seems lot of points have already been discussed).

Tetsuo Handa March 7, 2017, 10:35 a.m. UTC | #16

James, there seems to be no more comments.
Can we send this patch to linux-next.git via your tree?

Jose Bollo wrote:
> On Wed, 8 Feb 2017 22:21:14 +0900
> Tetsuo Handa <[e-mail]> wrote:
> 
> > John Johansen wrote:
> > > On 02/01/2017 08:02 PM, James Morris wrote:  
> > > > On Wed, 1 Feb 2017, John Johansen wrote:
> > > >   
> > > >> Sorry this took so long, it looks good to me, and I have done
> > > >> some builds and tests with apparmor using it. The apparmor patch
> > > >> to make use of this follows as a reply.
> > > >>
> > > >> Acked-by: John Johansen <[e-mail]>  
> > > > 
> > > > We're too late in the -rc cycle to take these for 4.11.  Please
> > > > keep testing them and some more review/acks would also be good.
> > > >   
> > > Certainly, I didn't expect this to land in 4.11. I would really like
> > > get some feed back/review/ack from the owner of the task struct.  
> > 
> > What does "the owner of the task struct" mean? "struct task_struct" is
> > an object where fields are added/removed by individual subsystem. I
> > think there is no explicit owner to ask feed back/review/ack.
> > 
> > We added CONFIG_SECURITY_PATH in 2.6.29 and its user in 2.6.30.
> > I think applying this patch without users in 4.11-rc1 for trial is
> > better.
> 
> agreed
> 
> > TOMOYO can test this patch in 4.11-rcX if this patch is applied now.
> > AppArmor can test this patch in AppArmor's tree.
> > SELinux can apply security_task_create() => security_task_alloc()
> > change in SELinux's tree.
> > Casey can update "LSM: Stacking for major security modules - resend"
> > with this change included.
> > ptags and Timgad can evaluate this patch in their trees.
> > 
> > We can update this patch in 4.12-rc1 if it turned out that this patch
> > is insufficient for somebody.
> 
> agreed
> 
> I believe that there is a general agreement on the behaviour and the
> spirit of the patch. So why not to go ahead?
> 
> a+j
--
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

Djalal Harouni March 8, 2017, 1:44 p.m. UTC | #17

On Tue, Mar 7, 2017 at 11:35 AM, Tetsuo Handa
<[e-mail]> wrote:
>
> James, there seems to be no more comments.
> Can we send this patch to linux-next.git via your tree?

I've been testing this hook and updated the new minimal LSM to block
module autoload to use it. Also I'm investigating if it's possible to
use Yama ptrace restrictions in containers where we don't want to
apply Yama ptrace_scope flag globally but opt-in, the flag should be
applied/dup'ed on processes/container tree without affecting the rest
of the system.

Thanks!

>
> Jose Bollo wrote:
> > On Wed, 8 Feb 2017 22:21:14 +0900
> > Tetsuo Handa <[e-mail]> wrote:
> >
> > > John Johansen wrote:
> > > > On 02/01/2017 08:02 PM, James Morris wrote:
> > > > > On Wed, 1 Feb 2017, John Johansen wrote:
> > > > >
> > > > >> Sorry this took so long, it looks good to me, and I have done
> > > > >> some builds and tests with apparmor using it. The apparmor patch
> > > > >> to make use of this follows as a reply.
> > > > >>
> > > > >> Acked-by: John Johansen <[e-mail]>
> > > > >
> > > > > We're too late in the -rc cycle to take these for 4.11.  Please
> > > > > keep testing them and some more review/acks would also be good.
> > > > >
> > > > Certainly, I didn't expect this to land in 4.11. I would really like
> > > > get some feed back/review/ack from the owner of the task struct.
> > >
> > > What does "the owner of the task struct" mean? "struct task_struct" is
> > > an object where fields are added/removed by individual subsystem. I
> > > think there is no explicit owner to ask feed back/review/ack.
> > >
> > > We added CONFIG_SECURITY_PATH in 2.6.29 and its user in 2.6.30.
> > > I think applying this patch without users in 4.11-rc1 for trial is
> > > better.
> >
> > agreed
> >
> > > TOMOYO can test this patch in 4.11-rcX if this patch is applied now.
> > > AppArmor can test this patch in AppArmor's tree.
> > > SELinux can apply security_task_create() => security_task_alloc()
> > > change in SELinux's tree.
> > > Casey can update "LSM: Stacking for major security modules - resend"
> > > with this change included.
> > > ptags and Timgad can evaluate this patch in their trees.
> > >
> > > We can update this patch in 4.12-rc1 if it turned out that this patch
> > > is insufficient for somebody.
> >
> > agreed
> >
> > I believe that there is a general agreement on the behaviour and the
> > spirit of the patch. So why not to go ahead?
> >
> > a+j
> --
> 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..6680e57 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 .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 9cf50ad..22f78e6 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -533,8 +533,13 @@ 
  *	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.
+ *      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:
@@ -1478,6 +1483,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 +1750,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;
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 4d19052..c733036 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
+	void *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 c2125e9..ee1685f 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..ad55915 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1643,9 +1643,12 @@  static __latent_entropy struct task_struct *copy_process(
 		goto bad_fork_cleanup_perf;
 	/* copy all the process information */
 	shm_init_task(p);
-	retval = copy_semundo(clone_flags, 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_security;
 	retval = copy_files(clone_flags, p);
 	if (retval)
 		goto bad_fork_cleanup_semundo;
@@ -1860,6 +1863,8 @@  static __latent_entropy struct task_struct *copy_process(
 	exit_files(p); /* blocking */
 bad_fork_cleanup_semundo:
 	exit_sem(p);
+bad_fork_cleanup_security:
+	security_task_free(p);
 bad_fork_cleanup_audit:
 	audit_free(p);
 bad_fork_cleanup_perf:
diff --git a/security/security.c b/security/security.c
index f825304..246f844 100644
--- a/security/security.c
+++ b/security/security.c
@@ -892,6 +892,11 @@  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)
+{
+	return call_int_hook(task_alloc, 0, task, clone_flags);
+}
+
 void security_task_free(struct task_struct *task)
 {
 	call_void_hook(task_free, task);
@@ -1731,6 +1736,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),