From 7d313f4322e44425882b21764bc9c42a790d030a Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Tue, 13 Sep 2022 07:35:09 +0200
Subject: tools/xenstore: limit max number of nodes accessed in a transaction

Today a guest is free to access as many nodes in a single transaction
as it wants. This can lead to unbounded memory consumption in Xenstore
as there is the need to keep track of all nodes having been accessed
during a transaction.

In oxenstored the number of requests in a transaction is being limited
via a quota maxrequests (default is 1024). As multiple accesses of a
node are not problematic in C Xenstore, limit the number of accessed
nodes.

In order to let read_node() detect a quota error in case too many nodes
are being accessed, check the return value of access_node() and return
NULL in case an error has been seen. Introduce __must_check and add it
to the access_node() prototype.

This is part of XSA-326 / CVE-2022-42314.

Suggested-by: Julien Grall <julien@xen.org>
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>

--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2871,6 +2871,9 @@ void libxl__numa_candidate_put_nodemap(l
  */
 #define CTYPE(isfoo,c) (isfoo((unsigned char)(c)))
 
+#ifndef __must_check
+#define __must_check __attribute__((__warn_unused_result__))
+#endif
 
 #endif
 
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -92,6 +92,7 @@ int quota_nb_watch_per_domain = 128;
 int quota_max_entry_size = 2048; /* 2K */
 int quota_max_transaction = 10;
 int quota_nb_perms_per_node = 5;
+int quota_trans_nodes = 1024;
 int quota_req_outstanding = 20;
 
 unsigned int timeout_watch_event_msec = 20000;
@@ -491,6 +492,7 @@ struct node *read_node(struct connection
 	TDB_DATA key, data;
 	struct xs_tdb_record_hdr *hdr;
 	struct node *node;
+	int err;
 
 	node = talloc(ctx, struct node);
 	if (!node) {
@@ -512,14 +514,13 @@ struct node *read_node(struct connection
 	if (data.dptr == NULL) {
 		if (tdb_error(tdb_ctx) == TDB_ERR_NOEXIST) {
 			node->generation = NO_GENERATION;
-			access_node(conn, node, NODE_ACCESS_READ, NULL);
-			errno = ENOENT;
+			err = access_node(conn, node, NODE_ACCESS_READ, NULL);
+			errno = err ? : ENOENT;
 		} else {
 			log("TDB error on read: %s", tdb_errorstr(tdb_ctx));
 			errno = EIO;
 		}
-		talloc_free(node);
-		return NULL;
+		goto error;
 	}
 
 	node->parent = NULL;
@@ -534,19 +535,36 @@ struct node *read_node(struct connection
 
 	/* Permissions are struct xs_permissions. */
 	node->perms.p = hdr->perms;
-	if (domain_adjust_node_perms(conn, node)) {
-		talloc_free(node);
-		return NULL;
-	}
+	if (domain_adjust_node_perms(conn, node))
+		goto error;
 
 	/* Data is binary blob (usually ascii, no nul). */
 	node->data = node->perms.p + hdr->num_perms;
 	/* Children is strings, nul separated. */
 	node->children = node->data + node->datalen;
 
-	access_node(conn, node, NODE_ACCESS_READ, NULL);
+	if (access_node(conn, node, NODE_ACCESS_READ, NULL))
+		goto error;
 
 	return node;
+
+ error:
+	err = errno;
+	talloc_free(node);
+	errno = err;
+	return NULL;
+}
+
+static bool read_node_can_propagate_errno(void)
+{
+	/*
+	 * 2 error cases for read_node() can always be propagated up:
+	 * ENOMEM, because this has nothing to do with the node being in the
+	 * data base or not, but is caused by a general lack of memory.
+	 * ENOSPC, because this is related to hitting quota limits which need
+	 * to be respected.
+	 */
+	return errno == ENOMEM || errno == ENOSPC;
 }
 
 int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node,
@@ -661,7 +679,7 @@ static int ask_parents(struct connection
 		node = read_node(conn, ctx, name);
 		if (node)
 			break;
-		if (errno == ENOMEM)
+		if (read_node_can_propagate_errno())
 			return errno;
 	} while (!streq(name, "/"));
 
@@ -724,7 +742,7 @@ static struct node *get_node(struct conn
 		}
 	}
 	/* Clean up errno if they weren't supposed to know. */
-	if (!node && errno != ENOMEM)
+	if (!node && !read_node_can_propagate_errno())
 		errno = errno_from_parents(conn, ctx, name, errno, perm);
 	return node;
 }
@@ -1106,7 +1124,7 @@ static struct node *construct_node(struc
 
 	/* If parent doesn't exist, create it. */
 	parent = read_node(conn, parentname, parentname);
-	if (!parent)
+	if (!parent && errno == ENOENT)
 		parent = construct_node(conn, ctx, parentname);
 	if (!parent)
 		return NULL;
@@ -1387,7 +1405,7 @@ static int _rm(struct connection *conn,
 
 	parent = read_node(conn, ctx, parentname);
 	if (!parent)
-		return (errno == ENOMEM) ? ENOMEM : EINVAL;
+		return read_node_can_propagate_errno() ? errno : EINVAL;
 	node->parent = parent;
 
 	return delete_node(conn, ctx, parent, node, false);
@@ -1415,7 +1433,7 @@ static int do_rm(struct connection *conn
 				return 0;
 			}
 			/* Restore errno, just in case. */
-			if (errno != ENOMEM)
+			if (!read_node_can_propagate_errno())
 				errno = ENOENT;
 		}
 		return errno;
@@ -2178,6 +2196,8 @@ static void usage(void)
 "  --perm-nb <nb>      limit the number of permissions per node,\n"
 "  --quota <what>=<nb> set the quota <what> to the value <nb>, allowed\n"
 "                      quotas are:\n"
+"                      transaction-nodes: number of accessed node per\n"
+"                                         transaction\n"
 "                      outstanding: number of outstanding requests\n"
 "  --timeout <what>=<seconds>   set the timeout in seconds for <what>,\n"
 "                      allowed timeout candidates are:\n"
@@ -2257,6 +2277,8 @@ static void set_quota(const char *arg)
 	val = get_optval_int(eq + 1);
 	if (what_matches(arg, "outstanding"))
 		quota_req_outstanding = val;
+	else if (what_matches(arg, "transaction-nodes"))
+		quota_trans_nodes = val;
 	else
 		barf("unknown quota \"%s\"\n", arg);
 }
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -32,6 +32,7 @@
 #include "list.h"
 #include "tdb.h"
 #include "hashtable.h"
+#include "utils.h"
 
 #define MIN(a, b) (((a) < (b))? (a) : (b))
 
@@ -222,6 +223,7 @@ extern int dom0_event;
 extern int priv_domid;
 extern int quota_nb_entry_per_domain;
 extern int quota_req_outstanding;
+extern int quota_trans_nodes;
 
 extern unsigned int timeout_watch_event_msec;
 
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -157,6 +157,9 @@ struct transaction
 	/* Connection-local identifier for this transaction. */
 	uint32_t id;
 
+	/* Node counter. */
+	unsigned int nodes;
+
 	/* Generation when transaction started. */
 	uint64_t generation;
 
@@ -267,6 +270,11 @@ int access_node(struct connection *conn,
 
 	i = find_accessed_node(trans, node->name);
 	if (!i) {
+		if (trans->nodes >= quota_trans_nodes &&
+		    domain_is_unprivileged(conn)) {
+			ret = ENOSPC;
+			goto err;
+		}
 		i = talloc_zero(trans, struct accessed_node);
 		if (!i)
 			goto nomem;
@@ -304,6 +312,7 @@ int access_node(struct connection *conn,
 				i->ta_node = true;
 			}
 		}
+		trans->nodes++;
 		list_add_tail(&i->list, &trans->accessed);
 	}
 
--- a/tools/xenstore/xenstored_transaction.h
+++ b/tools/xenstore/xenstored_transaction.h
@@ -20,6 +20,10 @@
 #define _XENSTORED_TRANSACTION_H
 #include "xenstored_core.h"
 
+#ifndef __must_check
+#define __must_check __attribute__((__warn_unused_result__))
+#endif
+
 enum node_access_type {
     NODE_ACCESS_READ,
     NODE_ACCESS_WRITE,
@@ -40,8 +44,8 @@ void transaction_entry_inc(struct transa
 void transaction_entry_dec(struct transaction *trans, unsigned int domid);
 
 /* This node was accessed. */
-int access_node(struct connection *conn, struct node *node,
-                enum node_access_type type, TDB_DATA *key);
+int __must_check access_node(struct connection *conn, struct node *node,
+                             enum node_access_type type, TDB_DATA *key);
 
 /* Queue watches for a modified node. */
 void queue_watches(struct connection *conn, const char *name, bool watch_exact);
