* [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.
@ 2017-03-12 4:35 Tetsuo Handa
2017-03-13 6:32 ` [PATCH] apparmor: move task specific domain change info out of cred John Johansen
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Tetsuo Handa @ 2017-03-12 4:35 UTC (permalink / raw)
To: linux-security-module
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] apparmor: move task specific domain change info out of cred
2017-03-12 4:35 [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob Tetsuo Handa
@ 2017-03-13 6:32 ` John Johansen
2017-03-13 16:47 ` Serge E. Hallyn
2017-03-13 15:37 ` [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob Serge E. Hallyn
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: John Johansen @ 2017-03-13 6:32 UTC (permalink / raw)
To: linux-security-module
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.
2017-03-12 4:35 [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob Tetsuo Handa
2017-03-13 6:32 ` [PATCH] apparmor: move task specific domain change info out of cred John Johansen
@ 2017-03-13 15:37 ` Serge E. Hallyn
2017-03-16 11:09 ` Tetsuo Handa
2017-03-13 16:08 ` Casey Schaufler
2017-03-17 10:37 ` Tetsuo Handa
3 siblings, 1 reply; 10+ messages in thread
From: Serge E. Hallyn @ 2017-03-13 15:37 UTC (permalink / raw)
To: linux-security-module
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.
2017-03-12 4:35 [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob Tetsuo Handa
2017-03-13 6:32 ` [PATCH] apparmor: move task specific domain change info out of cred John Johansen
2017-03-13 15:37 ` [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob Serge E. Hallyn
@ 2017-03-13 16:08 ` Casey Schaufler
2017-03-17 10:37 ` Tetsuo Handa
3 siblings, 0 replies; 10+ messages in thread
From: Casey Schaufler @ 2017-03-13 16:08 UTC (permalink / raw)
To: linux-security-module
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] apparmor: move task specific domain change info out of cred
2017-03-13 6:32 ` [PATCH] apparmor: move task specific domain change info out of cred John Johansen
@ 2017-03-13 16:47 ` Serge E. Hallyn
2017-03-13 17:05 ` John Johansen
0 siblings, 1 reply; 10+ messages in thread
From: Serge E. Hallyn @ 2017-03-13 16:47 UTC (permalink / raw)
To: linux-security-module
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] apparmor: move task specific domain change info out of cred
2017-03-13 16:47 ` Serge E. Hallyn
@ 2017-03-13 17:05 ` John Johansen
2017-03-13 17:16 ` Stephen Smalley
0 siblings, 1 reply; 10+ messages in thread
From: John Johansen @ 2017-03-13 17:05 UTC (permalink / raw)
To: linux-security-module
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] apparmor: move task specific domain change info out of cred
2017-03-13 17:05 ` John Johansen
@ 2017-03-13 17:16 ` Stephen Smalley
2017-03-13 17:50 ` John Johansen
0 siblings, 1 reply; 10+ messages in thread
From: Stephen Smalley @ 2017-03-13 17:16 UTC (permalink / raw)
To: linux-security-module
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] apparmor: move task specific domain change info out of cred
2017-03-13 17:16 ` Stephen Smalley
@ 2017-03-13 17:50 ` John Johansen
0 siblings, 0 replies; 10+ messages in thread
From: John Johansen @ 2017-03-13 17:50 UTC (permalink / raw)
To: linux-security-module
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.
2017-03-13 15:37 ` [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob Serge E. Hallyn
@ 2017-03-16 11:09 ` Tetsuo Handa
0 siblings, 0 replies; 10+ messages in thread
From: Tetsuo Handa @ 2017-03-16 11:09 UTC (permalink / raw)
To: linux-security-module
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob.
2017-03-12 4:35 [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob Tetsuo Handa
` (2 preceding siblings ...)
2017-03-13 16:08 ` Casey Schaufler
@ 2017-03-17 10:37 ` Tetsuo Handa
3 siblings, 0 replies; 10+ messages in thread
From: Tetsuo Handa @ 2017-03-17 10:37 UTC (permalink / raw)
To: linux-security-module
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-17 10:37 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-12 4:35 [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob Tetsuo Handa
2017-03-13 6:32 ` [PATCH] apparmor: move task specific domain change info out of cred John Johansen
2017-03-13 16:47 ` Serge E. Hallyn
2017-03-13 17:05 ` John Johansen
2017-03-13 17:16 ` Stephen Smalley
2017-03-13 17:50 ` John Johansen
2017-03-13 15:37 ` [PATCH] LSM: Revive security_task_alloc() hook and per "struct task_struct" security blob Serge E. Hallyn
2017-03-16 11:09 ` Tetsuo Handa
2017-03-13 16:08 ` Casey Schaufler
2017-03-17 10:37 ` Tetsuo Handa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).