On Mon, 2017-03-13 at 10:05 -0700, John Johansen wrote:

On 03/13/2017 09:47 AM, Serge E. Hallyn wrote:

quoted

Quoting John Johansen (john.johansen at canonical.com):

quoted

Now that security_task_alloc() hook and "struct task_struct"-

quoted

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?

quoted

quoted

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

quoted

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-

quoted

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-

quoted

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-

quoted

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-

quoted

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