apparmor: fix race on rawdata dereference

commit a0b7091c4de45a7325c8780e6934a894f92ac86b upstream.

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: 5d5182cae401 ("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>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

1 files changed, 43 insertions, 28 deletions

diff --git a/security/apparmor/include/policy_unpack.h b/security/apparmor/include/policy_unpack.h
index a6f4611ee50c..4f800fbb805a 100644
--- a/security/apparmor/include/policy_unpack.h
+++ b/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];
@@ -119,52 +131,55 @@ 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
+ * aa_get_loaddata - get a reference count from a counted 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
+ * 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)
 {
-	if (data && kref_get_unless_zero(&(data->count)))
-		return data;
 
-	return NULL;
+	if (data)
+		kref_get(&(data->count));
+	return data;
 }
 
+
 /**
- * aa_get_loaddata - get a reference count from a counted data reference
+ * aa_get_profile_loaddata - get a profile reference count on loaddata
  * @data: reference to get a count on
  *
- * Returns: point 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.
+ * Returns: pointer to reference
+ * Requires: @data to have a valid reference count on it.
  */
 static inline struct aa_loaddata *
-aa_get_loaddata(struct aa_loaddata *data)
+aa_get_profile_loaddata(struct aa_loaddata *data)
 {
-	struct aa_loaddata *tmp = __aa_get_loaddata(data);
-
-	AA_BUG(data && !tmp);
-
-	return tmp;
+	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);