Message ID | 201701101958.JAD43709.OtJSOQFVFOLHMF@I-love.SAKURA.ne.jp (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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 <penguin-kernel@I-love.SAKURA.ne.jp> > 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 <penguin-kernel@I-love.SAKURA.ne.jp> > Cc: John Johansen <john.johansen@canonical.com> > Cc: Paul Moore <paul@paul-moore.com> > Cc: Stephen Smalley <sds@tycho.nsa.gov> > Cc: Eric Paris <eparis@parisplace.org> > Cc: Casey Schaufler <casey@schaufler-ca.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: James Morris <james.l.morris@oracle.com> > Cc: Serge E. Hallyn <serge@hallyn.com> > Cc: Jose Bollo <jobol@nonadev.net> 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 <john.johansen@canonical.com> > --- > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <john.johansen@canonical.com> 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
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 <john.johansen@canonical.com> > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <john.johansen@canonical.com> > > > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 10, 2017 at 11:58 AM, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> 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 <penguin-kernel@I-love.SAKURA.ne.jp> > 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 <penguin-kernel@I-love.SAKURA.ne.jp> > Cc: John Johansen <john.johansen@canonical.com> > Cc: Paul Moore <paul@paul-moore.com> > Cc: Stephen Smalley <sds@tycho.nsa.gov> > Cc: Eric Paris <eparis@parisplace.org> > Cc: Casey Schaufler <casey@schaufler-ca.com> > Cc: Kees Cook <keescook@chromium.org> > Cc: James Morris <james.l.morris@oracle.com> > Cc: Serge E. Hallyn <serge@hallyn.com> > Cc: Jose Bollo <jobol@nonadev.net> > --- > 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
On Wed, Feb 8, 2017 at 2:21 PM, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> 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 <john.johansen@canonical.com> >> > >> > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Djalal Harouni wrote: > On Wed, Feb 8, 2017 at 2:21 PM, Tetsuo Handa > <penguin-kernel@i-love.sakura.ne.jp> 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 <john.johansen@canonical.com> > >> > > >> > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 9, 2017 at 12:30 PM, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> 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 >>
On Thu, Feb 9, 2017 at 12:30 PM, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > Djalal Harouni wrote: >> On Wed, Feb 8, 2017 at 2:21 PM, Tetsuo Handa >> <penguin-kernel@i-love.sakura.ne.jp> 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 <john.johansen@canonical.com> >> >> > >> >> > 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 <tixxdz@gmail.com>
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 <john.johansen@canonical.com> >>> >>> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 8 Feb 2017 22:21:14 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> 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 <john.johansen@canonical.com> > > > > > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 14, 2017 at 11:49 AM, John Johansen <john.johansen@canonical.com> 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 <john.johansen@canonical.com> >>>> >>>> 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!
On 02/14/2017 04:46 AM, Djalal Harouni wrote: > On Tue, Feb 14, 2017 at 11:49 AM, John Johansen > <john.johansen@canonical.com> 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 <john.johansen@canonical.com> >>>>> >>>>> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 14, 2017 at 2:17 PM, John Johansen <john.johansen@canonical.com> wrote: > On 02/14/2017 04:46 AM, Djalal Harouni wrote: >> On Tue, Feb 14, 2017 at 11:49 AM, John Johansen >> <john.johansen@canonical.com> 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 <john.johansen@canonical.com> >>>>>> >>>>>> 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).
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 <penguin-kernel@I-love.SAKURA.ne.jp> 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 <john.johansen@canonical.com> > > > > > > > > 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 7, 2017 at 11:35 AM, Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> 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 <penguin-kernel@I-love.SAKURA.ne.jp> 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 <john.johansen@canonical.com> > > > > > > > > > > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
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),