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 <john.johansen@canonical.com>

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);

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;

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;

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;
 }
 
 /**

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;
 }

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;

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;

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)