apparmor: fix race on rawdata dereference

There is a race condition that leads to a use-after-free situation:
because the rawdata inodes are not refcounted, an attacker can start
open()ing one of the rawdata files, and at the same time remove the
last reference to this rawdata (by removing the corresponding profile,
for example), which frees its struct aa_loaddata; as a result, when
seq_rawdata_open() is reached, i_private is a dangling pointer and
freed memory is accessed.

The rawdata inodes weren't refcounted to avoid a circular refcount and
were supposed to be held by the profile rawdata reference.  However
during profile removal there is a window where the vfs and profile
destruction race, resulting in the use after free.

Fix this by moving to a double refcount scheme. Where the profile
refcount on rawdata is used to break the circular dependency. Allowing
for freeing of the rawdata once all inode references to the rawdata
are put.

Fixes: 5d5182cae4 ("apparmor: move to per loaddata files, instead of replicating in profiles")
Reported-by: Qualys Security Advisory <qsa@qualys.com>
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Reviewed-by: Maxime Bélair <maxime.belair@canonical.com>
Reviewed-by: Cengiz Can <cengiz.can@canonical.com>
Tested-by: Salvatore Bonaccorso <carnil@debian.org>
Signed-off-by: John Johansen <john.johansen@canonical.com>

security/apparmor/apparmorfs.c

@@ -79,7 +79,7 @@ static void rawdata_f_data_free(struct rawdata_f_data *private)
 	if (!private)
 		return;
 
-	aa_put_loaddata(private->loaddata);
+	aa_put_i_loaddata(private->loaddata);
 	kvfree(private);
 }
 
@@ -409,7 +409,8 @@ static struct aa_loaddata *aa_simple_write_to_buffer(const char __user *userbuf,
 
 	data->size = copy_size;
 	if (copy_from_user(data->data, userbuf, copy_size)) {
-		aa_put_loaddata(data);
+		/* trigger free - don't need to put pcount */
+		aa_put_i_loaddata(data);
 		return ERR_PTR(-EFAULT);
 	}
 
@@ -437,7 +438,10 @@ static ssize_t policy_update(u32 mask, const char __user *buf, size_t size,
 	error = PTR_ERR(data);
 	if (!IS_ERR(data)) {
 		error = aa_replace_profiles(ns, label, mask, data);
-		aa_put_loaddata(data);
+		/* put pcount, which will put count and free if no
+		 * profiles referencing it.
+		 */
+		aa_put_profile_loaddata(data);
 	}
 end_section:
 	end_current_label_crit_section(label);
@@ -508,7 +512,7 @@ static ssize_t profile_remove(struct file *f, const char __user *buf,
 	if (!IS_ERR(data)) {
 		data->data[size] = 0;
 		error = aa_remove_profiles(ns, label, data->data, size);
-		aa_put_loaddata(data);
+		aa_put_profile_loaddata(data);
 	}
  out:
 	end_current_label_crit_section(label);
@@ -1255,18 +1259,17 @@ static const struct file_operations seq_rawdata_ ##NAME ##_fops = {	      \
 static int seq_rawdata_open(struct inode *inode, struct file *file,
 			    int (*show)(struct seq_file *, void *))
 {
-	struct aa_loaddata *data = __aa_get_loaddata(inode->i_private);
+	struct aa_loaddata *data = aa_get_i_loaddata(inode->i_private);
 	int error;
 
 	if (!data)
-		/* lost race this ent is being reaped */
 		return -ENOENT;
 
 	error = single_open(file, show, data);
 	if (error) {
 		AA_BUG(file->private_data &&
 		       ((struct seq_file *)file->private_data)->private);
-		aa_put_loaddata(data);
+		aa_put_i_loaddata(data);
 	}
 
 	return error;
@@ -1277,7 +1280,7 @@ static int seq_rawdata_release(struct inode *inode, struct file *file)
 	struct seq_file *seq = (struct seq_file *) file->private_data;
 
 	if (seq)
-		aa_put_loaddata(seq->private);
+		aa_put_i_loaddata(seq->private);
 
 	return single_release(inode, file);
 }
@@ -1389,9 +1392,8 @@ static int rawdata_open(struct inode *inode, struct file *file)
 	if (!aa_current_policy_view_capable(NULL))
 		return -EACCES;
 
-	loaddata = __aa_get_loaddata(inode->i_private);
+	loaddata = aa_get_i_loaddata(inode->i_private);
 	if (!loaddata)
-		/* lost race: this entry is being reaped */
 		return -ENOENT;
 
 	private = rawdata_f_data_alloc(loaddata->size);
@@ -1416,7 +1418,7 @@ fail_decompress:
 	return error;
 
 fail_private_alloc:
-	aa_put_loaddata(loaddata);
+	aa_put_i_loaddata(loaddata);
 	return error;
 }
 
@@ -1433,9 +1435,9 @@ static void remove_rawdata_dents(struct aa_loaddata *rawdata)
 
 	for (i = 0; i < AAFS_LOADDATA_NDENTS; i++) {
 		if (!IS_ERR_OR_NULL(rawdata->dents[i])) {
-			/* no refcounts on i_private */
 			aafs_remove(rawdata->dents[i]);
 			rawdata->dents[i] = NULL;
+			aa_put_i_loaddata(rawdata);
 		}
 	}
 }
@@ -1474,18 +1476,21 @@ int __aa_fs_create_rawdata(struct aa_ns *ns, struct aa_loaddata *rawdata)
 	if (IS_ERR(dir))
 		/* ->name freed when rawdata freed */
 		return PTR_ERR(dir);
+	aa_get_i_loaddata(rawdata);
 	rawdata->dents[AAFS_LOADDATA_DIR] = dir;
 
 	dent = aafs_create_file("abi", S_IFREG | 0444, dir, rawdata,
 				      &seq_rawdata_abi_fops);
 	if (IS_ERR(dent))
 		goto fail;
+	aa_get_i_loaddata(rawdata);
 	rawdata->dents[AAFS_LOADDATA_ABI] = dent;
 
 	dent = aafs_create_file("revision", S_IFREG | 0444, dir, rawdata,
 				      &seq_rawdata_revision_fops);
 	if (IS_ERR(dent))
 		goto fail;
+	aa_get_i_loaddata(rawdata);
 	rawdata->dents[AAFS_LOADDATA_REVISION] = dent;
 
 	if (aa_g_hash_policy) {
@@ -1493,6 +1498,7 @@ int __aa_fs_create_rawdata(struct aa_ns *ns, struct aa_loaddata *rawdata)
 					      rawdata, &seq_rawdata_hash_fops);
 		if (IS_ERR(dent))
 			goto fail;
+		aa_get_i_loaddata(rawdata);
 		rawdata->dents[AAFS_LOADDATA_HASH] = dent;
 	}
 
@@ -1501,24 +1507,25 @@ int __aa_fs_create_rawdata(struct aa_ns *ns, struct aa_loaddata *rawdata)
 				&seq_rawdata_compressed_size_fops);
 	if (IS_ERR(dent))
 		goto fail;
+	aa_get_i_loaddata(rawdata);
 	rawdata->dents[AAFS_LOADDATA_COMPRESSED_SIZE] = dent;
 
 	dent = aafs_create_file("raw_data", S_IFREG | 0444,
 				      dir, rawdata, &rawdata_fops);
 	if (IS_ERR(dent))
 		goto fail;
+	aa_get_i_loaddata(rawdata);
 	rawdata->dents[AAFS_LOADDATA_DATA] = dent;
 	d_inode(dent)->i_size = rawdata->size;
 
 	rawdata->ns = aa_get_ns(ns);
 	list_add(&rawdata->list, &ns->rawdata_list);
-	/* no refcount on inode rawdata */
 
 	return 0;
 
 fail:
 	remove_rawdata_dents(rawdata);
-
+	aa_put_i_loaddata(rawdata);
 	return PTR_ERR(dent);
 }
 #endif /* CONFIG_SECURITY_APPARMOR_EXPORT_BINARY */

security/apparmor/include/policy_unpack.h

@@ -87,17 +87,29 @@ struct aa_ext {
 	u32 version;
 };
 
-/*
- * struct aa_loaddata - buffer of policy raw_data set
+/* struct aa_loaddata - buffer of policy raw_data set
+ * @count: inode/filesystem refcount - use aa_get_i_loaddata()
+ * @pcount: profile refcount - use aa_get_profile_loaddata()
+ * @list: list the loaddata is on
+ * @work: used to do a delayed cleanup
+ * @dents: refs to dents created in aafs
+ * @ns: the namespace this loaddata was loaded into
+ * @name:
+ * @size: the size of the data that was loaded
+ * @compressed_size: the size of the data when it is compressed
+ * @revision: unique revision count that this data was loaded as
+ * @abi: the abi number the loaddata uses
+ * @hash: a hash of the loaddata, used to help dedup data
  *
- * there is no loaddata ref for being on ns list, nor a ref from
- * d_inode(@dentry) when grab a ref from these, @ns->lock must be held
- * && __aa_get_loaddata() needs to be used, and the return value
- * checked, if NULL the loaddata is already being reaped and should be
- * considered dead.
+ * There is no loaddata ref for being on ns->rawdata_list, so
+ * @ns->lock must be held when walking the list. Dentries and
+ * inode opens hold refs on @count; profiles hold refs on @pcount.
+ * When the last @pcount drops, do_ploaddata_rmfs() removes the
+ * fs entries and drops the associated @count ref.
  */
 struct aa_loaddata {
 	struct kref count;
+	struct kref pcount;
 	struct list_head list;
 	struct work_struct work;
 	struct dentry *dents[AAFS_LOADDATA_NDENTS];
@@ -118,53 +130,56 @@ struct aa_loaddata {
 
 int aa_unpack(struct aa_loaddata *udata, struct list_head *lh, const char **ns);
 
-/**
- * __aa_get_loaddata - get a reference count to uncounted data reference
- * @data: reference to get a count on
- *
- * Returns: pointer to reference OR NULL if race is lost and reference is
- *          being repeated.
- * Requires: @data->ns->lock held, and the return code MUST be checked
- *
- * Use only from inode->i_private and @data->list found references
- */
-static inline struct aa_loaddata *
-__aa_get_loaddata(struct aa_loaddata *data)
-{
-	if (data && kref_get_unless_zero(&(data->count)))
-		return data;
-
-	return NULL;
-}
-
 /**
  * aa_get_loaddata - get a reference count from a counted data reference
  * @data: reference to get a count on
  *
- * Returns: point to reference
+ * Returns: pointer to reference
  * Requires: @data to have a valid reference count on it. It is a bug
  *           if the race to reap can be encountered when it is used.
  */
 static inline struct aa_loaddata *
-aa_get_loaddata(struct aa_loaddata *data)
+aa_get_i_loaddata(struct aa_loaddata *data)
 {
-	struct aa_loaddata *tmp = __aa_get_loaddata(data);
 
-	AA_BUG(data && !tmp);
+	if (data)
+		kref_get(&(data->count));
+	return data;
+}
 
-	return tmp;
+
+/**
+ * aa_get_profile_loaddata - get a profile reference count on loaddata
+ * @data: reference to get a count on
+ *
+ * Returns: pointer to reference
+ * Requires: @data to have a valid reference count on it.
+ */
+static inline struct aa_loaddata *
+aa_get_profile_loaddata(struct aa_loaddata *data)
+{
+	if (data)
+		kref_get(&(data->pcount));
+	return data;
 }
 
 void __aa_loaddata_update(struct aa_loaddata *data, long revision);
 bool aa_rawdata_eq(struct aa_loaddata *l, struct aa_loaddata *r);
 void aa_loaddata_kref(struct kref *kref);
+void aa_ploaddata_kref(struct kref *kref);
 struct aa_loaddata *aa_loaddata_alloc(size_t size);
-static inline void aa_put_loaddata(struct aa_loaddata *data)
+static inline void aa_put_i_loaddata(struct aa_loaddata *data)
 {
 	if (data)
 		kref_put(&data->count, aa_loaddata_kref);
 }
 
+static inline void aa_put_profile_loaddata(struct aa_loaddata *data)
+{
+	if (data)
+		kref_put(&data->pcount, aa_ploaddata_kref);
+}
+
 #if IS_ENABLED(CONFIG_KUNIT)
 bool aa_inbounds(struct aa_ext *e, size_t size);
 size_t aa_unpack_u16_chunk(struct aa_ext *e, char **chunk);

security/apparmor/policy.c

@@ -350,7 +350,7 @@ void aa_free_profile(struct aa_profile *profile)
 	}
 
 	kfree_sensitive(profile->hash);
-	aa_put_loaddata(profile->rawdata);
+	aa_put_profile_loaddata(profile->rawdata);
 	aa_label_destroy(&profile->label);
 
 	kfree_sensitive(profile);
@@ -1171,7 +1171,7 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label,
 	LIST_HEAD(lh);
 
 	op = mask & AA_MAY_REPLACE_POLICY ? OP_PROF_REPL : OP_PROF_LOAD;
-	aa_get_loaddata(udata);
+	aa_get_profile_loaddata(udata);
 	/* released below */
 	error = aa_unpack(udata, &lh, &ns_name);
 	if (error)
@@ -1223,10 +1223,10 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label,
 			if (aa_rawdata_eq(rawdata_ent, udata)) {
 				struct aa_loaddata *tmp;
 
-				tmp = __aa_get_loaddata(rawdata_ent);
+				tmp = aa_get_profile_loaddata(rawdata_ent);
 				/* check we didn't fail the race */
 				if (tmp) {
-					aa_put_loaddata(udata);
+					aa_put_profile_loaddata(udata);
 					udata = tmp;
 					break;
 				}
@@ -1239,7 +1239,7 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label,
 		struct aa_profile *p;
 
 		if (aa_g_export_binary)
-			ent->new->rawdata = aa_get_loaddata(udata);
+			ent->new->rawdata = aa_get_profile_loaddata(udata);
 		error = __lookup_replace(ns, ent->new->base.hname,
 					 !(mask & AA_MAY_REPLACE_POLICY),
 					 &ent->old, &info);
@@ -1372,7 +1372,7 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label,
 
 out:
 	aa_put_ns(ns);
-	aa_put_loaddata(udata);
+	aa_put_profile_loaddata(udata);
 	kfree(ns_name);
 
 	if (error)

security/apparmor/policy_unpack.c

@@ -109,22 +109,8 @@ bool aa_rawdata_eq(struct aa_loaddata *l, struct aa_loaddata *r)
 	return memcmp(l->data, r->data, r->compressed_size ?: r->size) == 0;
 }
 
-/*
- * need to take the ns mutex lock which is NOT safe most places that
- * put_loaddata is called, so we have to delay freeing it
- */
-static void do_loaddata_free(struct work_struct *work)
+static void do_loaddata_free(struct aa_loaddata *d)
 {
-	struct aa_loaddata *d = container_of(work, struct aa_loaddata, work);
-	struct aa_ns *ns = aa_get_ns(d->ns);
-
-	if (ns) {
-		mutex_lock_nested(&ns->lock, ns->level);
-		__aa_fs_remove_rawdata(d);
-		mutex_unlock(&ns->lock);
-		aa_put_ns(ns);
-	}
-
 	kfree_sensitive(d->hash);
 	kfree_sensitive(d->name);
 	kvfree(d->data);
@@ -135,8 +121,35 @@ void aa_loaddata_kref(struct kref *kref)
 {
 	struct aa_loaddata *d = container_of(kref, struct aa_loaddata, count);
 
+	do_loaddata_free(d);
+}
+
+/*
+ * need to take the ns mutex lock which is NOT safe most places that
+ * put_loaddata is called, so we have to delay freeing it
+ */
+static void do_ploaddata_rmfs(struct work_struct *work)
+{
+	struct aa_loaddata *d = container_of(work, struct aa_loaddata, work);
+	struct aa_ns *ns = aa_get_ns(d->ns);
+
+	if (ns) {
+		mutex_lock_nested(&ns->lock, ns->level);
+		/* remove fs ref to loaddata */
+		__aa_fs_remove_rawdata(d);
+		mutex_unlock(&ns->lock);
+		aa_put_ns(ns);
+	}
+	/* called by dropping last pcount, so drop its associated icount */
+	aa_put_i_loaddata(d);
+}
+
+void aa_ploaddata_kref(struct kref *kref)
+{
+	struct aa_loaddata *d = container_of(kref, struct aa_loaddata, pcount);
+
 	if (d) {
-		INIT_WORK(&d->work, do_loaddata_free);
+		INIT_WORK(&d->work, do_ploaddata_rmfs);
 		schedule_work(&d->work);
 	}
 }
@@ -154,6 +167,7 @@ struct aa_loaddata *aa_loaddata_alloc(size_t size)
 		return ERR_PTR(-ENOMEM);
 	}
 	kref_init(&d->count);
+	kref_init(&d->pcount);
 	INIT_LIST_HEAD(&d->list);
 
 	return d;