[xen master] tools/xenstore: Fail a transaction if it is not possible to create a node

[patchbot@xxxxxxx]

commit 5d71766bd1a4a3a8b2fe952ca2be80e02fe48f34
Author:     Julien Grall <jgrall@xxxxxxxxxx>
AuthorDate: Tue Sep 13 07:35:06 2022 +0200
Commit:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CommitDate: Tue Nov 1 13:05:44 2022 +0000

    tools/xenstore: Fail a transaction if it is not possible to create a node
    
    Commit f2bebf72c4d5 "xenstore: rework of transaction handling" moved
    out from copying the entire database everytime a new transaction is
    opened to track the list of nodes changed.
    
    The content of all the nodes accessed during a transaction will be
    temporarily stored in TDB using a different key.
    
    The function create_node() may write/update multiple nodes if the child
    doesn't exist. In case of a failure, the function will revert any
    changes (this include any update to TDB). Unfortunately, the function
    which reverts the changes (i.e. destroy_node()) will not use the correct
    key to delete any update or even request the transaction to fail.
    
    This means that if a client decide to go ahead with committing the
    transaction, orphan nodes will be created because they were not linked
    to an existing node (create_node() will write the nodes backwards).
    
    Once some nodes have been partially updated in a transaction, it is not
    easily possible to undo any changes. So rather than continuing and hit
    weird issue while committing, it is much saner to fail the transaction.
    
    This will have an impact on any client that decides to commit even if it
    can't write a node. Although, it is not clear why a normal client would
    want to do that...
    
    Lastly, update destroy_node() to use the correct key for deleting the
    node. Rather than recreating it (this will allocate memory and
    therefore fail), stash the key in the structure node.
    
    This is XSA-415 / CVE-2022-42310.
    
    Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
    Reviewed-by: Juergen Gross <jgross@xxxxxxxx>
---
 tools/xenstore/xenstored_core.c        | 23 +++++++++++++++--------
 tools/xenstore/xenstored_core.h        |  2 ++
 tools/xenstore/xenstored_transaction.c |  5 +++++
 tools/xenstore/xenstored_transaction.h |  3 +++
 4 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index c30d14cbf2..55b79e4c03 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -562,15 +562,17 @@ int write_node_raw(struct connection *conn, TDB_DATA 
*key, struct node *node,
        return 0;
 }
 
+/*
+ * Write the node. If the node is written, caller can find the key used in
+ * node->key. This can later be used if the change needs to be reverted.
+ */
 static int write_node(struct connection *conn, struct node *node,
                      bool no_quota_check)
 {
-       TDB_DATA key;
-
-       if (access_node(conn, node, NODE_ACCESS_WRITE, &key))
+       if (access_node(conn, node, NODE_ACCESS_WRITE, &node->key))
                return errno;
 
-       return write_node_raw(conn, &key, node, no_quota_check);
+       return write_node_raw(conn, &node->key, node, no_quota_check);
 }
 
 unsigned int perm_for_conn(struct connection *conn,
@@ -1086,16 +1088,21 @@ nomem:
 
 static int destroy_node(struct connection *conn, struct node *node)
 {
-       TDB_DATA key;
-
        if (streq(node->name, "/"))
                corrupt(NULL, "Destroying root node!");
 
-       set_tdb_key(node->name, &key);
-       tdb_delete(tdb_ctx, key);
+       tdb_delete(tdb_ctx, node->key);
 
        domain_entry_dec(conn, node);
 
+       /*
+        * It is not possible to easily revert the changes in a transaction.
+        * So if the failure happens in a transaction, mark it as fail to
+        * prevent any commit.
+        */
+       if ( conn->transaction )
+               fail_transaction(conn->transaction);
+
        return 0;
 }
 
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h
index 742812a974..7d0fe77e79 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -155,6 +155,8 @@ struct node_perms {
 
 struct node {
        const char *name;
+       /* Key used to update TDB */
+       TDB_DATA key;
 
        /* Parent (optional) */
        struct node *parent;
diff --git a/tools/xenstore/xenstored_transaction.c 
b/tools/xenstore/xenstored_transaction.c
index cd07fb0f21..faf6c930e4 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -580,6 +580,11 @@ void transaction_entry_dec(struct transaction *trans, 
unsigned int domid)
        list_add_tail(&d->list, &trans->changed_domains);
 }
 
+void fail_transaction(struct transaction *trans)
+{
+       trans->fail = true;
+}
+
 void conn_delete_all_transactions(struct connection *conn)
 {
        struct transaction *trans;
diff --git a/tools/xenstore/xenstored_transaction.h 
b/tools/xenstore/xenstored_transaction.h
index 43a162bea3..14062730e3 100644
--- a/tools/xenstore/xenstored_transaction.h
+++ b/tools/xenstore/xenstored_transaction.h
@@ -46,6 +46,9 @@ int access_node(struct connection *conn, struct node *node,
 int transaction_prepend(struct connection *conn, const char *name,
                         TDB_DATA *key);
 
+/* Mark the transaction as failed. This will prevent it to be committed. */
+void fail_transaction(struct transaction *trans);
+
 void conn_delete_all_transactions(struct connection *conn);
 int check_transactions(struct hashtable *hash);
 
--
generated by git-patchbot for /home/xen/git/xen.git#master