From cdb759a09cb1ef90e882cbd93095c5d755de7f92 Mon Sep 17 00:00:00 2001
From: John Johansen <john.johansen@canonical.com>
Date: Fri, 9 Jun 2017 02:08:28 -0700
Subject: [PATCH 36/65] apparmor: convert to profile block critical sections
Git-commit: cf797c0e5e312520b0b9f0367039fc0279a07a76
Patch-mainline: v4.13-rc1
References: FATE#323500
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 <john.johansen@canonical.com>
Acked-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
---
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 @@ int aa_change_profile(const char *fqname, bool onexec,
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 @@ static int apparmor_setprocattr(const char *name, void *value,
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 @@ static int apparmor_setprocattr(const char *name, void *value,
*/
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 5ea6a7cb0212..1db4746270ab 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.12.3