From c13d85a2fe94bbf3cb8186b89324c5d1b4f9a61f Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Tue, 13 Sep 2022 07:35:11 +0200
Subject: tools/xenstore: don't let remove_child_entry() call corrupt()

In case of write_node() returning an error, remove_child_entry() will
call corrupt() today. This could result in an endless recursion, as
remove_child_entry() is called by corrupt(), too:

corrupt()
  check_store()
    check_store_()
      remove_child_entry()

Fix that by letting remove_child_entry() return an error instead and
let the caller decide what to do.

This is part of XSA-418 / CVE-2022-42321.

Reported-by: Julien Grall <dvrabel@amazon.co.uk>
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 3907c35643e9..f433a45dc217 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1608,15 +1608,15 @@ static void memdel(void *mem, unsigned off, unsigned len, unsigned total)
 	memmove(mem + off, mem + off + len, total - off - len);
 }
 
-static void remove_child_entry(struct connection *conn, struct node *node,
-			       size_t offset)
+static int remove_child_entry(struct connection *conn, struct node *node,
+			      size_t offset)
 {
 	size_t childlen = strlen(node->children + offset);
 
 	memdel(node->children, offset, childlen + 1, node->childlen);
 	node->childlen -= childlen + 1;
-	if (write_node(conn, node, true))
-		corrupt(conn, "Can't update parent node '%s'", node->name);
+
+	return write_node(conn, node, true);
 }
 
 static void delete_child(struct connection *conn,
@@ -1626,7 +1626,9 @@ static void delete_child(struct connection *conn,
 
 	for (i = 0; i < node->childlen; i += strlen(node->children+i) + 1) {
 		if (streq(node->children+i, childname)) {
-			remove_child_entry(conn, node, i);
+			if (remove_child_entry(conn, node, i))
+				corrupt(conn, "Can't update parent node '%s'",
+					node->name);
 			return;
 		}
 	}
@@ -2325,6 +2327,17 @@ int remember_string(struct hashtable *hash, const char *str)
 	return hashtable_insert(hash, k, (void *)1);
 }
 
+static int rm_child_entry(struct node *node, size_t off, size_t len)
+{
+	if (!recovery)
+		return off;
+
+	if (remove_child_entry(NULL, node, off))
+		log("check_store: child entry could not be removed from '%s'",
+		    node->name);
+
+	return off - len - 1;
+}
 
 /**
  * A node has a children field that names the children of the node, separated
@@ -2377,12 +2390,7 @@ static int check_store_(const char *name, struct hashtable *reachable)
 				if (hashtable_search(children, childname)) {
 					log("check_store: '%s' is duplicated!",
 					    childname);
-
-					if (recovery) {
-						remove_child_entry(NULL, node,
-								   i);
-						i -= childlen + 1;
-					}
+					i = rm_child_entry(node, i, childlen);
 				}
 				else {
 					if (!remember_string(children,
@@ -2399,11 +2407,7 @@ static int check_store_(const char *name, struct hashtable *reachable)
 			} else if (errno != ENOMEM) {
 				log("check_store: No child '%s' found!\n",
 				    childname);
-
-				if (recovery) {
-					remove_child_entry(NULL, node, i);
-					i -= childlen + 1;
-				}
+				i = rm_child_entry(node, i, childlen);
 			} else {
 				log("check_store: ENOMEM");
 				ret = ENOMEM;