[PATCH] apparmor: move task specific domain change info out of cred
Serge E. Hallyn
serge at hallyn.com
Mon Mar 13 16:47:10 UTC 2017
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 at 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
More information about the Linux-security-module-archive
mailing list