[PATCH v3 2/5] libselinux: stringrep: serialize access to the discover_class_cache

The discover_class_cache list is accessed without locking by the
string_to_*() and *_to_string() functions in libselinux for
translating between string names and class/perm values, and returns
pointers to strings within this list to callers.
selinux_flush_class_cache() frees the list upon a policy reload,
called by avc_process_policyload() from selinux_status_updated() and
avc_netlink_process().

Make this safe for multi-threaded users by introducing and taking a
mutex around the list accesses, and by moving flushed nodes to a
retired list rather than freeing them so that returned string pointers
remain valid. Given the infrequency of policy reloads, the relative
stability of the class/perm mapping even across policy reloads, and
the small amount of memory required, this seems a worthwhile tradeoff.
If it becomes an issue, there are several options:
1. Only load/refresh new/modified class/permission names and values
upon policy reload rather than flushing and reloading them all,
2. Provide a way to drain the retired list safely when the application
knows it is no longer using any returned strings.

Fixes: #287 #335 #336
Link: https://lore.kernel.org/selinux/CAJsHiNx1E7x1jaBkS0i4L1nBWXp8YXLHRWcCaDaH4LOn=zm+Zw@mail.gmail.com/
Link: https://lore.kernel.org/selinux/[email protected]/
Reported-by: Seth Moore <[email protected]>
Reported-by: Purushottam Choudhary <[email protected]>

Signed-off-by: Stephen Smalley <[email protected]>
---
 libselinux/src/stringrep.c | 120 +++++++++++++++++++++++++++----------
 1 file changed, 87 insertions(+), 33 deletions(-)

diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c
index ec6fc637..cd9841ca 100644
--- a/libselinux/src/stringrep.c
+++ b/libselinux/src/stringrep.c
@@ -27,7 +27,9 @@ struct discover_class_node {
 	struct discover_class_node *next;
 };
 
+static pthread_mutex_t discover_class_lock = PTHREAD_MUTEX_INITIALIZER;
 static struct discover_class_node *discover_class_cache = NULL;
+static struct discover_class_node *discover_class_retired = NULL;
 
 static struct discover_class_node *get_class_cache_entry_name(const char *s)
 {
@@ -156,9 +158,6 @@ static struct discover_class_node *discover_class(const char *s)
 	}
 	closedir(dir);
 
-	node->next = discover_class_cache;
-	discover_class_cache = node;
-
 	return node;
 
 err4:
@@ -176,41 +175,84 @@ err1:
 
 void selinux_flush_class_cache(void)
 {
-	struct discover_class_node *cur = discover_class_cache, *prev = NULL;
-	size_t i;
-
-	while (cur != NULL) {
-		free(cur->name);
-
-		for (i = 0; i < MAXVECTORS; i++)
-			free(cur->perms[i]);
-
-		free(cur->perms);
+	struct discover_class_node *head, *tail;
 
-		prev = cur;
-		cur = cur->next;
+	__pthread_mutex_lock(&discover_class_lock);
+	head = discover_class_cache;
+	discover_class_cache = NULL;
 
-		free(prev);
+	/*
+	 * Move old list to the retired chain rather than
+	 * freeing it; other threads may hold pointers to
+	 * class/perm strings within it returned by
+	 * security_class_to_string() and
+	 * security_av_perm_to_string(). This incurs a small
+	 * memory cost but given the infrequency of policy
+	 * reloads, it should never be significant. If
+	 * required, a new interface could be introduced to
+	 * allow the application to safely drain the retired
+	 * list when it knows it is safe to do so.
+	 */
+	if (head) {
+		tail = head;
+		while (tail->next)
+			tail = tail->next;
+		tail->next = discover_class_retired;
+		discover_class_retired = head;
 	}
-
-	discover_class_cache = NULL;
+	__pthread_mutex_unlock(&discover_class_lock);
 }
 
 security_class_t string_to_security_class(const char *s)
 {
-	struct discover_class_node *node;
+	struct discover_class_node *node, *retry;
+	security_class_t value;
+	size_t i;
 
+	__pthread_mutex_lock(&discover_class_lock);
 	node = get_class_cache_entry_name(s);
-	if (node == NULL) {
-		node = discover_class(s);
+	if (node) {
+		value = node->value;
+		__pthread_mutex_unlock(&discover_class_lock);
+		return map_class(value);
+	}
+	__pthread_mutex_unlock(&discover_class_lock);
 
-		if (node == NULL) {
-			errno = EINVAL;
-			return 0;
-		}
+	/* Discover outside of lock; reads selinuxfs */
+	node = discover_class(s);
+	if (node == NULL) {
+		errno = EINVAL;
+		return 0;
 	}
+	value = node->value;
+
+	/*
+	 * Retry cache lookup with lock held in case
+	 * another thread raced with us. If we don't
+	 * find it, add the new one; else use
+	 * the old one and free the new one.
+	 */
+	__pthread_mutex_lock(&discover_class_lock);
+	retry = get_class_cache_entry_name(s);
+	if (!retry) {
+		node->next = discover_class_cache;
+		discover_class_cache = node;
+	} else {
+		value = retry->value;
+		/*
+		 * We can only safely free node here
+		 * because it wasn't yet added to the cache.
+		 */
+		free(node->name);
+		for (i = 0; i < MAXVECTORS; i++)
+			free(node->perms[i]);
+		free(node->perms);
+		free(node);
+		node = NULL;
+	}
+	__pthread_mutex_unlock(&discover_class_lock);
 
-	return map_class(node->value);
+	return map_class(value);
 }
 
 security_class_t mode_to_security_class(mode_t m)
@@ -239,13 +281,17 @@ access_vector_t string_to_av_perm(security_class_t tclass, const char *s)
 	struct discover_class_node *node;
 	security_class_t kclass = unmap_class(tclass);
 
+	__pthread_mutex_lock(&discover_class_lock);
 	node = get_class_cache_entry_value(kclass);
 	if (node != NULL) {
 		size_t i;
 		for (i = 0; i < MAXVECTORS && node->perms[i] != NULL; i++)
-			if (strcmp(node->perms[i], s) == 0)
+			if (strcmp(node->perms[i], s) == 0) {
+				__pthread_mutex_unlock(&discover_class_lock);
 				return map_perm(tclass, UINT32_C(1) << i);
+			}
 	}
+	__pthread_mutex_unlock(&discover_class_lock);
 
 	errno = EINVAL;
 	return 0;
@@ -254,14 +300,16 @@ access_vector_t string_to_av_perm(security_class_t tclass, const char *s)
 const char *security_class_to_string(security_class_t tclass)
 {
 	struct discover_class_node *node;
+	const char *name;
 
 	tclass = unmap_class(tclass);
 
+	__pthread_mutex_lock(&discover_class_lock);
 	node = get_class_cache_entry_value(tclass);
-	if (node == NULL)
-		return NULL;
-	else
-		return node->name;
+	name = node ? node->name : NULL;
+	__pthread_mutex_unlock(&discover_class_lock);
+
+	return name;
 }
 
 const char *security_av_perm_to_string(security_class_t tclass,
@@ -269,16 +317,22 @@ const char *security_av_perm_to_string(security_class_t tclass,
 {
 	struct discover_class_node *node;
 	size_t i;
+	const char *name;
 
 	av = unmap_perm(tclass, av);
 	tclass = unmap_class(tclass);
 
+	__pthread_mutex_lock(&discover_class_lock);
 	node = get_class_cache_entry_value(tclass);
 	if (av && node)
 		for (i = 0; i < MAXVECTORS; i++)
-			if ((UINT32_C(1) << i) & av)
-				return node->perms[i];
+			if ((UINT32_C(1) << i) & av) {
+				name = node->perms[i];
+				__pthread_mutex_unlock(&discover_class_lock);
+				return name;
+			}
 
+	__pthread_mutex_unlock(&discover_class_lock);
 	return NULL;
 }
 
-- 
2.54.0