From cf797c0e5e312520b0b9f0367039fc0279a07a76 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 9 Jun 2017 02:08:28 -0700 Subject: [PATCH] apparmor: convert to profile block critical sections There are still a few places where profile replacement fails to update and a stale profile is used for mediation. Fix this by moving to accessing the current label through a critical section that will always ensure mediation is using the current label regardless of whether the tasks cred has been updated or not. Signed-off-by: John Johansen --- security/apparmor/apparmorfs.c | 40 +++++---- security/apparmor/context.c | 2 +- security/apparmor/domain.c | 5 +- security/apparmor/include/context.h | 123 ++++++++++++++++++++++------ security/apparmor/lsm.c | 41 +++++++--- security/apparmor/policy_unpack.c | 2 +- security/apparmor/procattr.c | 3 +- security/apparmor/resource.c | 2 +- 8 files changed, 162 insertions(+), 56 deletions(-) diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index b64ea21a42ad..e2919a0766b0 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -407,7 +407,10 @@ static ssize_t policy_update(u32 mask, const char __user *buf, size_t size, { ssize_t error; struct aa_loaddata *data; - struct aa_profile *profile = aa_current_profile(); + struct aa_profile *profile; + + profile = begin_current_profile_crit_section(); + /* high level check about policy management - fine grained in * below after unpack */ @@ -421,6 +424,7 @@ static ssize_t policy_update(u32 mask, const char __user *buf, size_t size, error = aa_replace_profiles(ns, profile, mask, data); aa_put_loaddata(data); } + end_current_profile_crit_section(profile); return error; } @@ -468,7 +472,7 @@ static ssize_t profile_remove(struct file *f, const char __user *buf, ssize_t error; struct aa_ns *ns = aa_get_ns(f->f_inode->i_private); - profile = aa_current_profile(); + profile = begin_current_profile_crit_section(); /* high level check about policy management - fine grained in * below after unpack */ @@ -489,6 +493,7 @@ static ssize_t profile_remove(struct file *f, const char __user *buf, aa_put_loaddata(data); } out: + end_current_profile_crit_section(profile); aa_put_ns(ns); return error; } @@ -667,8 +672,9 @@ static ssize_t query_data(char *buf, size_t buf_len, if (buf_len < sizeof(bytes) + sizeof(blocks)) return -EINVAL; /* not enough space */ - curr = aa_current_profile(); + curr = begin_current_profile_crit_section(); profile = aa_fqlookupn_profile(curr, query, strnlen(query, query_len)); + end_current_profile_crit_section(curr); if (!profile) return -ENOENT; @@ -754,8 +760,9 @@ static ssize_t query_label(char *buf, size_t buf_len, match_str = label_name + label_name_len + 1; match_len = query_len - label_name_len - 1; - curr = aa_current_profile(); + curr = begin_current_profile_crit_section(); profile = aa_fqlookupn_profile(curr, label_name, label_name_len); + end_current_profile_crit_section(curr); if (!profile) return -ENOENT; @@ -1094,18 +1101,22 @@ static const struct file_operations seq_ns_ ##NAME ##_fops = { \ static int seq_ns_level_show(struct seq_file *seq, void *v) { - struct aa_ns *ns = aa_current_profile()->ns; + struct aa_profile *profile; - seq_printf(seq, "%d\n", ns->level); + profile = begin_current_profile_crit_section(); + seq_printf(seq, "%d\n", profile->ns->level); + end_current_profile_crit_section(profile); return 0; } static int seq_ns_name_show(struct seq_file *seq, void *v) { - struct aa_ns *ns = aa_current_profile()->ns; + struct aa_profile *profile; - seq_printf(seq, "%s\n", aa_ns_name(ns, ns, true)); + profile = begin_current_profile_crit_section(); + seq_printf(seq, "%s\n", aa_ns_name(profile->ns, profile->ns, true)); + end_current_profile_crit_section(profile); return 0; } @@ -1530,9 +1541,9 @@ static int ns_mkdir_op(struct inode *dir, struct dentry *dentry, umode_t mode) { struct aa_ns *ns, *parent; /* TODO: improve permission check */ - struct aa_profile *profile = aa_current_profile(); + struct aa_profile *profile = begin_current_profile_crit_section(); int error = aa_may_manage_policy(profile, NULL, AA_MAY_LOAD_POLICY); - + end_current_profile_crit_section(profile); if (error) return error; @@ -1576,9 +1587,9 @@ static int ns_rmdir_op(struct inode *dir, struct dentry *dentry) { struct aa_ns *ns, *parent; /* TODO: improve permission check */ - struct aa_profile *profile = aa_current_profile(); + struct aa_profile *profile = begin_current_profile_crit_section(); int error = aa_may_manage_policy(profile, NULL, AA_MAY_LOAD_POLICY); - + end_current_profile_crit_section(profile); if (error) return error; @@ -1922,10 +1933,9 @@ static struct aa_profile *next_profile(struct aa_ns *root, static void *p_start(struct seq_file *f, loff_t *pos) { struct aa_profile *profile = NULL; - struct aa_ns *root = aa_current_profile()->ns; + struct aa_ns *root = aa_get_current_ns(); loff_t l = *pos; - f->private = aa_get_ns(root); - + f->private = root; /* find the first profile */ mutex_lock(&root->lock); diff --git a/security/apparmor/context.c b/security/apparmor/context.c index 1fc16b88efbf..410b9f7f68a1 100644 --- a/security/apparmor/context.c +++ b/security/apparmor/context.c @@ -79,7 +79,7 @@ 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)); + p = aa_get_newest_profile(__aa_task_raw_profile(task)); rcu_read_unlock(); return p; diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 2b1524c79fb8..0c02eac33a45 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -594,7 +594,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest) /* released below */ cred = get_current_cred(); ctx = cred_ctx(cred); - profile = aa_get_newest_profile(aa_cred_profile(cred)); + profile = aa_get_newest_cred_profile(cred); previous_profile = aa_get_newest_profile(ctx->previous); if (unconfined(profile)) { @@ -737,7 +737,7 @@ int aa_change_profile(const char *fqname, bool onexec, } cred = get_current_cred(); - profile = aa_cred_profile(cred); + profile = aa_get_newest_cred_profile(cred); /* * Fail explicitly requested domain transitions if no_new_privs @@ -795,6 +795,7 @@ audit: fqname, GLOBAL_ROOT_UID, info, error); aa_put_profile(target); + aa_put_profile(profile); put_cred(cred); return error; diff --git a/security/apparmor/include/context.h b/security/apparmor/include/context.h index 420cfd041218..7665fae7131f 100644 --- a/security/apparmor/include/context.h +++ b/security/apparmor/include/context.h @@ -56,14 +56,14 @@ struct aa_profile *aa_get_task_profile(struct task_struct *task); /** - * aa_cred_profile - obtain cred's profiles + * aa_cred_raw_profile - obtain cred's profiles * @cred: cred to obtain profiles from (NOT NULL) * * Returns: confining profile * * does NOT increment reference count */ -static inline struct aa_profile *aa_cred_profile(const struct cred *cred) +static inline struct aa_profile *aa_cred_raw_profile(const struct cred *cred) { struct aa_task_ctx *ctx = cred_ctx(cred); @@ -72,16 +72,28 @@ static inline struct aa_profile *aa_cred_profile(const struct cred *cred) } /** - * __aa_task_profile - retrieve another task's profile + * aa_get_newest_cred_profile - obtain the newest profile on a cred + * @cred: cred to obtain profile from (NOT NULL) + * + * Returns: newest version of confining profile + */ +static inline +struct aa_profile *aa_get_newest_cred_profile(const struct cred *cred) +{ + return aa_get_newest_profile(aa_cred_raw_profile(cred)); +} + +/** + * __aa_task_raw_profile - retrieve another task's profile * @task: task to query (NOT NULL) * * Returns: @task's profile without incrementing its ref count * * If @task != current needs to be called in RCU safe critical section */ -static inline struct aa_profile *__aa_task_profile(struct task_struct *task) +static inline struct aa_profile *__aa_task_raw_profile(struct task_struct *task) { - return aa_cred_profile(__task_cred(task)); + return aa_cred_raw_profile(__task_cred(task)); } /** @@ -92,50 +104,115 @@ static inline struct aa_profile *__aa_task_profile(struct task_struct *task) */ static inline bool __aa_task_is_confined(struct task_struct *task) { - return !unconfined(__aa_task_profile(task)); + return !unconfined(__aa_task_raw_profile(task)); } /** - * __aa_current_profile - find the current tasks confining profile + * aa_current_raw_profile - find the current tasks confining profile * * Returns: up to date confining profile or the ns unconfined profile (NOT NULL) * * This fn will not update the tasks cred to the most up to date version * of the profile so it is safe to call when inside of locks. */ -static inline struct aa_profile *__aa_current_profile(void) +static inline struct aa_profile *aa_current_raw_profile(void) { - return aa_cred_profile(current_cred()); + return aa_cred_raw_profile(current_cred()); } /** - * aa_current_profile - find the current tasks confining profile and do updates + * aa_get_current_profile - get the newest version of the current tasks profile * - * Returns: up to date confining profile or the ns unconfined profile (NOT NULL) + * Returns: newest version of confining profile (NOT NULL) + * + * This fn will not update the tasks cred, so it is safe inside of locks * - * This fn will update the tasks cred structure if the profile has been - * replaced. Not safe to call inside locks + * The returned reference must be put with aa_put_profile() */ -static inline struct aa_profile *aa_current_profile(void) +static inline struct aa_profile *aa_get_current_profile(void) { - const struct aa_task_ctx *ctx = current_ctx(); - struct aa_profile *profile; + struct aa_profile *p = aa_current_raw_profile(); - AA_BUG(!ctx || !ctx->profile); + if (profile_is_stale(p)) + return aa_get_newest_profile(p); + return aa_get_profile(p); +} - if (profile_is_stale(ctx->profile)) { - profile = aa_get_newest_profile(ctx->profile); - aa_replace_current_profile(profile); +#define __end_current_profile_crit_section(X) \ + end_current_profile_crit_section(X) + +/** + * end_profile_crit_section - put a reference found with begin_current_profile.. + * @profile: profile reference to put + * + * Should only be used with a reference obtained with + * begin_current_profile_crit_section and never used in situations where the + * task cred may be updated + */ +static inline void end_current_profile_crit_section(struct aa_profile *profile) +{ + if (profile != aa_current_raw_profile()) aa_put_profile(profile); - ctx = current_ctx(); +} + +/** + * __begin_current_profile_crit_section - current's confining profile + * + * Returns: up to date confining profile or the ns unconfined profile (NOT NULL) + * + * safe to call inside locks + * + * The returned reference must be put with __end_current_profile_crit_section() + * This must NOT be used if the task cred could be updated within the + * critical section between __begin_current_profile_crit_section() .. + * __end_current_profile_crit_section() + */ +static inline struct aa_profile *__begin_current_profile_crit_section(void) +{ + struct aa_profile *profile = aa_current_raw_profile(); + + if (profile_is_stale(profile)) + profile = aa_get_newest_profile(profile); + + return profile; +} + +/** + * begin_current_profile_crit_section - current's profile and update if needed + * + * Returns: up to date confining profile or the ns unconfined profile (NOT NULL) + * + * Not safe to call inside locks + * + * The returned reference must be put with end_current_profile_crit_section() + * This must NOT be used if the task cred could be updated within the + * critical section between begin_current_profile_crit_section() .. + * end_current_profile_crit_section() + */ +static inline struct aa_profile *begin_current_profile_crit_section(void) +{ + struct aa_profile *profile = aa_current_raw_profile(); + + if (profile_is_stale(profile)) { + profile = aa_get_newest_profile(profile); + if (aa_replace_current_profile(profile) == 0) + /* task cred will keep the reference */ + aa_put_profile(profile); } - return ctx->profile; + return profile; } static inline struct aa_ns *aa_get_current_ns(void) { - return aa_get_ns(__aa_current_profile()->ns); + struct aa_profile *profile; + struct aa_ns *ns; + + profile = __begin_current_profile_crit_section(); + ns = aa_get_ns(profile->ns); + __end_current_profile_crit_section(profile); + + return ns; } /** diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 35492008658f..49b780b4c53b 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -120,7 +120,7 @@ static int apparmor_capget(struct task_struct *target, kernel_cap_t *effective, rcu_read_lock(); cred = __task_cred(target); - profile = aa_cred_profile(cred); + profile = aa_get_newest_cred_profile(cred); /* * cap_capget is stacked ahead of this and will @@ -131,6 +131,7 @@ static int apparmor_capget(struct task_struct *target, kernel_cap_t *effective, *permitted = cap_intersect(*permitted, profile->caps.allow); } rcu_read_unlock(); + aa_put_profile(profile); return 0; } @@ -141,9 +142,11 @@ static int apparmor_capable(const struct cred *cred, struct user_namespace *ns, struct aa_profile *profile; int error = 0; - profile = aa_cred_profile(cred); + profile = aa_get_newest_cred_profile(cred); if (!unconfined(profile)) error = aa_capable(profile, cap, audit); + aa_put_profile(profile); + return error; } @@ -162,9 +165,10 @@ static int common_perm(const char *op, const struct path *path, u32 mask, struct aa_profile *profile; int error = 0; - profile = __aa_current_profile(); + profile = __begin_current_profile_crit_section(); if (!unconfined(profile)) error = aa_path_perm(op, profile, path, 0, mask, cond); + __end_current_profile_crit_section(profile); return error; } @@ -297,9 +301,11 @@ static int apparmor_path_link(struct dentry *old_dentry, const struct path *new_ if (!path_mediated_fs(old_dentry)) return 0; - profile = aa_current_profile(); + profile = begin_current_profile_crit_section(); if (!unconfined(profile)) error = aa_path_link(profile, old_dentry, new_dir, new_dentry); + end_current_profile_crit_section(profile); + return error; } @@ -312,7 +318,7 @@ static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_d if (!path_mediated_fs(old_dentry)) return 0; - profile = aa_current_profile(); + profile = begin_current_profile_crit_section(); if (!unconfined(profile)) { struct path old_path = { .mnt = old_dir->mnt, .dentry = old_dentry }; @@ -332,6 +338,8 @@ static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_d AA_MAY_CREATE, &cond); } + end_current_profile_crit_section(profile); + return error; } @@ -369,7 +377,7 @@ static int apparmor_file_open(struct file *file, const struct cred *cred) return 0; } - profile = aa_cred_profile(cred); + profile = aa_get_newest_cred_profile(cred); if (!unconfined(profile)) { struct inode *inode = file_inode(file); struct path_cond cond = { inode->i_uid, inode->i_mode }; @@ -379,18 +387,23 @@ static int apparmor_file_open(struct file *file, const struct cred *cred) /* todo cache full allowed permissions set and state */ fctx->allow = aa_map_file_to_perms(file); } + aa_put_profile(profile); return error; } static int apparmor_file_alloc_security(struct file *file) { + int error = 0; + /* freed by apparmor_file_free_security */ + struct aa_profile *profile = begin_current_profile_crit_section(); file->f_security = aa_alloc_file_context(GFP_KERNEL); if (!file->f_security) return -ENOMEM; - return 0; + end_current_profile_crit_section(profile); + return error; } static void apparmor_file_free_security(struct file *file) @@ -403,16 +416,17 @@ static void apparmor_file_free_security(struct file *file) static int common_file_perm(const char *op, struct file *file, u32 mask) { struct aa_file_ctx *fctx = file->f_security; - struct aa_profile *profile, *fprofile = aa_cred_profile(file->f_cred); + struct aa_profile *profile, *fprofile; int error = 0; + fprofile = aa_cred_raw_profile(file->f_cred); AA_BUG(!fprofile); if (!file->f_path.mnt || !path_mediated_fs(file->f_path.dentry)) return 0; - profile = __aa_current_profile(); + profile = __begin_current_profile_crit_section(); /* revalidate access, if task is unconfined, or the cached cred * doesn't match or if the request is for more permissions than @@ -424,6 +438,7 @@ static int common_file_perm(const char *op, struct file *file, u32 mask) if (!unconfined(profile) && !unconfined(fprofile) && ((fprofile != profile) || (mask & ~fctx->allow))) error = aa_file_perm(op, profile, file, mask); + __end_current_profile_crit_section(profile); return error; } @@ -568,10 +583,11 @@ out: return error; fail: - aad(&sa)->profile = aa_current_profile(); + aad(&sa)->profile = begin_current_profile_crit_section(); aad(&sa)->info = name; aad(&sa)->error = error = -EINVAL; aa_audit_msg(AUDIT_APPARMOR_DENIED, &sa, NULL); + end_current_profile_crit_section(aad(&sa)->profile); goto out; } @@ -581,7 +597,7 @@ fail: */ static void apparmor_bprm_committing_creds(struct linux_binprm *bprm) { - struct aa_profile *profile = __aa_current_profile(); + struct aa_profile *profile = aa_current_raw_profile(); struct aa_task_ctx *new_ctx = cred_ctx(bprm->cred); /* bail out if unconfined or not changing profile */ @@ -608,11 +624,12 @@ static void apparmor_bprm_committed_creds(struct linux_binprm *bprm) static int apparmor_task_setrlimit(struct task_struct *task, unsigned int resource, struct rlimit *new_rlim) { - struct aa_profile *profile = __aa_current_profile(); + struct aa_profile *profile = __begin_current_profile_crit_section(); int error = 0; if (!unconfined(profile)) error = aa_task_setrlimit(profile, task, resource, new_rlim); + __end_current_profile_crit_section(profile); return error; } diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index e521df1bd1fb..cac69f2cb86d 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -107,7 +107,7 @@ static int audit_iface(struct aa_profile *new, const char *ns_name, const char *name, const char *info, struct aa_ext *e, int error) { - struct aa_profile *profile = __aa_current_profile(); + struct aa_profile *profile = aa_current_raw_profile(); DEFINE_AUDIT_DATA(sa, LSM_AUDIT_DATA_NONE, NULL); if (e) aad(&sa)->iface.pos = e->pos - e->start; diff --git a/security/apparmor/procattr.c b/security/apparmor/procattr.c index 3466a27bca09..41b7b64a906b 100644 --- a/security/apparmor/procattr.c +++ b/security/apparmor/procattr.c @@ -41,7 +41,7 @@ int aa_getprocattr(struct aa_profile *profile, char **string) const char *mode_str = aa_profile_mode_names[profile->mode]; const char *ns_name = NULL; struct aa_ns *ns = profile->ns; - struct aa_ns *current_ns = __aa_current_profile()->ns; + struct aa_ns *current_ns = aa_get_current_ns(); char *s; if (!aa_ns_visible(current_ns, ns, true)) @@ -75,6 +75,7 @@ int aa_getprocattr(struct aa_profile *profile, char **string) else sprintf(s, "%s (%s)\n", profile->base.hname, mode_str); *string = str; + aa_put_ns(current_ns); /* NOTE: len does not include \0 of string, not saved as part of file */ return len; diff --git a/security/apparmor/resource.c b/security/apparmor/resource.c index ae66151fdc38..b26f1dac5106 100644 --- a/security/apparmor/resource.c +++ b/security/apparmor/resource.c @@ -90,7 +90,7 @@ int aa_task_setrlimit(struct aa_profile *profile, struct task_struct *task, int error = 0; rcu_read_lock(); - task_profile = aa_get_profile(aa_cred_profile(__task_cred(task))); + task_profile = aa_get_newest_cred_profile((__task_cred(task))); rcu_read_unlock(); /* TODO: extend resource control to handle other (non current) -- 2.20.1