[PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

[PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

[Tetsuo Handa]

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>
Acked-by: John Johansen <john.johansen@canonical.com>
Tested-by: Djalal Harouni <tixxdz@gmail.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: Jos? Bollo <jobol@nonadev.net>
---
 include/linux/init_task.h | 7 +++++++
 include/linux/lsm_hooks.h | 9 ++++++++-
 include/linux/sched.h     | 4 ++++
 include/linux/security.h  | 7 +++++++
 kernel/fork.c             | 7 ++++++-
 security/security.c       | 6 ++++++
 6 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index 91d9049..926f2f5 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -210,6 +210,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)
@@ -288,6 +294,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 1aa6333..a4193c3 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:
@@ -1482,6 +1487,7 @@
 	int (*file_open)(struct file *file, const struct cred *cred);
 
 	int (*task_create)(unsigned long clone_flags);
+	int (*task_alloc)(struct task_struct *task, unsigned long clone_flags);
 	void (*task_free)(struct task_struct *task);
 	int (*cred_alloc_blank)(struct cred *cred, gfp_t gfp);
 	void (*cred_free)(struct cred *cred);
@@ -1748,6 +1754,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 d67eee8..71b8df3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1038,6 +1038,10 @@ struct task_struct {
 	/* A live task holds one reference: */
 	atomic_t			stack_refcount;
 #endif
+#ifdef CONFIG_SECURITY
+	/* Used by LSM modules for access restriction: */
+	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 97df7ba..af675b5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -308,6 +308,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);
@@ -861,6 +862,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 6c463c80..3d32513 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1679,9 +1679,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;
@@ -1903,6 +1906,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 d6d18a3..5b6cdd0 100644
--- a/security/security.c
+++ b/security/security.c
@@ -930,6 +930,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);
@@ -1770,6 +1775,7 @@ struct security_hook_heads security_hook_heads __lsm_ro_after_init = {
 	.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),
-- 
1.8.3.1

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

[PATCH] apparmor: move task specific domain change info out of cred

[John Johansen]

Now that security_task_alloc() hook and "struct task_struct"->security
field are revived, move task specific domain change information for
change_onexec (equiv of setexeccon) and change_hat out of the cred
into a context off of the task_struct.

This cleans up apparmor's use of the cred structure so that it only
carries the reference to current mediation.

Signed-off-by: John Johansen <john.johansen@canonical.com>
---
 security/apparmor/context.c         | 129 +++++++++++++++++-------------------
 security/apparmor/domain.c          |  28 ++++----
 security/apparmor/include/context.h |  63 ++++++++----------
 security/apparmor/lsm.c             |  65 ++++++++++--------
 security/apparmor/policy.c          |   5 +-
 5 files changed, 143 insertions(+), 147 deletions(-)

diff --git a/security/apparmor/context.c b/security/apparmor/context.c
index 1fc16b8..8afb304 100644
--- a/security/apparmor/context.c
+++ b/security/apparmor/context.c
@@ -13,11 +13,9 @@
  * License.
  *
  *
- * AppArmor sets confinement on every task, via the the aa_task_ctx and
- * the aa_task_ctx.profile, both of which are required and are not allowed
- * to be NULL.  The aa_task_ctx is not reference counted and is unique
- * to each cred (which is reference count).  The profile pointed to by
- * the task_ctx is reference counted.
+ * AppArmor sets confinement on every task, via the cred_profile() which
+ * is required and is not allowed to be NULL.  The cred_profile is
+ * reference counted.
  *
  * TODO
  * If a task uses change_hat it currently does not return to the old
@@ -29,25 +27,42 @@
 #include "include/context.h"
 #include "include/policy.h"
 
+
+/**
+ * aa_get_task_profile - Get another task's profile
+ * @task: task to query  (NOT NULL)
+ *
+ * Returns: counted reference to @task's profile
+ */
+struct aa_profile *aa_get_task_profile(struct task_struct *task)
+{
+	struct aa_profile *p;
+
+	rcu_read_lock();
+	p = aa_get_profile(__aa_task_profile(task));
+	rcu_read_unlock();
+
+	return p;
+}
+
 /**
- * aa_alloc_task_context - allocate a new task_ctx
+ * aa_alloc_task_ctx - allocate a new task_ctx
  * @flags: gfp flags for allocation
  *
  * Returns: allocated buffer or NULL on failure
  */
-struct aa_task_ctx *aa_alloc_task_context(gfp_t flags)
+struct aa_task_ctx *aa_alloc_task_ctx(gfp_t flags)
 {
 	return kzalloc(sizeof(struct aa_task_ctx), flags);
 }
 
 /**
- * aa_free_task_context - free a task_ctx
+ * aa_free_task_ctx - free a task_ctx
  * @ctx: task_ctx to free (MAYBE NULL)
  */
-void aa_free_task_context(struct aa_task_ctx *ctx)
+void aa_free_task_ctx(struct aa_task_ctx *ctx)
 {
 	if (ctx) {
-		aa_put_profile(ctx->profile);
 		aa_put_profile(ctx->previous);
 		aa_put_profile(ctx->onexec);
 
@@ -56,36 +71,18 @@ void aa_free_task_context(struct aa_task_ctx *ctx)
 }
 
 /**
- * aa_dup_task_context - duplicate a task context, incrementing reference counts
+ * aa_dup_task_ctx - duplicate a task context, incrementing reference counts
  * @new: a blank task context      (NOT NULL)
  * @old: the task context to copy  (NOT NULL)
  */
-void aa_dup_task_context(struct aa_task_ctx *new, const struct aa_task_ctx *old)
+void aa_dup_task_ctx(struct aa_task_ctx *new, const struct aa_task_ctx *old)
 {
 	*new = *old;
-	aa_get_profile(new->profile);
 	aa_get_profile(new->previous);
 	aa_get_profile(new->onexec);
 }
 
 /**
- * aa_get_task_profile - Get another task's profile
- * @task: task to query  (NOT NULL)
- *
- * Returns: counted reference to @task's profile
- */
-struct aa_profile *aa_get_task_profile(struct task_struct *task)
-{
-	struct aa_profile *p;
-
-	rcu_read_lock();
-	p = aa_get_profile(__aa_task_profile(task));
-	rcu_read_unlock();
-
-	return p;
-}
-
-/**
  * aa_replace_current_profile - replace the current tasks profiles
  * @profile: new profile  (NOT NULL)
  *
@@ -93,36 +90,37 @@ struct aa_profile *aa_get_task_profile(struct task_struct *task)
  */
 int aa_replace_current_profile(struct aa_profile *profile)
 {
-	struct aa_task_ctx *ctx = current_ctx();
+	struct aa_profile *old = __aa_current_profile();
 	struct cred *new;
+
 	AA_BUG(!profile);
 
-	if (ctx->profile == profile)
+	if (old == profile)
 		return 0;
 
 	if (current_cred() != current_real_cred())
 		return -EBUSY;
 
 	new  = prepare_creds();
+	old = cred_profile(new);
 	if (!new)
 		return -ENOMEM;
 
-	ctx = cred_ctx(new);
-	if (unconfined(profile) || (ctx->profile->ns != profile->ns))
+	if (unconfined(profile) || (old->ns != profile->ns))
 		/* if switching to unconfined or a different profile namespace
 		 * clear out context state
 		 */
-		aa_clear_task_ctx_trans(ctx);
+		aa_clear_task_ctx(current_task_ctx());
 
 	/*
-	 * be careful switching ctx->profile, when racing replacement it
-	 * is possible that ctx->profile->proxy->profile is the reference
+	 * be careful switching cred profile, when racing replacement it
+	 * is possible that the cred profile's->proxy->profile is the reference
 	 * keeping @profile valid, so make sure to get its reference before
-	 * dropping the reference on ctx->profile
+	 * dropping the reference on the cred's profile
 	 */
 	aa_get_profile(profile);
-	aa_put_profile(ctx->profile);
-	ctx->profile = profile;
+	aa_put_profile(old);
+	cred_profile(new) = profile;
 
 	commit_creds(new);
 	return 0;
@@ -137,16 +135,12 @@ int aa_replace_current_profile(struct aa_profile *profile)
 int aa_set_current_onexec(struct aa_profile *profile)
 {
 	struct aa_task_ctx *ctx;
-	struct cred *new = prepare_creds();
-	if (!new)
-		return -ENOMEM;
 
-	ctx = cred_ctx(new);
+	ctx = current_task_ctx();
 	aa_get_profile(profile);
 	aa_put_profile(ctx->onexec);
 	ctx->onexec = profile;
 
-	commit_creds(new);
 	return 0;
 }
 
@@ -162,25 +156,28 @@ int aa_set_current_onexec(struct aa_profile *profile)
  */
 int aa_set_current_hat(struct aa_profile *profile, u64 token)
 {
-	struct aa_task_ctx *ctx;
-	struct cred *new = prepare_creds();
+	struct aa_task_ctx *ctx = current_task_ctx();
+	struct cred *new;
+
+	AA_BUG(!profile);
+
+	new = prepare_creds();
 	if (!new)
 		return -ENOMEM;
-	AA_BUG(!profile);
 
-	ctx = cred_ctx(new);
 	if (!ctx->previous) {
 		/* transfer refcount */
-		ctx->previous = ctx->profile;
+		ctx->previous = cred_profile(new);
 		ctx->token = token;
 	} else if (ctx->token == token) {
-		aa_put_profile(ctx->profile);
+		aa_put_profile(cred_profile(new));
 	} else {
 		/* previous_profile && ctx->token != token */
 		abort_creds(new);
 		return -EACCES;
 	}
-	ctx->profile = aa_get_newest_profile(profile);
+
+	cred_profile(new) = aa_get_newest_profile(profile);
 	/* clear exec on switching context */
 	aa_put_profile(ctx->onexec);
 	ctx->onexec = NULL;
@@ -200,28 +197,26 @@ int aa_set_current_hat(struct aa_profile *profile, u64 token)
  */
 int aa_restore_previous_profile(u64 token)
 {
-	struct aa_task_ctx *ctx;
-	struct cred *new = prepare_creds();
-	if (!new)
-		return -ENOMEM;
+	struct aa_task_ctx *ctx = current_task_ctx();
+	struct cred *new;
 
-	ctx = cred_ctx(new);
-	if (ctx->token != token) {
-		abort_creds(new);
+	if (ctx->token != token)
 		return -EACCES;
-	}
 	/* ignore restores when there is no saved profile */
-	if (!ctx->previous) {
-		abort_creds(new);
+	if (!ctx->previous)
 		return 0;
-	}
 
-	aa_put_profile(ctx->profile);
-	ctx->profile = aa_get_newest_profile(ctx->previous);
-	AA_BUG(!ctx->profile);
+	new = prepare_creds();
+	if (!new)
+		return -ENOMEM;
+
+	aa_put_profile(cred_profile(new));
+	cred_profile(new) = aa_get_newest_profile(ctx->previous);
+	AA_BUG(!cred_profile(new));
 	/* clear exec && prev information when restoring to previous context */
-	aa_clear_task_ctx_trans(ctx);
+	aa_clear_task_ctx(ctx);
 
 	commit_creds(new);
+
 	return 0;
 }
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index ef4beef..1994c02 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -353,10 +353,10 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 	if (bprm->cred_prepared)
 		return 0;
 
-	ctx = cred_ctx(bprm->cred);
+	ctx = current_task_ctx();
 	AA_BUG(!ctx);
-
-	profile = aa_get_newest_profile(ctx->profile);
+	AA_BUG(!cred_profile(bprm->cred));
+	profile = aa_get_newest_profile(cred_profile(bprm->cred));
 	/*
 	 * get the namespace from the replacement profile as replacement
 	 * can change the namespace
@@ -499,14 +499,11 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 	bprm->per_clear |= PER_CLEAR_ON_SETID;
 
 x_clear:
-	aa_put_profile(ctx->profile);
-	/* transfer new profile reference will be released when ctx is freed */
-	ctx->profile = new_profile;
+	aa_put_profile(profile);
+	/* transfer new profile reference will be released when cred is freed */
+	cred_profile(bprm->cred) = new_profile;
 	new_profile = NULL;
 
-	/* clear out all temporary/transitional state from the context */
-	aa_clear_task_ctx_trans(ctx);
-
 audit:
 	error = aa_audit_file(profile, &perms, OP_EXEC, MAY_EXEC, name,
 			      new_profile ? new_profile->base.hname : NULL,
@@ -544,17 +541,16 @@ int apparmor_bprm_secureexec(struct linux_binprm *bprm)
 void apparmor_bprm_committing_creds(struct linux_binprm *bprm)
 {
 	struct aa_profile *profile = __aa_current_profile();
-	struct aa_task_ctx *new_ctx = cred_ctx(bprm->cred);
+	struct aa_profile *new = cred_profile(bprm->cred);
 
 	/* bail out if unconfined or not changing profile */
-	if ((new_ctx->profile == profile) ||
-	    (unconfined(new_ctx->profile)))
+	if (new == profile || unconfined(new))
 		return;
 
 	current->pdeath_signal = 0;
 
 	/* reset soft limits and set hard limits for the new profile */
-	__aa_transition_rlimits(profile, new_ctx->profile);
+	__aa_transition_rlimits(profile, new);
 }
 
 /**
@@ -564,6 +560,10 @@ void apparmor_bprm_committing_creds(struct linux_binprm *bprm)
 void apparmor_bprm_committed_creds(struct linux_binprm *bprm)
 {
 	/* TODO: cleanup signals - ipc mediation */
+
+	/* clear out all temporary/transitional state from the context */
+	aa_clear_task_ctx(current_task_ctx());
+
 	return;
 }
 
@@ -621,7 +621,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
 
 	/* released below */
 	cred = get_current_cred();
-	ctx = cred_ctx(cred);
+	ctx = current_task_ctx();
 	profile = aa_get_newest_profile(aa_cred_profile(cred));
 	previous_profile = aa_get_newest_profile(ctx->previous);
 
diff --git a/security/apparmor/include/context.h b/security/apparmor/include/context.h
index 5b18fed..9943969 100644
--- a/security/apparmor/include/context.h
+++ b/security/apparmor/include/context.h
@@ -22,8 +22,9 @@
 #include "policy.h"
 #include "policy_ns.h"
 
-#define cred_ctx(X) ((X)->security)
-#define current_ctx() cred_ctx(current_cred())
+#define task_ctx(X) ((X)->security)
+#define current_task_ctx() (task_ctx(current))
+#define cred_profile(X) ((X)->security)
 
 /* struct aa_file_ctx - the AppArmor context the file was opened in
  * @perms: the permission the file was opened with
@@ -58,28 +59,23 @@ static inline void aa_free_file_context(struct aa_file_ctx *ctx)
 }
 
 /**
- * struct aa_task_ctx - primary label for confined tasks
- * @profile: the current profile   (NOT NULL)
- * @exec: profile to transition to on next exec  (MAYBE NULL)
- * @previous: profile the task may return to     (MAYBE NULL)
+ * struct aa_task_ctx - information for current task label change
+ * @onexec: profile to transition to on next exec  (MAY BE NULL)
+ * @previous: profile the task may return to     (MAY BE NULL)
  * @token: magic value the task must know for returning to @previous_profile
  *
- * Contains the task's current profile (which could change due to
- * change_hat).  Plus the hat_magic needed during change_hat.
- *
  * TODO: make so a task can be confined by a stack of contexts
  */
 struct aa_task_ctx {
-	struct aa_profile *profile;
 	struct aa_profile *onexec;
 	struct aa_profile *previous;
 	u64 token;
 };
 
-struct aa_task_ctx *aa_alloc_task_context(gfp_t flags);
-void aa_free_task_context(struct aa_task_ctx *ctx);
-void aa_dup_task_context(struct aa_task_ctx *new,
-			 const struct aa_task_ctx *old);
+struct aa_task_ctx *aa_alloc_task_ctx(gfp_t flags);
+void aa_free_task_ctx(struct aa_task_ctx *ctx);
+void aa_dup_task_ctx(struct aa_task_ctx *new, const struct aa_task_ctx *old);
+
 int aa_replace_current_profile(struct aa_profile *profile);
 int aa_set_current_onexec(struct aa_profile *profile);
 int aa_set_current_hat(struct aa_profile *profile, u64 token);
@@ -97,10 +93,11 @@ struct aa_profile *aa_get_task_profile(struct task_struct *task);
  */
 static inline struct aa_profile *aa_cred_profile(const struct cred *cred)
 {
-	struct aa_task_ctx *ctx = cred_ctx(cred);
+	struct aa_profile *profile = cred_profile(cred);
 
-	AA_BUG(!ctx || !ctx->profile);
-	return ctx->profile;
+	AA_BUG(!profile);
+
+	return profile;
 }
 
 /**
@@ -117,17 +114,6 @@ static inline struct aa_profile *__aa_task_profile(struct task_struct *task)
 }
 
 /**
- * __aa_task_is_confined - determine if @task has any confinement
- * @task: task to check confinement of  (NOT NULL)
- *
- * If @task != current needs to be called in RCU safe critical section
- */
-static inline bool __aa_task_is_confined(struct task_struct *task)
-{
-	return !unconfined(__aa_task_profile(task));
-}
-
-/**
  * __aa_current_profile - find the current tasks confining profile
  *
  * Returns: up to date confining profile or the ns unconfined profile (NOT NULL)
@@ -150,19 +136,17 @@ static inline struct aa_profile *__aa_current_profile(void)
  */
 static inline struct aa_profile *aa_current_profile(void)
 {
-	const struct aa_task_ctx *ctx = current_ctx();
-	struct aa_profile *profile;
+	struct aa_profile *profile = __aa_current_profile();
 
-	AA_BUG(!ctx || !ctx->profile);
+	AA_BUG(!profile);
 
-	if (profile_is_stale(ctx->profile)) {
-		profile = aa_get_newest_profile(ctx->profile);
+	if (profile_is_stale(profile)) {
+		profile = aa_get_newest_profile(profile);
 		aa_replace_current_profile(profile);
 		aa_put_profile(profile);
-		ctx = current_ctx();
 	}
 
-	return ctx->profile;
+	return profile;
 }
 
 static inline struct aa_ns *aa_get_current_ns(void)
@@ -170,12 +154,17 @@ static inline struct aa_ns *aa_get_current_ns(void)
 	return aa_get_ns(__aa_current_profile()->ns);
 }
 
+
+
+
 /**
- * aa_clear_task_ctx_trans - clear transition tracking info from the ctx
+ * aa_clear_task_ctx - clear transition tracking info from the ctx
  * @ctx: task context to clear (NOT NULL)
  */
-static inline void aa_clear_task_ctx_trans(struct aa_task_ctx *ctx)
+static inline void aa_clear_task_ctx(struct aa_task_ctx *ctx)
 {
+	AA_BUG(!ctx);
+
 	aa_put_profile(ctx->previous);
 	aa_put_profile(ctx->onexec);
 	ctx->previous = NULL;
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 1f2000d..ed9bf71 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -49,12 +49,12 @@ DEFINE_PER_CPU(struct aa_buffers, aa_buffers);
  */
 
 /*
- * free the associated aa_task_ctx and put its profiles
+ * put the associated profiles
  */
 static void apparmor_cred_free(struct cred *cred)
 {
-	aa_free_task_context(cred_ctx(cred));
-	cred_ctx(cred) = NULL;
+	aa_put_profile(cred_profile(cred));
+	cred_profile(cred) = NULL;
 }
 
 /*
@@ -62,30 +62,19 @@ static void apparmor_cred_free(struct cred *cred)
  */
 static int apparmor_cred_alloc_blank(struct cred *cred, gfp_t gfp)
 {
-	/* freed by apparmor_cred_free */
-	struct aa_task_ctx *ctx = aa_alloc_task_context(gfp);
-
-	if (!ctx)
-		return -ENOMEM;
-
-	cred_ctx(cred) = ctx;
+	cred_profile(cred) = NULL;
 	return 0;
 }
 
 /*
- * prepare new aa_task_ctx for modification by prepare_cred block
+ * prepare new cred profile for modification by prepare_cred block
  */
 static int apparmor_cred_prepare(struct cred *new, const struct cred *old,
 				 gfp_t gfp)
 {
-	/* freed by apparmor_cred_free */
-	struct aa_task_ctx *ctx = aa_alloc_task_context(gfp);
-
-	if (!ctx)
-		return -ENOMEM;
-
-	aa_dup_task_context(ctx, cred_ctx(old));
-	cred_ctx(new) = ctx;
+	struct aa_profile *tmp = cred_profile(new);
+	cred_profile(new) = aa_get_newest_profile(cred_profile(old));
+	aa_put_profile(tmp);
 	return 0;
 }
 
@@ -94,10 +83,30 @@ static int apparmor_cred_prepare(struct cred *new, const struct cred *old,
  */
 static void apparmor_cred_transfer(struct cred *new, const struct cred *old)
 {
-	const struct aa_task_ctx *old_ctx = cred_ctx(old);
-	struct aa_task_ctx *new_ctx = cred_ctx(new);
+	struct aa_profile *tmp = cred_profile(new);
+	cred_profile(new) = aa_get_newest_profile(cred_profile(old));
+	aa_put_profile(tmp);
+}
 
-	aa_dup_task_context(new_ctx, old_ctx);
+static void apparmor_task_free(struct task_struct *task)
+{
+
+	aa_free_task_ctx(task_ctx(task));
+	task_ctx(task) = NULL;
+}
+
+static int apparmor_task_alloc(struct task_struct *task,
+			       unsigned long clone_flags)
+{
+	struct aa_task_ctx *new = aa_alloc_task_ctx(GFP_KERNEL);
+
+	if (!new)
+		return -ENOMEM;
+
+	aa_dup_task_ctx(new, current_task_ctx());
+	task_ctx(task) = new;
+
+	return 0;
 }
 
 static int apparmor_ptrace_access_check(struct task_struct *child,
@@ -484,11 +493,11 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
 	int error = -ENOENT;
 	/* released below */
 	const struct cred *cred = get_task_cred(task);
-	struct aa_task_ctx *ctx = cred_ctx(cred);
+	struct aa_task_ctx *ctx = current_task_ctx();
 	struct aa_profile *profile = NULL;
 
 	if (strcmp(name, "current") == 0)
-		profile = aa_get_newest_profile(ctx->profile);
+		profile = aa_get_newest_profile(cred_profile(cred));
 	else if (strcmp(name, "prev") == 0  && ctx->previous)
 		profile = aa_get_newest_profile(ctx->previous);
 	else if (strcmp(name, "exec") == 0 && ctx->onexec)
@@ -629,6 +638,8 @@ static struct security_hook_list apparmor_hooks[] = {
 	LSM_HOOK_INIT(bprm_committed_creds, apparmor_bprm_committed_creds),
 	LSM_HOOK_INIT(bprm_secureexec, apparmor_bprm_secureexec),
 
+	LSM_HOOK_INIT(task_free, apparmor_task_free),
+	LSM_HOOK_INIT(task_alloc, apparmor_task_alloc),
 	LSM_HOOK_INIT(task_setrlimit, apparmor_task_setrlimit),
 };
 
@@ -871,12 +882,12 @@ static int __init set_init_ctx(void)
 	struct cred *cred = (struct cred *)current->real_cred;
 	struct aa_task_ctx *ctx;
 
-	ctx = aa_alloc_task_context(GFP_KERNEL);
+	ctx = aa_alloc_task_ctx(GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 
-	ctx->profile = aa_get_profile(root_ns->unconfined);
-	cred_ctx(cred) = ctx;
+	cred_profile(cred) = aa_get_profile(root_ns->unconfined);
+	task_ctx(current) = ctx;
 
 	return 0;
 }
diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index f44312a..b9c300b 100644
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -827,8 +827,9 @@ static int __lookup_replace(struct aa_ns *ns, const char *hname,
  * @udata: serialized data stream  (NOT NULL)
  *
  * unpack and replace a profile on the profile list and uses of that profile
- * by any aa_task_ctx.  If the profile does not exist on the profile list
- * it is added.
+ * by any task creds via invalidating the old version of the profile, which
+ * tasks will notice to update their own cred.  If the profile does not exist
+ * on the profile list it is added.
  *
  * Returns: size of data consumed else error code on failure.
  */
-- 
2.9.3

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

[PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

[Serge E. Hallyn]

Quoting Tetsuo Handa (penguin-kernel at I-love.SAKURA.ne.jp):
> 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>
> Acked-by: John Johansen <john.johansen@canonical.com>
> Tested-by: Djalal Harouni <tixxdz@gmail.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>

Acked-by: Serge Hallyn <serge@hallyn.com>

nit below,

> Cc: Jos? Bollo <jobol@nonadev.net>
> ---
>  include/linux/init_task.h | 7 +++++++
>  include/linux/lsm_hooks.h | 9 ++++++++-
>  include/linux/sched.h     | 4 ++++
>  include/linux/security.h  | 7 +++++++
>  kernel/fork.c             | 7 ++++++-
>  security/security.c       | 6 ++++++
>  6 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> index 91d9049..926f2f5 100644
> --- a/include/linux/init_task.h
> +++ b/include/linux/init_task.h
> @@ -210,6 +210,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)
> @@ -288,6 +294,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 1aa6333..a4193c3 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.

Note that here you have spaces not tabs

> + *      @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:
> @@ -1482,6 +1487,7 @@
>  	int (*file_open)(struct file *file, const struct cred *cred);
>  
>  	int (*task_create)(unsigned long clone_flags);
> +	int (*task_alloc)(struct task_struct *task, unsigned long clone_flags);
>  	void (*task_free)(struct task_struct *task);
>  	int (*cred_alloc_blank)(struct cred *cred, gfp_t gfp);
>  	void (*cred_free)(struct cred *cred);
> @@ -1748,6 +1754,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 d67eee8..71b8df3 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1038,6 +1038,10 @@ struct task_struct {
>  	/* A live task holds one reference: */
>  	atomic_t			stack_refcount;
>  #endif
> +#ifdef CONFIG_SECURITY
> +	/* Used by LSM modules for access restriction: */
> +	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 97df7ba..af675b5 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -308,6 +308,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);
> @@ -861,6 +862,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 6c463c80..3d32513 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1679,9 +1679,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;
> @@ -1903,6 +1906,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 d6d18a3..5b6cdd0 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -930,6 +930,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);
> @@ -1770,6 +1775,7 @@ struct security_hook_heads security_hook_heads __lsm_ro_after_init = {
>  	.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),
> -- 
> 1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

[Casey Schaufler]

On 3/11/2017 8:35 PM, Tetsuo Handa wrote:
> We switched from "struct task_struct"->security to "struct cred"->security
> in Linux 2.6.29. But not all LSM modules were happy with that change.
> TOMOYO LSM module is an example which want to use per "struct task_struct"
> security blob, for TOMOYO's security context is defined based on "struct
> task_struct" rather than "struct cred". AppArmor LSM module is another
> example which want to use it, for AppArmor is currently abusing the cred
> a little bit to store the change_hat and setexeccon info. Although
> security_task_free() hook was revived in Linux 3.4 because Yama LSM module
> wanted to release per "struct task_struct" security blob,
> security_task_alloc() hook and "struct task_struct"->security field were
> not revived. Nowadays, we are getting proposals of lightweight LSM modules
> which want to use per "struct task_struct" security blob. PTAGS LSM module
> and CaitSith LSM module which are currently under proposal for inclusion
> also want to use it. Therefore, it will be time to revive
> security_task_alloc() hook 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>
> Acked-by: John Johansen <john.johansen@canonical.com>
> Tested-by: Djalal Harouni <tixxdz@gmail.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>

Acked-by: 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: Jos? Bollo <jobol@nonadev.net>
> ---
>  include/linux/init_task.h | 7 +++++++
>  include/linux/lsm_hooks.h | 9 ++++++++-
>  include/linux/sched.h     | 4 ++++
>  include/linux/security.h  | 7 +++++++
>  kernel/fork.c             | 7 ++++++-
>  security/security.c       | 6 ++++++
>  6 files changed, 38 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> index 91d9049..926f2f5 100644
> --- a/include/linux/init_task.h
> +++ b/include/linux/init_task.h
> @@ -210,6 +210,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)
> @@ -288,6 +294,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 1aa6333..a4193c3 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:
> @@ -1482,6 +1487,7 @@
>  	int (*file_open)(struct file *file, const struct cred *cred);
>  
>  	int (*task_create)(unsigned long clone_flags);
> +	int (*task_alloc)(struct task_struct *task, unsigned long clone_flags);
>  	void (*task_free)(struct task_struct *task);
>  	int (*cred_alloc_blank)(struct cred *cred, gfp_t gfp);
>  	void (*cred_free)(struct cred *cred);
> @@ -1748,6 +1754,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 d67eee8..71b8df3 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1038,6 +1038,10 @@ struct task_struct {
>  	/* A live task holds one reference: */
>  	atomic_t			stack_refcount;
>  #endif
> +#ifdef CONFIG_SECURITY
> +	/* Used by LSM modules for access restriction: */
> +	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 97df7ba..af675b5 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -308,6 +308,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);
> @@ -861,6 +862,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 6c463c80..3d32513 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1679,9 +1679,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;
> @@ -1903,6 +1906,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 d6d18a3..5b6cdd0 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -930,6 +930,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);
> @@ -1770,6 +1775,7 @@ struct security_hook_heads security_hook_heads __lsm_ro_after_init = {
>  	.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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] apparmor: move task specific domain change info out of cred

[Serge E. Hallyn]

Quoting John Johansen (john.johansen at canonical.com):
> Now that security_task_alloc() hook and "struct task_struct"->security
> field are revived, move task specific domain change information for
> change_onexec (equiv of setexeccon) and change_hat out of the cred
> into a context off of the task_struct.
> 
> This cleans up apparmor's use of the cred structure so that it only
> carries the reference to current mediation.
> 
> Signed-off-by: John Johansen <john.johansen@canonical.com>

Thanks, John, that helps in compelling a review of the previous patch :)

So the task_struct->security pointer is only to store requested
transition profiles right?

> ---
>  security/apparmor/context.c         | 129 +++++++++++++++++-------------------
>  security/apparmor/domain.c          |  28 ++++----
>  security/apparmor/include/context.h |  63 ++++++++----------
>  security/apparmor/lsm.c             |  65 ++++++++++--------
>  security/apparmor/policy.c          |   5 +-
>  5 files changed, 143 insertions(+), 147 deletions(-)
> 
> diff --git a/security/apparmor/context.c b/security/apparmor/context.c
> index 1fc16b8..8afb304 100644
> --- a/security/apparmor/context.c
> +++ b/security/apparmor/context.c
> @@ -13,11 +13,9 @@
>   * License.
>   *
>   *
> - * AppArmor sets confinement on every task, via the the aa_task_ctx and
> - * the aa_task_ctx.profile, both of which are required and are not allowed
> - * to be NULL.  The aa_task_ctx is not reference counted and is unique
> - * to each cred (which is reference count).  The profile pointed to by
> - * the task_ctx is reference counted.
> + * AppArmor sets confinement on every task, via the cred_profile() which
> + * is required and is not allowed to be NULL.  The cred_profile is
> + * reference counted.
>   *
>   * TODO
>   * If a task uses change_hat it currently does not return to the old
> @@ -29,25 +27,42 @@
>  #include "include/context.h"
>  #include "include/policy.h"
>  
> +
> +/**
> + * aa_get_task_profile - Get another task's profile
> + * @task: task to query  (NOT NULL)
> + *
> + * Returns: counted reference to @task's profile
> + */
> +struct aa_profile *aa_get_task_profile(struct task_struct *task)
> +{
> +	struct aa_profile *p;
> +
> +	rcu_read_lock();
> +	p = aa_get_profile(__aa_task_profile(task));
> +	rcu_read_unlock();
> +
> +	return p;
> +}
> +
>  /**
> - * aa_alloc_task_context - allocate a new task_ctx
> + * aa_alloc_task_ctx - allocate a new task_ctx
>   * @flags: gfp flags for allocation
>   *
>   * Returns: allocated buffer or NULL on failure
>   */
> -struct aa_task_ctx *aa_alloc_task_context(gfp_t flags)
> +struct aa_task_ctx *aa_alloc_task_ctx(gfp_t flags)
>  {
>  	return kzalloc(sizeof(struct aa_task_ctx), flags);
>  }
>  
>  /**
> - * aa_free_task_context - free a task_ctx
> + * aa_free_task_ctx - free a task_ctx
>   * @ctx: task_ctx to free (MAYBE NULL)
>   */
> -void aa_free_task_context(struct aa_task_ctx *ctx)
> +void aa_free_task_ctx(struct aa_task_ctx *ctx)
>  {
>  	if (ctx) {
> -		aa_put_profile(ctx->profile);
>  		aa_put_profile(ctx->previous);
>  		aa_put_profile(ctx->onexec);
>  
> @@ -56,36 +71,18 @@ void aa_free_task_context(struct aa_task_ctx *ctx)
>  }
>  
>  /**
> - * aa_dup_task_context - duplicate a task context, incrementing reference counts
> + * aa_dup_task_ctx - duplicate a task context, incrementing reference counts
>   * @new: a blank task context      (NOT NULL)
>   * @old: the task context to copy  (NOT NULL)
>   */
> -void aa_dup_task_context(struct aa_task_ctx *new, const struct aa_task_ctx *old)
> +void aa_dup_task_ctx(struct aa_task_ctx *new, const struct aa_task_ctx *old)
>  {
>  	*new = *old;
> -	aa_get_profile(new->profile);
>  	aa_get_profile(new->previous);
>  	aa_get_profile(new->onexec);
>  }
>  
>  /**
> - * aa_get_task_profile - Get another task's profile
> - * @task: task to query  (NOT NULL)
> - *
> - * Returns: counted reference to @task's profile
> - */
> -struct aa_profile *aa_get_task_profile(struct task_struct *task)
> -{
> -	struct aa_profile *p;
> -
> -	rcu_read_lock();
> -	p = aa_get_profile(__aa_task_profile(task));
> -	rcu_read_unlock();
> -
> -	return p;
> -}
> -
> -/**
>   * aa_replace_current_profile - replace the current tasks profiles
>   * @profile: new profile  (NOT NULL)
>   *
> @@ -93,36 +90,37 @@ struct aa_profile *aa_get_task_profile(struct task_struct *task)
>   */
>  int aa_replace_current_profile(struct aa_profile *profile)
>  {
> -	struct aa_task_ctx *ctx = current_ctx();
> +	struct aa_profile *old = __aa_current_profile();
>  	struct cred *new;
> +
>  	AA_BUG(!profile);
>  
> -	if (ctx->profile == profile)
> +	if (old == profile)
>  		return 0;
>  
>  	if (current_cred() != current_real_cred())
>  		return -EBUSY;
>  
>  	new  = prepare_creds();
> +	old = cred_profile(new);
>  	if (!new)
>  		return -ENOMEM;
>  
> -	ctx = cred_ctx(new);
> -	if (unconfined(profile) || (ctx->profile->ns != profile->ns))
> +	if (unconfined(profile) || (old->ns != profile->ns))
>  		/* if switching to unconfined or a different profile namespace
>  		 * clear out context state
>  		 */
> -		aa_clear_task_ctx_trans(ctx);
> +		aa_clear_task_ctx(current_task_ctx());
>  
>  	/*
> -	 * be careful switching ctx->profile, when racing replacement it
> -	 * is possible that ctx->profile->proxy->profile is the reference
> +	 * be careful switching cred profile, when racing replacement it
> +	 * is possible that the cred profile's->proxy->profile is the reference
>  	 * keeping @profile valid, so make sure to get its reference before
> -	 * dropping the reference on ctx->profile
> +	 * dropping the reference on the cred's profile
>  	 */
>  	aa_get_profile(profile);
> -	aa_put_profile(ctx->profile);
> -	ctx->profile = profile;
> +	aa_put_profile(old);
> +	cred_profile(new) = profile;
>  
>  	commit_creds(new);
>  	return 0;
> @@ -137,16 +135,12 @@ int aa_replace_current_profile(struct aa_profile *profile)
>  int aa_set_current_onexec(struct aa_profile *profile)
>  {
>  	struct aa_task_ctx *ctx;
> -	struct cred *new = prepare_creds();
> -	if (!new)
> -		return -ENOMEM;
>  
> -	ctx = cred_ctx(new);
> +	ctx = current_task_ctx();
>  	aa_get_profile(profile);
>  	aa_put_profile(ctx->onexec);
>  	ctx->onexec = profile;
>  
> -	commit_creds(new);
>  	return 0;
>  }
>  
> @@ -162,25 +156,28 @@ int aa_set_current_onexec(struct aa_profile *profile)
>   */
>  int aa_set_current_hat(struct aa_profile *profile, u64 token)
>  {
> -	struct aa_task_ctx *ctx;
> -	struct cred *new = prepare_creds();
> +	struct aa_task_ctx *ctx = current_task_ctx();
> +	struct cred *new;
> +
> +	AA_BUG(!profile);
> +
> +	new = prepare_creds();
>  	if (!new)
>  		return -ENOMEM;
> -	AA_BUG(!profile);
>  
> -	ctx = cred_ctx(new);
>  	if (!ctx->previous) {
>  		/* transfer refcount */
> -		ctx->previous = ctx->profile;
> +		ctx->previous = cred_profile(new);
>  		ctx->token = token;
>  	} else if (ctx->token == token) {
> -		aa_put_profile(ctx->profile);
> +		aa_put_profile(cred_profile(new));
>  	} else {
>  		/* previous_profile && ctx->token != token */
>  		abort_creds(new);
>  		return -EACCES;
>  	}
> -	ctx->profile = aa_get_newest_profile(profile);
> +
> +	cred_profile(new) = aa_get_newest_profile(profile);
>  	/* clear exec on switching context */
>  	aa_put_profile(ctx->onexec);
>  	ctx->onexec = NULL;
> @@ -200,28 +197,26 @@ int aa_set_current_hat(struct aa_profile *profile, u64 token)
>   */
>  int aa_restore_previous_profile(u64 token)
>  {
> -	struct aa_task_ctx *ctx;
> -	struct cred *new = prepare_creds();
> -	if (!new)
> -		return -ENOMEM;
> +	struct aa_task_ctx *ctx = current_task_ctx();
> +	struct cred *new;
>  
> -	ctx = cred_ctx(new);
> -	if (ctx->token != token) {
> -		abort_creds(new);
> +	if (ctx->token != token)
>  		return -EACCES;
> -	}
>  	/* ignore restores when there is no saved profile */
> -	if (!ctx->previous) {
> -		abort_creds(new);
> +	if (!ctx->previous)
>  		return 0;
> -	}
>  
> -	aa_put_profile(ctx->profile);
> -	ctx->profile = aa_get_newest_profile(ctx->previous);
> -	AA_BUG(!ctx->profile);
> +	new = prepare_creds();
> +	if (!new)
> +		return -ENOMEM;
> +
> +	aa_put_profile(cred_profile(new));
> +	cred_profile(new) = aa_get_newest_profile(ctx->previous);
> +	AA_BUG(!cred_profile(new));
>  	/* clear exec && prev information when restoring to previous context */
> -	aa_clear_task_ctx_trans(ctx);
> +	aa_clear_task_ctx(ctx);
>  
>  	commit_creds(new);
> +
>  	return 0;
>  }
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index ef4beef..1994c02 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -353,10 +353,10 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  	if (bprm->cred_prepared)
>  		return 0;
>  
> -	ctx = cred_ctx(bprm->cred);
> +	ctx = current_task_ctx();
>  	AA_BUG(!ctx);
> -
> -	profile = aa_get_newest_profile(ctx->profile);
> +	AA_BUG(!cred_profile(bprm->cred));
> +	profile = aa_get_newest_profile(cred_profile(bprm->cred));
>  	/*
>  	 * get the namespace from the replacement profile as replacement
>  	 * can change the namespace
> @@ -499,14 +499,11 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  	bprm->per_clear |= PER_CLEAR_ON_SETID;
>  
>  x_clear:
> -	aa_put_profile(ctx->profile);
> -	/* transfer new profile reference will be released when ctx is freed */
> -	ctx->profile = new_profile;
> +	aa_put_profile(profile);
> +	/* transfer new profile reference will be released when cred is freed */
> +	cred_profile(bprm->cred) = new_profile;
>  	new_profile = NULL;
>  
> -	/* clear out all temporary/transitional state from the context */
> -	aa_clear_task_ctx_trans(ctx);
> -
>  audit:
>  	error = aa_audit_file(profile, &perms, OP_EXEC, MAY_EXEC, name,
>  			      new_profile ? new_profile->base.hname : NULL,
> @@ -544,17 +541,16 @@ int apparmor_bprm_secureexec(struct linux_binprm *bprm)
>  void apparmor_bprm_committing_creds(struct linux_binprm *bprm)
>  {
>  	struct aa_profile *profile = __aa_current_profile();
> -	struct aa_task_ctx *new_ctx = cred_ctx(bprm->cred);
> +	struct aa_profile *new = cred_profile(bprm->cred);
>  
>  	/* bail out if unconfined or not changing profile */
> -	if ((new_ctx->profile == profile) ||
> -	    (unconfined(new_ctx->profile)))
> +	if (new == profile || unconfined(new))
>  		return;
>  
>  	current->pdeath_signal = 0;
>  
>  	/* reset soft limits and set hard limits for the new profile */
> -	__aa_transition_rlimits(profile, new_ctx->profile);
> +	__aa_transition_rlimits(profile, new);
>  }
>  
>  /**
> @@ -564,6 +560,10 @@ void apparmor_bprm_committing_creds(struct linux_binprm *bprm)
>  void apparmor_bprm_committed_creds(struct linux_binprm *bprm)
>  {
>  	/* TODO: cleanup signals - ipc mediation */
> +
> +	/* clear out all temporary/transitional state from the context */
> +	aa_clear_task_ctx(current_task_ctx());
> +
>  	return;
>  }
>  
> @@ -621,7 +621,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
>  
>  	/* released below */
>  	cred = get_current_cred();
> -	ctx = cred_ctx(cred);
> +	ctx = current_task_ctx();
>  	profile = aa_get_newest_profile(aa_cred_profile(cred));
>  	previous_profile = aa_get_newest_profile(ctx->previous);
>  
> diff --git a/security/apparmor/include/context.h b/security/apparmor/include/context.h
> index 5b18fed..9943969 100644
> --- a/security/apparmor/include/context.h
> +++ b/security/apparmor/include/context.h
> @@ -22,8 +22,9 @@
>  #include "policy.h"
>  #include "policy_ns.h"
>  
> -#define cred_ctx(X) ((X)->security)
> -#define current_ctx() cred_ctx(current_cred())
> +#define task_ctx(X) ((X)->security)
> +#define current_task_ctx() (task_ctx(current))
> +#define cred_profile(X) ((X)->security)
>  
>  /* struct aa_file_ctx - the AppArmor context the file was opened in
>   * @perms: the permission the file was opened with
> @@ -58,28 +59,23 @@ static inline void aa_free_file_context(struct aa_file_ctx *ctx)
>  }
>  
>  /**
> - * struct aa_task_ctx - primary label for confined tasks
> - * @profile: the current profile   (NOT NULL)
> - * @exec: profile to transition to on next exec  (MAYBE NULL)
> - * @previous: profile the task may return to     (MAYBE NULL)
> + * struct aa_task_ctx - information for current task label change
> + * @onexec: profile to transition to on next exec  (MAY BE NULL)
> + * @previous: profile the task may return to     (MAY BE NULL)
>   * @token: magic value the task must know for returning to @previous_profile
>   *
> - * Contains the task's current profile (which could change due to
> - * change_hat).  Plus the hat_magic needed during change_hat.
> - *
>   * TODO: make so a task can be confined by a stack of contexts
>   */
>  struct aa_task_ctx {
> -	struct aa_profile *profile;
>  	struct aa_profile *onexec;
>  	struct aa_profile *previous;
>  	u64 token;
>  };
>  
> -struct aa_task_ctx *aa_alloc_task_context(gfp_t flags);
> -void aa_free_task_context(struct aa_task_ctx *ctx);
> -void aa_dup_task_context(struct aa_task_ctx *new,
> -			 const struct aa_task_ctx *old);
> +struct aa_task_ctx *aa_alloc_task_ctx(gfp_t flags);
> +void aa_free_task_ctx(struct aa_task_ctx *ctx);
> +void aa_dup_task_ctx(struct aa_task_ctx *new, const struct aa_task_ctx *old);
> +
>  int aa_replace_current_profile(struct aa_profile *profile);
>  int aa_set_current_onexec(struct aa_profile *profile);
>  int aa_set_current_hat(struct aa_profile *profile, u64 token);
> @@ -97,10 +93,11 @@ struct aa_profile *aa_get_task_profile(struct task_struct *task);
>   */
>  static inline struct aa_profile *aa_cred_profile(const struct cred *cred)
>  {
> -	struct aa_task_ctx *ctx = cred_ctx(cred);
> +	struct aa_profile *profile = cred_profile(cred);
>  
> -	AA_BUG(!ctx || !ctx->profile);
> -	return ctx->profile;
> +	AA_BUG(!profile);
> +
> +	return profile;
>  }
>  
>  /**
> @@ -117,17 +114,6 @@ static inline struct aa_profile *__aa_task_profile(struct task_struct *task)
>  }
>  
>  /**
> - * __aa_task_is_confined - determine if @task has any confinement
> - * @task: task to check confinement of  (NOT NULL)
> - *
> - * If @task != current needs to be called in RCU safe critical section
> - */
> -static inline bool __aa_task_is_confined(struct task_struct *task)
> -{
> -	return !unconfined(__aa_task_profile(task));
> -}
> -
> -/**
>   * __aa_current_profile - find the current tasks confining profile
>   *
>   * Returns: up to date confining profile or the ns unconfined profile (NOT NULL)
> @@ -150,19 +136,17 @@ static inline struct aa_profile *__aa_current_profile(void)
>   */
>  static inline struct aa_profile *aa_current_profile(void)
>  {
> -	const struct aa_task_ctx *ctx = current_ctx();
> -	struct aa_profile *profile;
> +	struct aa_profile *profile = __aa_current_profile();
>  
> -	AA_BUG(!ctx || !ctx->profile);
> +	AA_BUG(!profile);
>  
> -	if (profile_is_stale(ctx->profile)) {
> -		profile = aa_get_newest_profile(ctx->profile);
> +	if (profile_is_stale(profile)) {
> +		profile = aa_get_newest_profile(profile);
>  		aa_replace_current_profile(profile);
>  		aa_put_profile(profile);
> -		ctx = current_ctx();
>  	}
>  
> -	return ctx->profile;
> +	return profile;
>  }
>  
>  static inline struct aa_ns *aa_get_current_ns(void)
> @@ -170,12 +154,17 @@ static inline struct aa_ns *aa_get_current_ns(void)
>  	return aa_get_ns(__aa_current_profile()->ns);
>  }
>  
> +
> +
> +
>  /**
> - * aa_clear_task_ctx_trans - clear transition tracking info from the ctx
> + * aa_clear_task_ctx - clear transition tracking info from the ctx
>   * @ctx: task context to clear (NOT NULL)
>   */
> -static inline void aa_clear_task_ctx_trans(struct aa_task_ctx *ctx)
> +static inline void aa_clear_task_ctx(struct aa_task_ctx *ctx)
>  {
> +	AA_BUG(!ctx);
> +
>  	aa_put_profile(ctx->previous);
>  	aa_put_profile(ctx->onexec);
>  	ctx->previous = NULL;
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 1f2000d..ed9bf71 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -49,12 +49,12 @@ DEFINE_PER_CPU(struct aa_buffers, aa_buffers);
>   */
>  
>  /*
> - * free the associated aa_task_ctx and put its profiles
> + * put the associated profiles
>   */
>  static void apparmor_cred_free(struct cred *cred)
>  {
> -	aa_free_task_context(cred_ctx(cred));
> -	cred_ctx(cred) = NULL;
> +	aa_put_profile(cred_profile(cred));
> +	cred_profile(cred) = NULL;
>  }
>  
>  /*
> @@ -62,30 +62,19 @@ static void apparmor_cred_free(struct cred *cred)
>   */
>  static int apparmor_cred_alloc_blank(struct cred *cred, gfp_t gfp)
>  {
> -	/* freed by apparmor_cred_free */
> -	struct aa_task_ctx *ctx = aa_alloc_task_context(gfp);
> -
> -	if (!ctx)
> -		return -ENOMEM;
> -
> -	cred_ctx(cred) = ctx;
> +	cred_profile(cred) = NULL;
>  	return 0;
>  }
>  
>  /*
> - * prepare new aa_task_ctx for modification by prepare_cred block
> + * prepare new cred profile for modification by prepare_cred block
>   */
>  static int apparmor_cred_prepare(struct cred *new, const struct cred *old,
>  				 gfp_t gfp)
>  {
> -	/* freed by apparmor_cred_free */
> -	struct aa_task_ctx *ctx = aa_alloc_task_context(gfp);
> -
> -	if (!ctx)
> -		return -ENOMEM;
> -
> -	aa_dup_task_context(ctx, cred_ctx(old));
> -	cred_ctx(new) = ctx;
> +	struct aa_profile *tmp = cred_profile(new);
> +	cred_profile(new) = aa_get_newest_profile(cred_profile(old));
> +	aa_put_profile(tmp);
>  	return 0;
>  }
>  
> @@ -94,10 +83,30 @@ static int apparmor_cred_prepare(struct cred *new, const struct cred *old,
>   */
>  static void apparmor_cred_transfer(struct cred *new, const struct cred *old)
>  {
> -	const struct aa_task_ctx *old_ctx = cred_ctx(old);
> -	struct aa_task_ctx *new_ctx = cred_ctx(new);
> +	struct aa_profile *tmp = cred_profile(new);
> +	cred_profile(new) = aa_get_newest_profile(cred_profile(old));
> +	aa_put_profile(tmp);
> +}
>  
> -	aa_dup_task_context(new_ctx, old_ctx);
> +static void apparmor_task_free(struct task_struct *task)
> +{
> +
> +	aa_free_task_ctx(task_ctx(task));
> +	task_ctx(task) = NULL;
> +}
> +
> +static int apparmor_task_alloc(struct task_struct *task,
> +			       unsigned long clone_flags)
> +{
> +	struct aa_task_ctx *new = aa_alloc_task_ctx(GFP_KERNEL);
> +
> +	if (!new)
> +		return -ENOMEM;
> +
> +	aa_dup_task_ctx(new, current_task_ctx());
> +	task_ctx(task) = new;
> +
> +	return 0;
>  }
>  
>  static int apparmor_ptrace_access_check(struct task_struct *child,
> @@ -484,11 +493,11 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
>  	int error = -ENOENT;
>  	/* released below */
>  	const struct cred *cred = get_task_cred(task);
> -	struct aa_task_ctx *ctx = cred_ctx(cred);
> +	struct aa_task_ctx *ctx = current_task_ctx();
>  	struct aa_profile *profile = NULL;
>  
>  	if (strcmp(name, "current") == 0)
> -		profile = aa_get_newest_profile(ctx->profile);
> +		profile = aa_get_newest_profile(cred_profile(cred));
>  	else if (strcmp(name, "prev") == 0  && ctx->previous)
>  		profile = aa_get_newest_profile(ctx->previous);
>  	else if (strcmp(name, "exec") == 0 && ctx->onexec)
> @@ -629,6 +638,8 @@ static struct security_hook_list apparmor_hooks[] = {
>  	LSM_HOOK_INIT(bprm_committed_creds, apparmor_bprm_committed_creds),
>  	LSM_HOOK_INIT(bprm_secureexec, apparmor_bprm_secureexec),
>  
> +	LSM_HOOK_INIT(task_free, apparmor_task_free),
> +	LSM_HOOK_INIT(task_alloc, apparmor_task_alloc),
>  	LSM_HOOK_INIT(task_setrlimit, apparmor_task_setrlimit),
>  };
>  
> @@ -871,12 +882,12 @@ static int __init set_init_ctx(void)
>  	struct cred *cred = (struct cred *)current->real_cred;
>  	struct aa_task_ctx *ctx;
>  
> -	ctx = aa_alloc_task_context(GFP_KERNEL);
> +	ctx = aa_alloc_task_ctx(GFP_KERNEL);
>  	if (!ctx)
>  		return -ENOMEM;
>  
> -	ctx->profile = aa_get_profile(root_ns->unconfined);
> -	cred_ctx(cred) = ctx;
> +	cred_profile(cred) = aa_get_profile(root_ns->unconfined);
> +	task_ctx(current) = ctx;
>  
>  	return 0;
>  }
> diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
> index f44312a..b9c300b 100644
> --- a/security/apparmor/policy.c
> +++ b/security/apparmor/policy.c
> @@ -827,8 +827,9 @@ static int __lookup_replace(struct aa_ns *ns, const char *hname,
>   * @udata: serialized data stream  (NOT NULL)
>   *
>   * unpack and replace a profile on the profile list and uses of that profile
> - * by any aa_task_ctx.  If the profile does not exist on the profile list
> - * it is added.
> + * by any task creds via invalidating the old version of the profile, which
> + * tasks will notice to update their own cred.  If the profile does not exist
> + * on the profile list it is added.
>   *
>   * Returns: size of data consumed else error code on failure.
>   */
> -- 
> 2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] apparmor: move task specific domain change info out of cred

[John Johansen]

On 03/13/2017 09:47 AM, Serge E. Hallyn wrote:
> Quoting John Johansen (john.johansen at canonical.com):
>> Now that security_task_alloc() hook and "struct task_struct"->security
>> field are revived, move task specific domain change information for
>> change_onexec (equiv of setexeccon) and change_hat out of the cred
>> into a context off of the task_struct.
>>
>> This cleans up apparmor's use of the cred structure so that it only
>> carries the reference to current mediation.
>>
>> Signed-off-by: John Johansen <john.johansen@canonical.com>
> 
> Thanks, John, that helps in compelling a review of the previous patch :)
> 
> So the task_struct->security pointer is only to store requested
> transition profiles right?
> 
correct, well and support information for the transition like the random
magic token for change_hat.


>> ---
>>  security/apparmor/context.c         | 129 +++++++++++++++++-------------------
>>  security/apparmor/domain.c          |  28 ++++----
>>  security/apparmor/include/context.h |  63 ++++++++----------
>>  security/apparmor/lsm.c             |  65 ++++++++++--------
>>  security/apparmor/policy.c          |   5 +-
>>  5 files changed, 143 insertions(+), 147 deletions(-)
>>
>> diff --git a/security/apparmor/context.c b/security/apparmor/context.c
>> index 1fc16b8..8afb304 100644
>> --- a/security/apparmor/context.c
>> +++ b/security/apparmor/context.c
>> @@ -13,11 +13,9 @@
>>   * License.
>>   *
>>   *
>> - * AppArmor sets confinement on every task, via the the aa_task_ctx and
>> - * the aa_task_ctx.profile, both of which are required and are not allowed
>> - * to be NULL.  The aa_task_ctx is not reference counted and is unique
>> - * to each cred (which is reference count).  The profile pointed to by
>> - * the task_ctx is reference counted.
>> + * AppArmor sets confinement on every task, via the cred_profile() which
>> + * is required and is not allowed to be NULL.  The cred_profile is
>> + * reference counted.
>>   *
>>   * TODO
>>   * If a task uses change_hat it currently does not return to the old
>> @@ -29,25 +27,42 @@
>>  #include "include/context.h"
>>  #include "include/policy.h"
>>  
>> +
>> +/**
>> + * aa_get_task_profile - Get another task's profile
>> + * @task: task to query  (NOT NULL)
>> + *
>> + * Returns: counted reference to @task's profile
>> + */
>> +struct aa_profile *aa_get_task_profile(struct task_struct *task)
>> +{
>> +	struct aa_profile *p;
>> +
>> +	rcu_read_lock();
>> +	p = aa_get_profile(__aa_task_profile(task));
>> +	rcu_read_unlock();
>> +
>> +	return p;
>> +}
>> +
>>  /**
>> - * aa_alloc_task_context - allocate a new task_ctx
>> + * aa_alloc_task_ctx - allocate a new task_ctx
>>   * @flags: gfp flags for allocation
>>   *
>>   * Returns: allocated buffer or NULL on failure
>>   */
>> -struct aa_task_ctx *aa_alloc_task_context(gfp_t flags)
>> +struct aa_task_ctx *aa_alloc_task_ctx(gfp_t flags)
>>  {
>>  	return kzalloc(sizeof(struct aa_task_ctx), flags);
>>  }
>>  
>>  /**
>> - * aa_free_task_context - free a task_ctx
>> + * aa_free_task_ctx - free a task_ctx
>>   * @ctx: task_ctx to free (MAYBE NULL)
>>   */
>> -void aa_free_task_context(struct aa_task_ctx *ctx)
>> +void aa_free_task_ctx(struct aa_task_ctx *ctx)
>>  {
>>  	if (ctx) {
>> -		aa_put_profile(ctx->profile);
>>  		aa_put_profile(ctx->previous);
>>  		aa_put_profile(ctx->onexec);
>>  
>> @@ -56,36 +71,18 @@ void aa_free_task_context(struct aa_task_ctx *ctx)
>>  }
>>  
>>  /**
>> - * aa_dup_task_context - duplicate a task context, incrementing reference counts
>> + * aa_dup_task_ctx - duplicate a task context, incrementing reference counts
>>   * @new: a blank task context      (NOT NULL)
>>   * @old: the task context to copy  (NOT NULL)
>>   */
>> -void aa_dup_task_context(struct aa_task_ctx *new, const struct aa_task_ctx *old)
>> +void aa_dup_task_ctx(struct aa_task_ctx *new, const struct aa_task_ctx *old)
>>  {
>>  	*new = *old;
>> -	aa_get_profile(new->profile);
>>  	aa_get_profile(new->previous);
>>  	aa_get_profile(new->onexec);
>>  }
>>  
>>  /**
>> - * aa_get_task_profile - Get another task's profile
>> - * @task: task to query  (NOT NULL)
>> - *
>> - * Returns: counted reference to @task's profile
>> - */
>> -struct aa_profile *aa_get_task_profile(struct task_struct *task)
>> -{
>> -	struct aa_profile *p;
>> -
>> -	rcu_read_lock();
>> -	p = aa_get_profile(__aa_task_profile(task));
>> -	rcu_read_unlock();
>> -
>> -	return p;
>> -}
>> -
>> -/**
>>   * aa_replace_current_profile - replace the current tasks profiles
>>   * @profile: new profile  (NOT NULL)
>>   *
>> @@ -93,36 +90,37 @@ struct aa_profile *aa_get_task_profile(struct task_struct *task)
>>   */
>>  int aa_replace_current_profile(struct aa_profile *profile)
>>  {
>> -	struct aa_task_ctx *ctx = current_ctx();
>> +	struct aa_profile *old = __aa_current_profile();
>>  	struct cred *new;
>> +
>>  	AA_BUG(!profile);
>>  
>> -	if (ctx->profile == profile)
>> +	if (old == profile)
>>  		return 0;
>>  
>>  	if (current_cred() != current_real_cred())
>>  		return -EBUSY;
>>  
>>  	new  = prepare_creds();
>> +	old = cred_profile(new);
>>  	if (!new)
>>  		return -ENOMEM;
>>  
>> -	ctx = cred_ctx(new);
>> -	if (unconfined(profile) || (ctx->profile->ns != profile->ns))
>> +	if (unconfined(profile) || (old->ns != profile->ns))
>>  		/* if switching to unconfined or a different profile namespace
>>  		 * clear out context state
>>  		 */
>> -		aa_clear_task_ctx_trans(ctx);
>> +		aa_clear_task_ctx(current_task_ctx());
>>  
>>  	/*
>> -	 * be careful switching ctx->profile, when racing replacement it
>> -	 * is possible that ctx->profile->proxy->profile is the reference
>> +	 * be careful switching cred profile, when racing replacement it
>> +	 * is possible that the cred profile's->proxy->profile is the reference
>>  	 * keeping @profile valid, so make sure to get its reference before
>> -	 * dropping the reference on ctx->profile
>> +	 * dropping the reference on the cred's profile
>>  	 */
>>  	aa_get_profile(profile);
>> -	aa_put_profile(ctx->profile);
>> -	ctx->profile = profile;
>> +	aa_put_profile(old);
>> +	cred_profile(new) = profile;
>>  
>>  	commit_creds(new);
>>  	return 0;
>> @@ -137,16 +135,12 @@ int aa_replace_current_profile(struct aa_profile *profile)
>>  int aa_set_current_onexec(struct aa_profile *profile)
>>  {
>>  	struct aa_task_ctx *ctx;
>> -	struct cred *new = prepare_creds();
>> -	if (!new)
>> -		return -ENOMEM;
>>  
>> -	ctx = cred_ctx(new);
>> +	ctx = current_task_ctx();
>>  	aa_get_profile(profile);
>>  	aa_put_profile(ctx->onexec);
>>  	ctx->onexec = profile;
>>  
>> -	commit_creds(new);
>>  	return 0;
>>  }
>>  
>> @@ -162,25 +156,28 @@ int aa_set_current_onexec(struct aa_profile *profile)
>>   */
>>  int aa_set_current_hat(struct aa_profile *profile, u64 token)
>>  {
>> -	struct aa_task_ctx *ctx;
>> -	struct cred *new = prepare_creds();
>> +	struct aa_task_ctx *ctx = current_task_ctx();
>> +	struct cred *new;
>> +
>> +	AA_BUG(!profile);
>> +
>> +	new = prepare_creds();
>>  	if (!new)
>>  		return -ENOMEM;
>> -	AA_BUG(!profile);
>>  
>> -	ctx = cred_ctx(new);
>>  	if (!ctx->previous) {
>>  		/* transfer refcount */
>> -		ctx->previous = ctx->profile;
>> +		ctx->previous = cred_profile(new);
>>  		ctx->token = token;
>>  	} else if (ctx->token == token) {
>> -		aa_put_profile(ctx->profile);
>> +		aa_put_profile(cred_profile(new));
>>  	} else {
>>  		/* previous_profile && ctx->token != token */
>>  		abort_creds(new);
>>  		return -EACCES;
>>  	}
>> -	ctx->profile = aa_get_newest_profile(profile);
>> +
>> +	cred_profile(new) = aa_get_newest_profile(profile);
>>  	/* clear exec on switching context */
>>  	aa_put_profile(ctx->onexec);
>>  	ctx->onexec = NULL;
>> @@ -200,28 +197,26 @@ int aa_set_current_hat(struct aa_profile *profile, u64 token)
>>   */
>>  int aa_restore_previous_profile(u64 token)
>>  {
>> -	struct aa_task_ctx *ctx;
>> -	struct cred *new = prepare_creds();
>> -	if (!new)
>> -		return -ENOMEM;
>> +	struct aa_task_ctx *ctx = current_task_ctx();
>> +	struct cred *new;
>>  
>> -	ctx = cred_ctx(new);
>> -	if (ctx->token != token) {
>> -		abort_creds(new);
>> +	if (ctx->token != token)
>>  		return -EACCES;
>> -	}
>>  	/* ignore restores when there is no saved profile */
>> -	if (!ctx->previous) {
>> -		abort_creds(new);
>> +	if (!ctx->previous)
>>  		return 0;
>> -	}
>>  
>> -	aa_put_profile(ctx->profile);
>> -	ctx->profile = aa_get_newest_profile(ctx->previous);
>> -	AA_BUG(!ctx->profile);
>> +	new = prepare_creds();
>> +	if (!new)
>> +		return -ENOMEM;
>> +
>> +	aa_put_profile(cred_profile(new));
>> +	cred_profile(new) = aa_get_newest_profile(ctx->previous);
>> +	AA_BUG(!cred_profile(new));
>>  	/* clear exec && prev information when restoring to previous context */
>> -	aa_clear_task_ctx_trans(ctx);
>> +	aa_clear_task_ctx(ctx);
>>  
>>  	commit_creds(new);
>> +
>>  	return 0;
>>  }
>> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
>> index ef4beef..1994c02 100644
>> --- a/security/apparmor/domain.c
>> +++ b/security/apparmor/domain.c
>> @@ -353,10 +353,10 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>>  	if (bprm->cred_prepared)
>>  		return 0;
>>  
>> -	ctx = cred_ctx(bprm->cred);
>> +	ctx = current_task_ctx();
>>  	AA_BUG(!ctx);
>> -
>> -	profile = aa_get_newest_profile(ctx->profile);
>> +	AA_BUG(!cred_profile(bprm->cred));
>> +	profile = aa_get_newest_profile(cred_profile(bprm->cred));
>>  	/*
>>  	 * get the namespace from the replacement profile as replacement
>>  	 * can change the namespace
>> @@ -499,14 +499,11 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>>  	bprm->per_clear |= PER_CLEAR_ON_SETID;
>>  
>>  x_clear:
>> -	aa_put_profile(ctx->profile);
>> -	/* transfer new profile reference will be released when ctx is freed */
>> -	ctx->profile = new_profile;
>> +	aa_put_profile(profile);
>> +	/* transfer new profile reference will be released when cred is freed */
>> +	cred_profile(bprm->cred) = new_profile;
>>  	new_profile = NULL;
>>  
>> -	/* clear out all temporary/transitional state from the context */
>> -	aa_clear_task_ctx_trans(ctx);
>> -
>>  audit:
>>  	error = aa_audit_file(profile, &perms, OP_EXEC, MAY_EXEC, name,
>>  			      new_profile ? new_profile->base.hname : NULL,
>> @@ -544,17 +541,16 @@ int apparmor_bprm_secureexec(struct linux_binprm *bprm)
>>  void apparmor_bprm_committing_creds(struct linux_binprm *bprm)
>>  {
>>  	struct aa_profile *profile = __aa_current_profile();
>> -	struct aa_task_ctx *new_ctx = cred_ctx(bprm->cred);
>> +	struct aa_profile *new = cred_profile(bprm->cred);
>>  
>>  	/* bail out if unconfined or not changing profile */
>> -	if ((new_ctx->profile == profile) ||
>> -	    (unconfined(new_ctx->profile)))
>> +	if (new == profile || unconfined(new))
>>  		return;
>>  
>>  	current->pdeath_signal = 0;
>>  
>>  	/* reset soft limits and set hard limits for the new profile */
>> -	__aa_transition_rlimits(profile, new_ctx->profile);
>> +	__aa_transition_rlimits(profile, new);
>>  }
>>  
>>  /**
>> @@ -564,6 +560,10 @@ void apparmor_bprm_committing_creds(struct linux_binprm *bprm)
>>  void apparmor_bprm_committed_creds(struct linux_binprm *bprm)
>>  {
>>  	/* TODO: cleanup signals - ipc mediation */
>> +
>> +	/* clear out all temporary/transitional state from the context */
>> +	aa_clear_task_ctx(current_task_ctx());
>> +
>>  	return;
>>  }
>>  
>> @@ -621,7 +621,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
>>  
>>  	/* released below */
>>  	cred = get_current_cred();
>> -	ctx = cred_ctx(cred);
>> +	ctx = current_task_ctx();
>>  	profile = aa_get_newest_profile(aa_cred_profile(cred));
>>  	previous_profile = aa_get_newest_profile(ctx->previous);
>>  
>> diff --git a/security/apparmor/include/context.h b/security/apparmor/include/context.h
>> index 5b18fed..9943969 100644
>> --- a/security/apparmor/include/context.h
>> +++ b/security/apparmor/include/context.h
>> @@ -22,8 +22,9 @@
>>  #include "policy.h"
>>  #include "policy_ns.h"
>>  
>> -#define cred_ctx(X) ((X)->security)
>> -#define current_ctx() cred_ctx(current_cred())
>> +#define task_ctx(X) ((X)->security)
>> +#define current_task_ctx() (task_ctx(current))
>> +#define cred_profile(X) ((X)->security)
>>  
>>  /* struct aa_file_ctx - the AppArmor context the file was opened in
>>   * @perms: the permission the file was opened with
>> @@ -58,28 +59,23 @@ static inline void aa_free_file_context(struct aa_file_ctx *ctx)
>>  }
>>  
>>  /**
>> - * struct aa_task_ctx - primary label for confined tasks
>> - * @profile: the current profile   (NOT NULL)
>> - * @exec: profile to transition to on next exec  (MAYBE NULL)
>> - * @previous: profile the task may return to     (MAYBE NULL)
>> + * struct aa_task_ctx - information for current task label change
>> + * @onexec: profile to transition to on next exec  (MAY BE NULL)
>> + * @previous: profile the task may return to     (MAY BE NULL)
>>   * @token: magic value the task must know for returning to @previous_profile
>>   *
>> - * Contains the task's current profile (which could change due to
>> - * change_hat).  Plus the hat_magic needed during change_hat.
>> - *
>>   * TODO: make so a task can be confined by a stack of contexts
>>   */
>>  struct aa_task_ctx {
>> -	struct aa_profile *profile;
>>  	struct aa_profile *onexec;
>>  	struct aa_profile *previous;
>>  	u64 token;
>>  };
>>  
>> -struct aa_task_ctx *aa_alloc_task_context(gfp_t flags);
>> -void aa_free_task_context(struct aa_task_ctx *ctx);
>> -void aa_dup_task_context(struct aa_task_ctx *new,
>> -			 const struct aa_task_ctx *old);
>> +struct aa_task_ctx *aa_alloc_task_ctx(gfp_t flags);
>> +void aa_free_task_ctx(struct aa_task_ctx *ctx);
>> +void aa_dup_task_ctx(struct aa_task_ctx *new, const struct aa_task_ctx *old);
>> +
>>  int aa_replace_current_profile(struct aa_profile *profile);
>>  int aa_set_current_onexec(struct aa_profile *profile);
>>  int aa_set_current_hat(struct aa_profile *profile, u64 token);
>> @@ -97,10 +93,11 @@ struct aa_profile *aa_get_task_profile(struct task_struct *task);
>>   */
>>  static inline struct aa_profile *aa_cred_profile(const struct cred *cred)
>>  {
>> -	struct aa_task_ctx *ctx = cred_ctx(cred);
>> +	struct aa_profile *profile = cred_profile(cred);
>>  
>> -	AA_BUG(!ctx || !ctx->profile);
>> -	return ctx->profile;
>> +	AA_BUG(!profile);
>> +
>> +	return profile;
>>  }
>>  
>>  /**
>> @@ -117,17 +114,6 @@ static inline struct aa_profile *__aa_task_profile(struct task_struct *task)
>>  }
>>  
>>  /**
>> - * __aa_task_is_confined - determine if @task has any confinement
>> - * @task: task to check confinement of  (NOT NULL)
>> - *
>> - * If @task != current needs to be called in RCU safe critical section
>> - */
>> -static inline bool __aa_task_is_confined(struct task_struct *task)
>> -{
>> -	return !unconfined(__aa_task_profile(task));
>> -}
>> -
>> -/**
>>   * __aa_current_profile - find the current tasks confining profile
>>   *
>>   * Returns: up to date confining profile or the ns unconfined profile (NOT NULL)
>> @@ -150,19 +136,17 @@ static inline struct aa_profile *__aa_current_profile(void)
>>   */
>>  static inline struct aa_profile *aa_current_profile(void)
>>  {
>> -	const struct aa_task_ctx *ctx = current_ctx();
>> -	struct aa_profile *profile;
>> +	struct aa_profile *profile = __aa_current_profile();
>>  
>> -	AA_BUG(!ctx || !ctx->profile);
>> +	AA_BUG(!profile);
>>  
>> -	if (profile_is_stale(ctx->profile)) {
>> -		profile = aa_get_newest_profile(ctx->profile);
>> +	if (profile_is_stale(profile)) {
>> +		profile = aa_get_newest_profile(profile);
>>  		aa_replace_current_profile(profile);
>>  		aa_put_profile(profile);
>> -		ctx = current_ctx();
>>  	}
>>  
>> -	return ctx->profile;
>> +	return profile;
>>  }
>>  
>>  static inline struct aa_ns *aa_get_current_ns(void)
>> @@ -170,12 +154,17 @@ static inline struct aa_ns *aa_get_current_ns(void)
>>  	return aa_get_ns(__aa_current_profile()->ns);
>>  }
>>  
>> +
>> +
>> +
>>  /**
>> - * aa_clear_task_ctx_trans - clear transition tracking info from the ctx
>> + * aa_clear_task_ctx - clear transition tracking info from the ctx
>>   * @ctx: task context to clear (NOT NULL)
>>   */
>> -static inline void aa_clear_task_ctx_trans(struct aa_task_ctx *ctx)
>> +static inline void aa_clear_task_ctx(struct aa_task_ctx *ctx)
>>  {
>> +	AA_BUG(!ctx);
>> +
>>  	aa_put_profile(ctx->previous);
>>  	aa_put_profile(ctx->onexec);
>>  	ctx->previous = NULL;
>> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
>> index 1f2000d..ed9bf71 100644
>> --- a/security/apparmor/lsm.c
>> +++ b/security/apparmor/lsm.c
>> @@ -49,12 +49,12 @@ DEFINE_PER_CPU(struct aa_buffers, aa_buffers);
>>   */
>>  
>>  /*
>> - * free the associated aa_task_ctx and put its profiles
>> + * put the associated profiles
>>   */
>>  static void apparmor_cred_free(struct cred *cred)
>>  {
>> -	aa_free_task_context(cred_ctx(cred));
>> -	cred_ctx(cred) = NULL;
>> +	aa_put_profile(cred_profile(cred));
>> +	cred_profile(cred) = NULL;
>>  }
>>  
>>  /*
>> @@ -62,30 +62,19 @@ static void apparmor_cred_free(struct cred *cred)
>>   */
>>  static int apparmor_cred_alloc_blank(struct cred *cred, gfp_t gfp)
>>  {
>> -	/* freed by apparmor_cred_free */
>> -	struct aa_task_ctx *ctx = aa_alloc_task_context(gfp);
>> -
>> -	if (!ctx)
>> -		return -ENOMEM;
>> -
>> -	cred_ctx(cred) = ctx;
>> +	cred_profile(cred) = NULL;
>>  	return 0;
>>  }
>>  
>>  /*
>> - * prepare new aa_task_ctx for modification by prepare_cred block
>> + * prepare new cred profile for modification by prepare_cred block
>>   */
>>  static int apparmor_cred_prepare(struct cred *new, const struct cred *old,
>>  				 gfp_t gfp)
>>  {
>> -	/* freed by apparmor_cred_free */
>> -	struct aa_task_ctx *ctx = aa_alloc_task_context(gfp);
>> -
>> -	if (!ctx)
>> -		return -ENOMEM;
>> -
>> -	aa_dup_task_context(ctx, cred_ctx(old));
>> -	cred_ctx(new) = ctx;
>> +	struct aa_profile *tmp = cred_profile(new);
>> +	cred_profile(new) = aa_get_newest_profile(cred_profile(old));
>> +	aa_put_profile(tmp);
>>  	return 0;
>>  }
>>  
>> @@ -94,10 +83,30 @@ static int apparmor_cred_prepare(struct cred *new, const struct cred *old,
>>   */
>>  static void apparmor_cred_transfer(struct cred *new, const struct cred *old)
>>  {
>> -	const struct aa_task_ctx *old_ctx = cred_ctx(old);
>> -	struct aa_task_ctx *new_ctx = cred_ctx(new);
>> +	struct aa_profile *tmp = cred_profile(new);
>> +	cred_profile(new) = aa_get_newest_profile(cred_profile(old));
>> +	aa_put_profile(tmp);
>> +}
>>  
>> -	aa_dup_task_context(new_ctx, old_ctx);
>> +static void apparmor_task_free(struct task_struct *task)
>> +{
>> +
>> +	aa_free_task_ctx(task_ctx(task));
>> +	task_ctx(task) = NULL;
>> +}
>> +
>> +static int apparmor_task_alloc(struct task_struct *task,
>> +			       unsigned long clone_flags)
>> +{
>> +	struct aa_task_ctx *new = aa_alloc_task_ctx(GFP_KERNEL);
>> +
>> +	if (!new)
>> +		return -ENOMEM;
>> +
>> +	aa_dup_task_ctx(new, current_task_ctx());
>> +	task_ctx(task) = new;
>> +
>> +	return 0;
>>  }
>>  
>>  static int apparmor_ptrace_access_check(struct task_struct *child,
>> @@ -484,11 +493,11 @@ static int apparmor_getprocattr(struct task_struct *task, char *name,
>>  	int error = -ENOENT;
>>  	/* released below */
>>  	const struct cred *cred = get_task_cred(task);
>> -	struct aa_task_ctx *ctx = cred_ctx(cred);
>> +	struct aa_task_ctx *ctx = current_task_ctx();
>>  	struct aa_profile *profile = NULL;
>>  
>>  	if (strcmp(name, "current") == 0)
>> -		profile = aa_get_newest_profile(ctx->profile);
>> +		profile = aa_get_newest_profile(cred_profile(cred));
>>  	else if (strcmp(name, "prev") == 0  && ctx->previous)
>>  		profile = aa_get_newest_profile(ctx->previous);
>>  	else if (strcmp(name, "exec") == 0 && ctx->onexec)
>> @@ -629,6 +638,8 @@ static struct security_hook_list apparmor_hooks[] = {
>>  	LSM_HOOK_INIT(bprm_committed_creds, apparmor_bprm_committed_creds),
>>  	LSM_HOOK_INIT(bprm_secureexec, apparmor_bprm_secureexec),
>>  
>> +	LSM_HOOK_INIT(task_free, apparmor_task_free),
>> +	LSM_HOOK_INIT(task_alloc, apparmor_task_alloc),
>>  	LSM_HOOK_INIT(task_setrlimit, apparmor_task_setrlimit),
>>  };
>>  
>> @@ -871,12 +882,12 @@ static int __init set_init_ctx(void)
>>  	struct cred *cred = (struct cred *)current->real_cred;
>>  	struct aa_task_ctx *ctx;
>>  
>> -	ctx = aa_alloc_task_context(GFP_KERNEL);
>> +	ctx = aa_alloc_task_ctx(GFP_KERNEL);
>>  	if (!ctx)
>>  		return -ENOMEM;
>>  
>> -	ctx->profile = aa_get_profile(root_ns->unconfined);
>> -	cred_ctx(cred) = ctx;
>> +	cred_profile(cred) = aa_get_profile(root_ns->unconfined);
>> +	task_ctx(current) = ctx;
>>  
>>  	return 0;
>>  }
>> diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
>> index f44312a..b9c300b 100644
>> --- a/security/apparmor/policy.c
>> +++ b/security/apparmor/policy.c
>> @@ -827,8 +827,9 @@ static int __lookup_replace(struct aa_ns *ns, const char *hname,
>>   * @udata: serialized data stream  (NOT NULL)
>>   *
>>   * unpack and replace a profile on the profile list and uses of that profile
>> - * by any aa_task_ctx.  If the profile does not exist on the profile list
>> - * it is added.
>> + * by any task creds via invalidating the old version of the profile, which
>> + * tasks will notice to update their own cred.  If the profile does not exist
>> + * on the profile list it is added.
>>   *
>>   * Returns: size of data consumed else error code on failure.
>>   */
>> -- 
>> 2.9.3

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

[PATCH] apparmor: move task specific domain change info out of cred

[Stephen Smalley]

On Mon, 2017-03-13 at 10:05 -0700, John Johansen wrote:
> On 03/13/2017 09:47 AM, Serge E. Hallyn wrote:
> > Quoting John Johansen (john.johansen at canonical.com):
> > > Now that security_task_alloc() hook and "struct task_struct"-
> > > >security
> > > field are revived, move task specific domain change information
> > > for
> > > change_onexec (equiv of setexeccon) and change_hat out of the
> > > cred
> > > into a context off of the task_struct.
> > > 
> > > This cleans up apparmor's use of the cred structure so that it
> > > only
> > > carries the reference to current mediation.
> > > 
> > > Signed-off-by: John Johansen <john.johansen@canonical.com>
> > 
> > Thanks, John, that helps in compelling a review of the previous
> > patch :)
> > 
> > So the task_struct->security pointer is only to store requested
> > transition profiles right?
> > 
> 
> correct, well and support information for the transition like the
> random
> magic token for change_hat.

Is it really a net win for AA?  You save some space in the per-cred
structure (but that was already shared by most tasks, particularly any
that are not using change_onexec/change_hat), but won't you end up
using more space overall since you will now be allocating space for
(onexec, previous, token) for every task, even ones that don't use
those operations?

> 
> 
> > > ---
> > > ?security/apparmor/context.c?????????| 129 +++++++++++++++++-----
> > > --------------
> > > ?security/apparmor/domain.c??????????|??28 ++++----
> > > ?security/apparmor/include/context.h |??63 ++++++++----------
> > > ?security/apparmor/lsm.c?????????????|??65 ++++++++++--------
> > > ?security/apparmor/policy.c??????????|???5 +-
> > > ?5 files changed, 143 insertions(+), 147 deletions(-)
> > > 
> > > diff --git a/security/apparmor/context.c
> > > b/security/apparmor/context.c
> > > index 1fc16b8..8afb304 100644
> > > --- a/security/apparmor/context.c
> > > +++ b/security/apparmor/context.c
> > > @@ -13,11 +13,9 @@
> > > ? * License.
> > > ? *
> > > ? *
> > > - * AppArmor sets confinement on every task, via the the
> > > aa_task_ctx and
> > > - * the aa_task_ctx.profile, both of which are required and are
> > > not allowed
> > > - * to be NULL.??The aa_task_ctx is not reference counted and is
> > > unique
> > > - * to each cred (which is reference count).??The profile pointed
> > > to by
> > > - * the task_ctx is reference counted.
> > > + * AppArmor sets confinement on every task, via the
> > > cred_profile() which
> > > + * is required and is not allowed to be NULL.??The cred_profile
> > > is
> > > + * reference counted.
> > > ? *
> > > ? * TODO
> > > ? * If a task uses change_hat it currently does not return to the
> > > old
> > > @@ -29,25 +27,42 @@
> > > ?#include "include/context.h"
> > > ?#include "include/policy.h"
> > > ?
> > > +
> > > +/**
> > > + * aa_get_task_profile - Get another task's profile
> > > + * @task: task to query??(NOT NULL)
> > > + *
> > > + * Returns: counted reference to @task's profile
> > > + */
> > > +struct aa_profile *aa_get_task_profile(struct task_struct *task)
> > > +{
> > > +	struct aa_profile *p;
> > > +
> > > +	rcu_read_lock();
> > > +	p = aa_get_profile(__aa_task_profile(task));
> > > +	rcu_read_unlock();
> > > +
> > > +	return p;
> > > +}
> > > +
> > > ?/**
> > > - * aa_alloc_task_context - allocate a new task_ctx
> > > + * aa_alloc_task_ctx - allocate a new task_ctx
> > > ? * @flags: gfp flags for allocation
> > > ? *
> > > ? * Returns: allocated buffer or NULL on failure
> > > ? */
> > > -struct aa_task_ctx *aa_alloc_task_context(gfp_t flags)
> > > +struct aa_task_ctx *aa_alloc_task_ctx(gfp_t flags)
> > > ?{
> > > ?	return kzalloc(sizeof(struct aa_task_ctx), flags);
> > > ?}
> > > ?
> > > ?/**
> > > - * aa_free_task_context - free a task_ctx
> > > + * aa_free_task_ctx - free a task_ctx
> > > ? * @ctx: task_ctx to free (MAYBE NULL)
> > > ? */
> > > -void aa_free_task_context(struct aa_task_ctx *ctx)
> > > +void aa_free_task_ctx(struct aa_task_ctx *ctx)
> > > ?{
> > > ?	if (ctx) {
> > > -		aa_put_profile(ctx->profile);
> > > ?		aa_put_profile(ctx->previous);
> > > ?		aa_put_profile(ctx->onexec);
> > > ?
> > > @@ -56,36 +71,18 @@ void aa_free_task_context(struct aa_task_ctx
> > > *ctx)
> > > ?}
> > > ?
> > > ?/**
> > > - * aa_dup_task_context - duplicate a task context, incrementing
> > > reference counts
> > > + * aa_dup_task_ctx - duplicate a task context, incrementing
> > > reference counts
> > > ? * @new: a blank task context??????(NOT NULL)
> > > ? * @old: the task context to copy??(NOT NULL)
> > > ? */
> > > -void aa_dup_task_context(struct aa_task_ctx *new, const struct
> > > aa_task_ctx *old)
> > > +void aa_dup_task_ctx(struct aa_task_ctx *new, const struct
> > > aa_task_ctx *old)
> > > ?{
> > > ?	*new = *old;
> > > -	aa_get_profile(new->profile);
> > > ?	aa_get_profile(new->previous);
> > > ?	aa_get_profile(new->onexec);
> > > ?}
> > > ?
> > > ?/**
> > > - * aa_get_task_profile - Get another task's profile
> > > - * @task: task to query??(NOT NULL)
> > > - *
> > > - * Returns: counted reference to @task's profile
> > > - */
> > > -struct aa_profile *aa_get_task_profile(struct task_struct *task)
> > > -{
> > > -	struct aa_profile *p;
> > > -
> > > -	rcu_read_lock();
> > > -	p = aa_get_profile(__aa_task_profile(task));
> > > -	rcu_read_unlock();
> > > -
> > > -	return p;
> > > -}
> > > -
> > > -/**
> > > ? * aa_replace_current_profile - replace the current tasks
> > > profiles
> > > ? * @profile: new profile??(NOT NULL)
> > > ? *
> > > @@ -93,36 +90,37 @@ struct aa_profile *aa_get_task_profile(struct
> > > task_struct *task)
> > > ? */
> > > ?int aa_replace_current_profile(struct aa_profile *profile)
> > > ?{
> > > -	struct aa_task_ctx *ctx = current_ctx();
> > > +	struct aa_profile *old = __aa_current_profile();
> > > ?	struct cred *new;
> > > +
> > > ?	AA_BUG(!profile);
> > > ?
> > > -	if (ctx->profile == profile)
> > > +	if (old == profile)
> > > ?		return 0;
> > > ?
> > > ?	if (current_cred() != current_real_cred())
> > > ?		return -EBUSY;
> > > ?
> > > ?	new??= prepare_creds();
> > > +	old = cred_profile(new);
> > > ?	if (!new)
> > > ?		return -ENOMEM;
> > > ?
> > > -	ctx = cred_ctx(new);
> > > -	if (unconfined(profile) || (ctx->profile->ns != profile-
> > > >ns))
> > > +	if (unconfined(profile) || (old->ns != profile->ns))
> > > ?		/* if switching to unconfined or a different
> > > profile namespace
> > > ?		?* clear out context state
> > > ?		?*/
> > > -		aa_clear_task_ctx_trans(ctx);
> > > +		aa_clear_task_ctx(current_task_ctx());
> > > ?
> > > ?	/*
> > > -	?* be careful switching ctx->profile, when racing
> > > replacement it
> > > -	?* is possible that ctx->profile->proxy->profile is the
> > > reference
> > > +	?* be careful switching cred profile, when racing
> > > replacement it
> > > +	?* is possible that the cred profile's->proxy->profile
> > > is the reference
> > > ?	?* keeping @profile valid, so make sure to get its
> > > reference before
> > > -	?* dropping the reference on ctx->profile
> > > +	?* dropping the reference on the cred's profile
> > > ?	?*/
> > > ?	aa_get_profile(profile);
> > > -	aa_put_profile(ctx->profile);
> > > -	ctx->profile = profile;
> > > +	aa_put_profile(old);
> > > +	cred_profile(new) = profile;
> > > ?
> > > ?	commit_creds(new);
> > > ?	return 0;
> > > @@ -137,16 +135,12 @@ int aa_replace_current_profile(struct
> > > aa_profile *profile)
> > > ?int aa_set_current_onexec(struct aa_profile *profile)
> > > ?{
> > > ?	struct aa_task_ctx *ctx;
> > > -	struct cred *new = prepare_creds();
> > > -	if (!new)
> > > -		return -ENOMEM;
> > > ?
> > > -	ctx = cred_ctx(new);
> > > +	ctx = current_task_ctx();
> > > ?	aa_get_profile(profile);
> > > ?	aa_put_profile(ctx->onexec);
> > > ?	ctx->onexec = profile;
> > > ?
> > > -	commit_creds(new);
> > > ?	return 0;
> > > ?}
> > > ?
> > > @@ -162,25 +156,28 @@ int aa_set_current_onexec(struct aa_profile
> > > *profile)
> > > ? */
> > > ?int aa_set_current_hat(struct aa_profile *profile, u64 token)
> > > ?{
> > > -	struct aa_task_ctx *ctx;
> > > -	struct cred *new = prepare_creds();
> > > +	struct aa_task_ctx *ctx = current_task_ctx();
> > > +	struct cred *new;
> > > +
> > > +	AA_BUG(!profile);
> > > +
> > > +	new = prepare_creds();
> > > ?	if (!new)
> > > ?		return -ENOMEM;
> > > -	AA_BUG(!profile);
> > > ?
> > > -	ctx = cred_ctx(new);
> > > ?	if (!ctx->previous) {
> > > ?		/* transfer refcount */
> > > -		ctx->previous = ctx->profile;
> > > +		ctx->previous = cred_profile(new);
> > > ?		ctx->token = token;
> > > ?	} else if (ctx->token == token) {
> > > -		aa_put_profile(ctx->profile);
> > > +		aa_put_profile(cred_profile(new));
> > > ?	} else {
> > > ?		/* previous_profile && ctx->token != token */
> > > ?		abort_creds(new);
> > > ?		return -EACCES;
> > > ?	}
> > > -	ctx->profile = aa_get_newest_profile(profile);
> > > +
> > > +	cred_profile(new) = aa_get_newest_profile(profile);
> > > ?	/* clear exec on switching context */
> > > ?	aa_put_profile(ctx->onexec);
> > > ?	ctx->onexec = NULL;
> > > @@ -200,28 +197,26 @@ int aa_set_current_hat(struct aa_profile
> > > *profile, u64 token)
> > > ? */
> > > ?int aa_restore_previous_profile(u64 token)
> > > ?{
> > > -	struct aa_task_ctx *ctx;
> > > -	struct cred *new = prepare_creds();
> > > -	if (!new)
> > > -		return -ENOMEM;
> > > +	struct aa_task_ctx *ctx = current_task_ctx();
> > > +	struct cred *new;
> > > ?
> > > -	ctx = cred_ctx(new);
> > > -	if (ctx->token != token) {
> > > -		abort_creds(new);
> > > +	if (ctx->token != token)
> > > ?		return -EACCES;
> > > -	}
> > > ?	/* ignore restores when there is no saved profile */
> > > -	if (!ctx->previous) {
> > > -		abort_creds(new);
> > > +	if (!ctx->previous)
> > > ?		return 0;
> > > -	}
> > > ?
> > > -	aa_put_profile(ctx->profile);
> > > -	ctx->profile = aa_get_newest_profile(ctx->previous);
> > > -	AA_BUG(!ctx->profile);
> > > +	new = prepare_creds();
> > > +	if (!new)
> > > +		return -ENOMEM;
> > > +
> > > +	aa_put_profile(cred_profile(new));
> > > +	cred_profile(new) = aa_get_newest_profile(ctx-
> > > >previous);
> > > +	AA_BUG(!cred_profile(new));
> > > ?	/* clear exec && prev information when restoring to
> > > previous context */
> > > -	aa_clear_task_ctx_trans(ctx);
> > > +	aa_clear_task_ctx(ctx);
> > > ?
> > > ?	commit_creds(new);
> > > +
> > > ?	return 0;
> > > ?}
> > > diff --git a/security/apparmor/domain.c
> > > b/security/apparmor/domain.c
> > > index ef4beef..1994c02 100644
> > > --- a/security/apparmor/domain.c
> > > +++ b/security/apparmor/domain.c
> > > @@ -353,10 +353,10 @@ int apparmor_bprm_set_creds(struct
> > > linux_binprm *bprm)
> > > ?	if (bprm->cred_prepared)
> > > ?		return 0;
> > > ?
> > > -	ctx = cred_ctx(bprm->cred);
> > > +	ctx = current_task_ctx();
> > > ?	AA_BUG(!ctx);
> > > -
> > > -	profile = aa_get_newest_profile(ctx->profile);
> > > +	AA_BUG(!cred_profile(bprm->cred));
> > > +	profile = aa_get_newest_profile(cred_profile(bprm-
> > > >cred));
> > > ?	/*
> > > ?	?* get the namespace from the replacement profile as
> > > replacement
> > > ?	?* can change the namespace
> > > @@ -499,14 +499,11 @@ int apparmor_bprm_set_creds(struct
> > > linux_binprm *bprm)
> > > ?	bprm->per_clear |= PER_CLEAR_ON_SETID;
> > > ?
> > > ?x_clear:
> > > -	aa_put_profile(ctx->profile);
> > > -	/* transfer new profile reference will be released when
> > > ctx is freed */
> > > -	ctx->profile = new_profile;
> > > +	aa_put_profile(profile);
> > > +	/* transfer new profile reference will be released when
> > > cred is freed */
> > > +	cred_profile(bprm->cred) = new_profile;
> > > ?	new_profile = NULL;
> > > ?
> > > -	/* clear out all temporary/transitional state from the
> > > context */
> > > -	aa_clear_task_ctx_trans(ctx);
> > > -
> > > ?audit:
> > > ?	error = aa_audit_file(profile, &perms, OP_EXEC,
> > > MAY_EXEC, name,
> > > ?			??????new_profile ? new_profile-
> > > >base.hname : NULL,
> > > @@ -544,17 +541,16 @@ int apparmor_bprm_secureexec(struct
> > > linux_binprm *bprm)
> > > ?void apparmor_bprm_committing_creds(struct linux_binprm *bprm)
> > > ?{
> > > ?	struct aa_profile *profile = __aa_current_profile();
> > > -	struct aa_task_ctx *new_ctx = cred_ctx(bprm->cred);
> > > +	struct aa_profile *new = cred_profile(bprm->cred);
> > > ?
> > > ?	/* bail out if unconfined or not changing profile */
> > > -	if ((new_ctx->profile == profile) ||
> > > -	????(unconfined(new_ctx->profile)))
> > > +	if (new == profile || unconfined(new))
> > > ?		return;
> > > ?
> > > ?	current->pdeath_signal = 0;
> > > ?
> > > ?	/* reset soft limits and set hard limits for the new
> > > profile */
> > > -	__aa_transition_rlimits(profile, new_ctx->profile);
> > > +	__aa_transition_rlimits(profile, new);
> > > ?}
> > > ?
> > > ?/**
> > > @@ -564,6 +560,10 @@ void apparmor_bprm_committing_creds(struct
> > > linux_binprm *bprm)
> > > ?void apparmor_bprm_committed_creds(struct linux_binprm *bprm)
> > > ?{
> > > ?	/* TODO: cleanup signals - ipc mediation */
> > > +
> > > +	/* clear out all temporary/transitional state from the
> > > context */
> > > +	aa_clear_task_ctx(current_task_ctx());
> > > +
> > > ?	return;
> > > ?}
> > > ?
> > > @@ -621,7 +621,7 @@ int aa_change_hat(const char *hats[], int
> > > count, u64 token, bool permtest)
> > > ?
> > > ?	/* released below */
> > > ?	cred = get_current_cred();
> > > -	ctx = cred_ctx(cred);
> > > +	ctx = current_task_ctx();
> > > ?	profile = aa_get_newest_profile(aa_cred_profile(cred));
> > > ?	previous_profile = aa_get_newest_profile(ctx->previous);
> > > ?
> > > diff --git a/security/apparmor/include/context.h
> > > b/security/apparmor/include/context.h
> > > index 5b18fed..9943969 100644
> > > --- a/security/apparmor/include/context.h
> > > +++ b/security/apparmor/include/context.h
> > > @@ -22,8 +22,9 @@
> > > ?#include "policy.h"
> > > ?#include "policy_ns.h"
> > > ?
> > > -#define cred_ctx(X) ((X)->security)
> > > -#define current_ctx() cred_ctx(current_cred())
> > > +#define task_ctx(X) ((X)->security)
> > > +#define current_task_ctx() (task_ctx(current))
> > > +#define cred_profile(X) ((X)->security)
> > > ?
> > > ?/* struct aa_file_ctx - the AppArmor context the file was opened
> > > in
> > > ? * @perms: the permission the file was opened with
> > > @@ -58,28 +59,23 @@ static inline void
> > > aa_free_file_context(struct aa_file_ctx *ctx)
> > > ?}
> > > ?
> > > ?/**
> > > - * struct aa_task_ctx - primary label for confined tasks
> > > - * @profile: the current profile???(NOT NULL)
> > > - * @exec: profile to transition to on next exec??(MAYBE NULL)
> > > - * @previous: profile the task may return to?????(MAYBE NULL)
> > > + * struct aa_task_ctx - information for current task label
> > > change
> > > + * @onexec: profile to transition to on next exec??(MAY BE NULL)
> > > + * @previous: profile the task may return to?????(MAY BE NULL)
> > > ? * @token: magic value the task must know for returning to
> > > @previous_profile
> > > ? *
> > > - * Contains the task's current profile (which could change due
> > > to
> > > - * change_hat).??Plus the hat_magic needed during change_hat.
> > > - *
> > > ? * TODO: make so a task can be confined by a stack of contexts
> > > ? */
> > > ?struct aa_task_ctx {
> > > -	struct aa_profile *profile;
> > > ?	struct aa_profile *onexec;
> > > ?	struct aa_profile *previous;
> > > ?	u64 token;
> > > ?};
> > > ?
> > > -struct aa_task_ctx *aa_alloc_task_context(gfp_t flags);
> > > -void aa_free_task_context(struct aa_task_ctx *ctx);
> > > -void aa_dup_task_context(struct aa_task_ctx *new,
> > > -			?const struct aa_task_ctx *old);
> > > +struct aa_task_ctx *aa_alloc_task_ctx(gfp_t flags);
> > > +void aa_free_task_ctx(struct aa_task_ctx *ctx);
> > > +void aa_dup_task_ctx(struct aa_task_ctx *new, const struct
> > > aa_task_ctx *old);
> > > +
> > > ?int aa_replace_current_profile(struct aa_profile *profile);
> > > ?int aa_set_current_onexec(struct aa_profile *profile);
> > > ?int aa_set_current_hat(struct aa_profile *profile, u64 token);
> > > @@ -97,10 +93,11 @@ struct aa_profile *aa_get_task_profile(struct
> > > task_struct *task);
> > > ? */
> > > ?static inline struct aa_profile *aa_cred_profile(const struct
> > > cred *cred)
> > > ?{
> > > -	struct aa_task_ctx *ctx = cred_ctx(cred);
> > > +	struct aa_profile *profile = cred_profile(cred);
> > > ?
> > > -	AA_BUG(!ctx || !ctx->profile);
> > > -	return ctx->profile;
> > > +	AA_BUG(!profile);
> > > +
> > > +	return profile;
> > > ?}
> > > ?
> > > ?/**
> > > @@ -117,17 +114,6 @@ static inline struct aa_profile
> > > *__aa_task_profile(struct task_struct *task)
> > > ?}
> > > ?
> > > ?/**
> > > - * __aa_task_is_confined - determine if @task has any
> > > confinement
> > > - * @task: task to check confinement of??(NOT NULL)
> > > - *
> > > - * If @task != current needs to be called in RCU safe critical
> > > section
> > > - */
> > > -static inline bool __aa_task_is_confined(struct task_struct
> > > *task)
> > > -{
> > > -	return !unconfined(__aa_task_profile(task));
> > > -}
> > > -
> > > -/**
> > > ? * __aa_current_profile - find the current tasks confining
> > > profile
> > > ? *
> > > ? * Returns: up to date confining profile or the ns unconfined
> > > profile (NOT NULL)
> > > @@ -150,19 +136,17 @@ static inline struct aa_profile
> > > *__aa_current_profile(void)
> > > ? */
> > > ?static inline struct aa_profile *aa_current_profile(void)
> > > ?{
> > > -	const struct aa_task_ctx *ctx = current_ctx();
> > > -	struct aa_profile *profile;
> > > +	struct aa_profile *profile = __aa_current_profile();
> > > ?
> > > -	AA_BUG(!ctx || !ctx->profile);
> > > +	AA_BUG(!profile);
> > > ?
> > > -	if (profile_is_stale(ctx->profile)) {
> > > -		profile = aa_get_newest_profile(ctx->profile);
> > > +	if (profile_is_stale(profile)) {
> > > +		profile = aa_get_newest_profile(profile);
> > > ?		aa_replace_current_profile(profile);
> > > ?		aa_put_profile(profile);
> > > -		ctx = current_ctx();
> > > ?	}
> > > ?
> > > -	return ctx->profile;
> > > +	return profile;
> > > ?}
> > > ?
> > > ?static inline struct aa_ns *aa_get_current_ns(void)
> > > @@ -170,12 +154,17 @@ static inline struct aa_ns
> > > *aa_get_current_ns(void)
> > > ?	return aa_get_ns(__aa_current_profile()->ns);
> > > ?}
> > > ?
> > > +
> > > +
> > > +
> > > ?/**
> > > - * aa_clear_task_ctx_trans - clear transition tracking info from
> > > the ctx
> > > + * aa_clear_task_ctx - clear transition tracking info from the
> > > ctx
> > > ? * @ctx: task context to clear (NOT NULL)
> > > ? */
> > > -static inline void aa_clear_task_ctx_trans(struct aa_task_ctx
> > > *ctx)
> > > +static inline void aa_clear_task_ctx(struct aa_task_ctx *ctx)
> > > ?{
> > > +	AA_BUG(!ctx);
> > > +
> > > ?	aa_put_profile(ctx->previous);
> > > ?	aa_put_profile(ctx->onexec);
> > > ?	ctx->previous = NULL;
> > > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > > index 1f2000d..ed9bf71 100644
> > > --- a/security/apparmor/lsm.c
> > > +++ b/security/apparmor/lsm.c
> > > @@ -49,12 +49,12 @@ DEFINE_PER_CPU(struct aa_buffers,
> > > aa_buffers);
> > > ? */
> > > ?
> > > ?/*
> > > - * free the associated aa_task_ctx and put its profiles
> > > + * put the associated profiles
> > > ? */
> > > ?static void apparmor_cred_free(struct cred *cred)
> > > ?{
> > > -	aa_free_task_context(cred_ctx(cred));
> > > -	cred_ctx(cred) = NULL;
> > > +	aa_put_profile(cred_profile(cred));
> > > +	cred_profile(cred) = NULL;
> > > ?}
> > > ?
> > > ?/*
> > > @@ -62,30 +62,19 @@ static void apparmor_cred_free(struct cred
> > > *cred)
> > > ? */
> > > ?static int apparmor_cred_alloc_blank(struct cred *cred, gfp_t
> > > gfp)
> > > ?{
> > > -	/* freed by apparmor_cred_free */
> > > -	struct aa_task_ctx *ctx = aa_alloc_task_context(gfp);
> > > -
> > > -	if (!ctx)
> > > -		return -ENOMEM;
> > > -
> > > -	cred_ctx(cred) = ctx;
> > > +	cred_profile(cred) = NULL;
> > > ?	return 0;
> > > ?}
> > > ?
> > > ?/*
> > > - * prepare new aa_task_ctx for modification by prepare_cred
> > > block
> > > + * prepare new cred profile for modification by prepare_cred
> > > block
> > > ? */
> > > ?static int apparmor_cred_prepare(struct cred *new, const struct
> > > cred *old,
> > > ?				?gfp_t gfp)
> > > ?{
> > > -	/* freed by apparmor_cred_free */
> > > -	struct aa_task_ctx *ctx = aa_alloc_task_context(gfp);
> > > -
> > > -	if (!ctx)
> > > -		return -ENOMEM;
> > > -
> > > -	aa_dup_task_context(ctx, cred_ctx(old));
> > > -	cred_ctx(new) = ctx;
> > > +	struct aa_profile *tmp = cred_profile(new);
> > > +	cred_profile(new) =
> > > aa_get_newest_profile(cred_profile(old));
> > > +	aa_put_profile(tmp);
> > > ?	return 0;
> > > ?}
> > > ?
> > > @@ -94,10 +83,30 @@ static int apparmor_cred_prepare(struct cred
> > > *new, const struct cred *old,
> > > ? */
> > > ?static void apparmor_cred_transfer(struct cred *new, const
> > > struct cred *old)
> > > ?{
> > > -	const struct aa_task_ctx *old_ctx = cred_ctx(old);
> > > -	struct aa_task_ctx *new_ctx = cred_ctx(new);
> > > +	struct aa_profile *tmp = cred_profile(new);
> > > +	cred_profile(new) =
> > > aa_get_newest_profile(cred_profile(old));
> > > +	aa_put_profile(tmp);
> > > +}
> > > ?
> > > -	aa_dup_task_context(new_ctx, old_ctx);
> > > +static void apparmor_task_free(struct task_struct *task)
> > > +{
> > > +
> > > +	aa_free_task_ctx(task_ctx(task));
> > > +	task_ctx(task) = NULL;
> > > +}
> > > +
> > > +static int apparmor_task_alloc(struct task_struct *task,
> > > +			???????unsigned long clone_flags)
> > > +{
> > > +	struct aa_task_ctx *new = aa_alloc_task_ctx(GFP_KERNEL);
> > > +
> > > +	if (!new)
> > > +		return -ENOMEM;
> > > +
> > > +	aa_dup_task_ctx(new, current_task_ctx());
> > > +	task_ctx(task) = new;
> > > +
> > > +	return 0;
> > > ?}
> > > ?
> > > ?static int apparmor_ptrace_access_check(struct task_struct
> > > *child,
> > > @@ -484,11 +493,11 @@ static int apparmor_getprocattr(struct
> > > task_struct *task, char *name,
> > > ?	int error = -ENOENT;
> > > ?	/* released below */
> > > ?	const struct cred *cred = get_task_cred(task);
> > > -	struct aa_task_ctx *ctx = cred_ctx(cred);
> > > +	struct aa_task_ctx *ctx = current_task_ctx();
> > > ?	struct aa_profile *profile = NULL;
> > > ?
> > > ?	if (strcmp(name, "current") == 0)
> > > -		profile = aa_get_newest_profile(ctx->profile);
> > > +		profile =
> > > aa_get_newest_profile(cred_profile(cred));
> > > ?	else if (strcmp(name, "prev") == 0??&& ctx->previous)
> > > ?		profile = aa_get_newest_profile(ctx->previous);
> > > ?	else if (strcmp(name, "exec") == 0 && ctx->onexec)
> > > @@ -629,6 +638,8 @@ static struct security_hook_list
> > > apparmor_hooks[] = {
> > > ?	LSM_HOOK_INIT(bprm_committed_creds,
> > > apparmor_bprm_committed_creds),
> > > ?	LSM_HOOK_INIT(bprm_secureexec,
> > > apparmor_bprm_secureexec),
> > > ?
> > > +	LSM_HOOK_INIT(task_free, apparmor_task_free),
> > > +	LSM_HOOK_INIT(task_alloc, apparmor_task_alloc),
> > > ?	LSM_HOOK_INIT(task_setrlimit, apparmor_task_setrlimit),
> > > ?};
> > > ?
> > > @@ -871,12 +882,12 @@ static int __init set_init_ctx(void)
> > > ?	struct cred *cred = (struct cred *)current->real_cred;
> > > ?	struct aa_task_ctx *ctx;
> > > ?
> > > -	ctx = aa_alloc_task_context(GFP_KERNEL);
> > > +	ctx = aa_alloc_task_ctx(GFP_KERNEL);
> > > ?	if (!ctx)
> > > ?		return -ENOMEM;
> > > ?
> > > -	ctx->profile = aa_get_profile(root_ns->unconfined);
> > > -	cred_ctx(cred) = ctx;
> > > +	cred_profile(cred) = aa_get_profile(root_ns-
> > > >unconfined);
> > > +	task_ctx(current) = ctx;
> > > ?
> > > ?	return 0;
> > > ?}
> > > diff --git a/security/apparmor/policy.c
> > > b/security/apparmor/policy.c
> > > index f44312a..b9c300b 100644
> > > --- a/security/apparmor/policy.c
> > > +++ b/security/apparmor/policy.c
> > > @@ -827,8 +827,9 @@ static int __lookup_replace(struct aa_ns *ns,
> > > const char *hname,
> > > ? * @udata: serialized data stream??(NOT NULL)
> > > ? *
> > > ? * unpack and replace a profile on the profile list and uses of
> > > that profile
> > > - * by any aa_task_ctx.??If the profile does not exist on the
> > > profile list
> > > - * it is added.
> > > + * by any task creds via invalidating the old version of the
> > > profile, which
> > > + * tasks will notice to update their own cred.??If the profile
> > > does not exist
> > > + * on the profile list it is added.
> > > ? *
> > > ? * Returns: size of data consumed else error code on failure.
> > > ? */
> > > --?
> > > 2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] apparmor: move task specific domain change info out of cred

[John Johansen]

On 03/13/2017 10:16 AM, Stephen Smalley wrote:
> On Mon, 2017-03-13 at 10:05 -0700, John Johansen wrote:
>> On 03/13/2017 09:47 AM, Serge E. Hallyn wrote:
>>> Quoting John Johansen (john.johansen at canonical.com):
>>>> Now that security_task_alloc() hook and "struct task_struct"-
>>>>> security
>>>> field are revived, move task specific domain change information
>>>> for
>>>> change_onexec (equiv of setexeccon) and change_hat out of the
>>>> cred
>>>> into a context off of the task_struct.
>>>>
>>>> This cleans up apparmor's use of the cred structure so that it
>>>> only
>>>> carries the reference to current mediation.
>>>>
>>>> Signed-off-by: John Johansen <john.johansen@canonical.com>
>>>
>>> Thanks, John, that helps in compelling a review of the previous
>>> patch :)
>>>
>>> So the task_struct->security pointer is only to store requested
>>> transition profiles right?
>>>
>>
>> correct, well and support information for the transition like the
>> random
>> magic token for change_hat.
> 
> Is it really a net win for AA?  You save some space in the per-cred
> structure (but that was already shared by most tasks, particularly any
> that are not using change_onexec/change_hat), but won't you end up
> using more space overall since you will now be allocating space for
> (onexec, previous, token) for every task, even ones that don't use
> those operations?
> 
atm its more of a wash. Its a win for cred related operations but
allocating per task, instead of per cred does increase memory usage.
However this is just a first pass that is a fairly direct mapping of
onto the current code paths. There is no reason the task->security
struct couldn't be shared by tasks, or even more likely not allocated
at all except by those tasks using change_onexec/change_hat.

I just haven't had a chance to work on the improvements, long term it
will be a win for AA.


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

[PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

[Tetsuo Handa]

Serge E. Hallyn wrote:
> > @@ -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.
> 
> Note that here you have spaces not tabs
> 

Oh, thanks.

I think this patch got enough Acked-by:s.

  Serge Hallyn <serge@hallyn.com>
  Casey Schaufler <casey@schaufler-ca.com>

James, should I repost with spaces fixed?

Also, I appreciate if you can update #next to 4.11-rc2 because
4.11-rc1 is not suitable for testing due to memory corruption bug.
http://lkml.kernel.org/r/201703090625.v296PNFI070895 at www262.sakura.ne.jp
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

[Tetsuo Handa]

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>
Acked-by: John Johansen <john.johansen@canonical.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Tested-by: Djalal Harouni <tixxdz@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Eric Paris <eparis@parisplace.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: James Morris <james.l.morris@oracle.com>
Cc: Jos? Bollo <jobol@nonadev.net>
---
 include/linux/init_task.h | 7 +++++++
 include/linux/lsm_hooks.h | 9 ++++++++-
 include/linux/sched.h     | 4 ++++
 include/linux/security.h  | 7 +++++++
 kernel/fork.c             | 7 ++++++-
 security/security.c       | 6 ++++++
 6 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index 91d9049..926f2f5 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -210,6 +210,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)
@@ -288,6 +294,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 1aa6333..a4193c3 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:
@@ -1482,6 +1487,7 @@
 	int (*file_open)(struct file *file, const struct cred *cred);
 
 	int (*task_create)(unsigned long clone_flags);
+	int (*task_alloc)(struct task_struct *task, unsigned long clone_flags);
 	void (*task_free)(struct task_struct *task);
 	int (*cred_alloc_blank)(struct cred *cred, gfp_t gfp);
 	void (*cred_free)(struct cred *cred);
@@ -1748,6 +1754,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 d67eee8..71b8df3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1038,6 +1038,10 @@ struct task_struct {
 	/* A live task holds one reference: */
 	atomic_t			stack_refcount;
 #endif
+#ifdef CONFIG_SECURITY
+	/* Used by LSM modules for access restriction: */
+	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 97df7ba..af675b5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -308,6 +308,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);
@@ -861,6 +862,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 6c463c80..3d32513 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1679,9 +1679,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;
@@ -1903,6 +1906,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 d6d18a3..5b6cdd0 100644
--- a/security/security.c
+++ b/security/security.c
@@ -930,6 +930,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);
@@ -1770,6 +1775,7 @@ struct security_hook_heads security_hook_heads __lsm_ro_after_init = {
 	.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),
-- 
1.8.3.1

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

[PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

[Tetsuo Handa]

James Morris wrote:
> On Fri, 24 Mar 2017, Tetsuo Handa wrote:
> 
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > Acked-by: John Johansen <john.johansen@canonical.com>
> > Acked-by: Serge Hallyn <serge@hallyn.com>
> > Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> > Tested-by: Djalal Harouni <tixxdz@gmail.com>
> 
> Applied to
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next
> 
> I also synced to a newer -rc (4) as requested.
> 
> Please test!

Thank you very much. I confirmed that it passed TOMOYO's all testcases.
A patch which depends on this change follows.
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

[James Morris]

On Fri, 24 Mar 2017, Tetsuo Handa wrote:

> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Acked-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>
> Tested-by: Djalal Harouni <tixxdz@gmail.com>

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/linux-security.git next

I also synced to a newer -rc (4) as requested.

Please test!


-- 
James Morris
<jmorris@namei.org>

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

[PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

[James Morris]

On Sun, 26 Mar 2017, Tetsuo Handa wrote:

> James, would you teach me why this patch still cannot go to linux-next?
> If we don't merge this patch now, we will again fail to land in 4.12.

I'm still reviewing it.


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

-- 
James Morris
<jmorris@namei.org>

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

[PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

[Tetsuo Handa]

James, would you teach me why this patch still cannot go to linux-next?
If we don't merge this patch now, we will again fail to land in 4.12.

> 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

[Tetsuo Handa]

Jose Bollo wrote:
> Acked-by: Jose Bollo <jobol@nonadev.net>
> 
> (if it cares ;)

Thanks. I reposted this patch in case James missed this patch.

> 
> Also below, I expect in some future that task_alloc and task_create
> will be merged. IMHO not allocating the task is of less importance than
> having a simple and unique hook. Statistically the normal is "the
> task is allowed" so the cost of creating the task structure for
> nothing is only relevant in cases where efficiency is just not expected.
> 
You have below post in your mail box. ;-)

------- Forwarded Message
From: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
To: casey at schaufler-ca.com, jobol at nonadev.net
Cc: john.johansen at canonical.com, paul at paul-moore.com, sds at tycho.nsa.gov,eparis at parisplace.org, keescook at chromium.org,james.l.morris at oracle.com, serge at hallyn.com,linux-security-module at vger.kernel.org
Subject: Re: [PATCH] LSM: Revive security_task_alloc() hook.
Date: Fri, 6 Jan 2017 20:35:16 +0900

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

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

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

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

[PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

[José Bollo]

On Fri, 24 Mar 2017 20:46:33 +0900
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> We switched from "struct task_struct"->security to "struct
> cred"->security in Linux 2.6.29. But not all LSM modules were happy
> with that change. TOMOYO LSM module is an example which want to use
> per "struct task_struct" security blob, for TOMOYO's security context
> is defined based on "struct task_struct" rather than "struct cred".
> AppArmor LSM module is another example which want to use it, for
> AppArmor is currently abusing the cred a little bit to store the
> change_hat and setexeccon info. Although security_task_free() hook
> was revived in Linux 3.4 because Yama LSM module wanted to release
> per "struct task_struct" security blob, security_task_alloc() hook
> and "struct task_struct"->security field were not revived. Nowadays,
> we are getting proposals of lightweight LSM modules which want to use
> per "struct task_struct" security blob. PTAGS LSM module and CaitSith
> LSM module which are currently under proposal for inclusion also want
> to use it. Therefore, it will be time to revive security_task_alloc()
> hook 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>
> Acked-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> Acked-by: Casey Schaufler <casey@schaufler-ca.com>

Acked-by: Jos? Bollo <jobol@nonadev.net>

(if it cares ;)

Also below, I expect in some future that task_alloc and task_create
will be merged. IMHO not allocating the task is of less importance than
having a simple and unique hook. Statistically the normal is "the
task is allowed" so the cost of creating the task structure for
nothing is only relevant in cases where efficiency is just not expected.


> Tested-by: Djalal Harouni <tixxdz@gmail.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Stephen Smalley <sds@tycho.nsa.gov>
> Cc: Eric Paris <eparis@parisplace.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: James Morris <james.l.morris@oracle.com>
> Cc: Jos? Bollo <jobol@nonadev.net>
> ---
>  include/linux/init_task.h | 7 +++++++
>  include/linux/lsm_hooks.h | 9 ++++++++-
>  include/linux/sched.h     | 4 ++++
>  include/linux/security.h  | 7 +++++++
>  kernel/fork.c             | 7 ++++++-
>  security/security.c       | 5 +++++
>  6 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/init_task.h b/include/linux/init_task.h
> index 91d9049..926f2f5 100644
> --- a/include/linux/init_task.h
> +++ b/include/linux/init_task.h
> @@ -210,6 +210,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)
> @@ -288,6 +294,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 54191cf..865c11d 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:
> @@ -1482,6 +1487,7 @@
>  	int (*file_open)(struct file *file, const struct cred *cred);
>  
>  	int (*task_create)(unsigned long clone_flags);
> +	int (*task_alloc)(struct task_struct *task, unsigned long
> clone_flags); void (*task_free)(struct task_struct *task);
>  	int (*cred_alloc_blank)(struct cred *cred, gfp_t gfp);
>  	void (*cred_free)(struct cred *cred);
> @@ -1748,6 +1754,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 d67eee8..71b8df3 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -1038,6 +1038,10 @@ struct task_struct {
>  	/* A live task holds one reference: */
>  	atomic_t			stack_refcount;
>  #endif
> +#ifdef CONFIG_SECURITY
> +	/* Used by LSM modules for access restriction: */
> +	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 97df7ba..af675b5 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -308,6 +308,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);
> @@ -861,6 +862,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 6c463c80..3d32513 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1679,9 +1679,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;
> @@ -1903,6 +1906,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 45af8fb..8c62fc3 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -946,6 +946,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);

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

[PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.

[Tetsuo Handa]

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>
Acked-by: John Johansen <john.johansen@canonical.com>
Acked-by: Serge Hallyn <serge@hallyn.com>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
Tested-by: Djalal Harouni <tixxdz@gmail.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Stephen Smalley <sds@tycho.nsa.gov>
Cc: Eric Paris <eparis@parisplace.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: James Morris <james.l.morris@oracle.com>
Cc: Jos? Bollo <jobol@nonadev.net>
---
 include/linux/init_task.h | 7 +++++++
 include/linux/lsm_hooks.h | 9 ++++++++-
 include/linux/sched.h     | 4 ++++
 include/linux/security.h  | 7 +++++++
 kernel/fork.c             | 7 ++++++-
 security/security.c       | 5 +++++
 6 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/include/linux/init_task.h b/include/linux/init_task.h
index 91d9049..926f2f5 100644
--- a/include/linux/init_task.h
+++ b/include/linux/init_task.h
@@ -210,6 +210,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)
@@ -288,6 +294,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 54191cf..865c11d 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:
@@ -1482,6 +1487,7 @@
 	int (*file_open)(struct file *file, const struct cred *cred);
 
 	int (*task_create)(unsigned long clone_flags);
+	int (*task_alloc)(struct task_struct *task, unsigned long clone_flags);
 	void (*task_free)(struct task_struct *task);
 	int (*cred_alloc_blank)(struct cred *cred, gfp_t gfp);
 	void (*cred_free)(struct cred *cred);
@@ -1748,6 +1754,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 d67eee8..71b8df3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1038,6 +1038,10 @@ struct task_struct {
 	/* A live task holds one reference: */
 	atomic_t			stack_refcount;
 #endif
+#ifdef CONFIG_SECURITY
+	/* Used by LSM modules for access restriction: */
+	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 97df7ba..af675b5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -308,6 +308,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);
@@ -861,6 +862,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 6c463c80..3d32513 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1679,9 +1679,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;
@@ -1903,6 +1906,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 45af8fb..8c62fc3 100644
--- a/security/security.c
+++ b/security/security.c
@@ -946,6 +946,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);
-- 
1.8.3.1

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