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.

Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
(cherry picked from commit 0c00c51f3bc8206c7f9cf87d014650157bee2bf4)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 8ae0842ca3cbc7ae9ef3d7c7bcc2dd1db9409cea..006af157650eca0bb7f69caef1baf150b3f3342e 100644 (file)
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1503,15 +1503,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,
@@ -1521,7 +1521,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;
                }
        }
@@ -2123,6 +2125,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
@@ -2171,12 +2184,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,
@@ -2193,11 +2201,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;