[xen master] tools/xenstore: don't let remove_child_entry() call corrupt()

[patchbot@xxxxxxx]

commit 0c00c51f3bc8206c7f9cf87d014650157bee2bf4
Author:     Juergen Gross <jgross@xxxxxxxx>
AuthorDate: Tue Sep 13 07:35:11 2022 +0200
Commit:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CommitDate: Tue Nov 1 13:05:44 2022 +0000

    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@xxxxxxxx>
    Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>
---
 tools/xenstore/xenstored_core.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index cb52f68d4d..5642917e67 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1604,15 +1604,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,
@@ -1622,7 +1622,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;
                }
        }
@@ -2304,6 +2306,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
@@ -2356,12 +2369,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,
@@ -2378,11 +2386,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;
--
generated by git-patchbot for /home/xen/git/xen.git#master