[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[xen staging-4.16] tools/xenstore: harden transaction finalization against errors



commit 1bdd7c438b399e2ecce9e3c72bd7c1ae56df60f8
Author:     Juergen Gross <jgross@xxxxxxxx>
AuthorDate: Tue Sep 13 07:35:14 2022 +0200
Commit:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
CommitDate: Tue Nov 1 14:07:24 2022 +0000

    tools/xenstore: harden transaction finalization against errors
    
    When finalizing a transaction, any error occurring after checking for
    conflicts will result in the transaction being performed only
    partially today. Additionally accounting data will not be updated at
    the end of the transaction, which might result in further problems
    later.
    
    Avoid those problems by multiple modifications:
    
    - free any transaction specific nodes which don't need to be committed
      as they haven't been written during the transaction as soon as their
      generation count has been verified, this will reduce the risk of
      out-of-memory situations
    
    - store the transaction specific node name in struct accessed_node in
      order to avoid the need to allocate additional memory for it when
      finalizing the transaction
    
    - don't stop the transaction finalization when hitting an error
      condition, but try to continue to handle all modified nodes
    
    - in case of a detected error do the accounting update as needed and
      call the data base checking only after that
    
    - if writing a node in a transaction is failing (e.g. due to a failed
      quota check), fail the transaction, as prior changes to struct
      accessed_node can't easily be undone in that case
    
    This is part of XSA-421 / CVE-2022-42326.
    
    Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
    Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>
    Tested-by: Julien Grall <jgrall@xxxxxxxxxx>
    (cherry picked from commit 2dd823ca7237e7fb90c890642d6a3b357a26fcff)
---
 tools/xenstore/xenstored_core.c        |  16 ++-
 tools/xenstore/xenstored_transaction.c | 171 +++++++++++++++------------------
 tools/xenstore/xenstored_transaction.h |   4 +-
 3 files changed, 92 insertions(+), 99 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 041124d8b7..ccb7f0a925 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -727,8 +727,7 @@ struct node *read_node(struct connection *conn, const void 
*ctx,
                return NULL;
        }
 
-       if (transaction_prepend(conn, name, &key))
-               return NULL;
+       transaction_prepend(conn, name, &key);
 
        data = tdb_fetch(tdb_ctx, key);
 
@@ -846,10 +845,21 @@ int write_node_raw(struct connection *conn, TDB_DATA 
*key, struct node *node,
 static int write_node(struct connection *conn, struct node *node,
                      bool no_quota_check)
 {
+       int ret;
+
        if (access_node(conn, node, NODE_ACCESS_WRITE, &node->key))
                return errno;
 
-       return write_node_raw(conn, &node->key, node, no_quota_check);
+       ret = write_node_raw(conn, &node->key, node, no_quota_check);
+       if (ret && conn && conn->transaction) {
+               /*
+                * Reverting access_node() is hard, so just fail the
+                * transaction.
+                */
+               fail_transaction(conn->transaction);
+       }
+
+       return ret;
 }
 
 unsigned int perm_for_conn(struct connection *conn,
diff --git a/tools/xenstore/xenstored_transaction.c 
b/tools/xenstore/xenstored_transaction.c
index 7ffe21bb52..ac854197ca 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -114,7 +114,8 @@ struct accessed_node
        struct list_head list;
 
        /* The name of the node. */
-       char *node;
+       char *trans_name;       /* Transaction specific name. */
+       char *node;             /* Main data base name. */
 
        /* Generation count (or NO_GENERATION) for conflict checking. */
        uint64_t generation;
@@ -199,25 +200,20 @@ static char *transaction_get_node_name(void *ctx, struct 
transaction *trans,
  * Prepend the transaction to name if node has been modified in the current
  * transaction.
  */
-int transaction_prepend(struct connection *conn, const char *name,
-                       TDB_DATA *key)
+void transaction_prepend(struct connection *conn, const char *name,
+                        TDB_DATA *key)
 {
-       char *tdb_name;
+       struct accessed_node *i;
 
-       if (!conn || !conn->transaction ||
-           !find_accessed_node(conn->transaction, name)) {
-               set_tdb_key(name, key);
-               return 0;
+       if (conn && conn->transaction) {
+               i = find_accessed_node(conn->transaction, name);
+               if (i) {
+                       set_tdb_key(i->trans_name, key);
+                       return;
+               }
        }
 
-       tdb_name = transaction_get_node_name(conn->transaction,
-                                            conn->transaction, name);
-       if (!tdb_name)
-               return errno;
-
-       set_tdb_key(tdb_name, key);
-
-       return 0;
+       set_tdb_key(name, key);
 }
 
 /*
@@ -240,7 +236,6 @@ int access_node(struct connection *conn, struct node *node,
        struct accessed_node *i = NULL;
        struct transaction *trans;
        TDB_DATA local_key;
-       const char *trans_name = NULL;
        int ret;
        bool introduce = false;
 
@@ -259,10 +254,6 @@ int access_node(struct connection *conn, struct node *node,
 
        trans = conn->transaction;
 
-       trans_name = transaction_get_node_name(node, trans, node->name);
-       if (!trans_name)
-               goto nomem;
-
        i = find_accessed_node(trans, node->name);
        if (!i) {
                if (trans->nodes >= quota_trans_nodes &&
@@ -273,9 +264,10 @@ int access_node(struct connection *conn, struct node *node,
                i = talloc_zero(trans, struct accessed_node);
                if (!i)
                        goto nomem;
-               i->node = talloc_strdup(i, node->name);
-               if (!i->node)
+               i->trans_name = transaction_get_node_name(i, trans, node->name);
+               if (!i->trans_name)
                        goto nomem;
+               i->node = strchr(i->trans_name, '/') + 1;
                if (node->generation != NO_GENERATION && node->perms.num) {
                        i->perms.p = talloc_array(i, struct xs_permissions,
                                                  node->perms.num);
@@ -302,7 +294,7 @@ int access_node(struct connection *conn, struct node *node,
                        i->generation = node->generation;
                        i->check_gen = true;
                        if (node->generation != NO_GENERATION) {
-                               set_tdb_key(trans_name, &local_key);
+                               set_tdb_key(i->trans_name, &local_key);
                                ret = write_node_raw(conn, &local_key, node, 
true);
                                if (ret)
                                        goto err;
@@ -321,7 +313,7 @@ int access_node(struct connection *conn, struct node *node,
                return -1;
 
        if (key) {
-               set_tdb_key(trans_name, key);
+               set_tdb_key(i->trans_name, key);
                if (type == NODE_ACCESS_WRITE)
                        i->ta_node = true;
                if (type == NODE_ACCESS_DELETE)
@@ -333,7 +325,6 @@ int access_node(struct connection *conn, struct node *node,
 nomem:
        ret = ENOMEM;
 err:
-       talloc_free((void *)trans_name);
        talloc_free(i);
        trans->fail = true;
        errno = ret;
@@ -371,100 +362,90 @@ void queue_watches(struct connection *conn, const char 
*name, bool watch_exact)
  * base.
  */
 static int finalize_transaction(struct connection *conn,
-                               struct transaction *trans)
+                               struct transaction *trans, bool *is_corrupt)
 {
-       struct accessed_node *i;
+       struct accessed_node *i, *n;
        TDB_DATA key, ta_key, data;
        struct xs_tdb_record_hdr *hdr;
        uint64_t gen;
-       char *trans_name;
-       int ret;
 
-       list_for_each_entry(i, &trans->accessed, list) {
-               if (!i->check_gen)
-                       continue;
+       list_for_each_entry_safe(i, n, &trans->accessed, list) {
+               if (i->check_gen) {
+                       set_tdb_key(i->node, &key);
+                       data = tdb_fetch(tdb_ctx, key);
+                       hdr = (void *)data.dptr;
+                       if (!data.dptr) {
+                               if (tdb_error(tdb_ctx) != TDB_ERR_NOEXIST)
+                                       return EIO;
+                               gen = NO_GENERATION;
+                       } else
+                               gen = hdr->generation;
+                       talloc_free(data.dptr);
+                       if (i->generation != gen)
+                               return EAGAIN;
+               }
 
-               set_tdb_key(i->node, &key);
-               data = tdb_fetch(tdb_ctx, key);
-               hdr = (void *)data.dptr;
-               if (!data.dptr) {
-                       if (tdb_error(tdb_ctx) != TDB_ERR_NOEXIST)
-                               return EIO;
-                       gen = NO_GENERATION;
-               } else
-                       gen = hdr->generation;
-               talloc_free(data.dptr);
-               if (i->generation != gen)
-                       return EAGAIN;
+               /* Entries for unmodified nodes can be removed early. */
+               if (!i->modified) {
+                       if (i->ta_node) {
+                               set_tdb_key(i->trans_name, &ta_key);
+                               if (do_tdb_delete(conn, &ta_key, NULL))
+                                       return EIO;
+                       }
+                       list_del(&i->list);
+                       talloc_free(i);
+               }
        }
 
        while ((i = list_top(&trans->accessed, struct accessed_node, list))) {
-               trans_name = transaction_get_node_name(i, trans, i->node);
-               if (!trans_name)
-                       /* We are doomed: the transaction is only partial. */
-                       goto err;
-
-               set_tdb_key(trans_name, &ta_key);
-
-               if (i->modified) {
-                       set_tdb_key(i->node, &key);
-                       if (i->ta_node) {
-                               data = tdb_fetch(tdb_ctx, ta_key);
-                               if (!data.dptr)
-                                       goto err;
+               set_tdb_key(i->node, &key);
+               if (i->ta_node) {
+                       set_tdb_key(i->trans_name, &ta_key);
+                       data = tdb_fetch(tdb_ctx, ta_key);
+                       if (data.dptr) {
                                hdr = (void *)data.dptr;
                                hdr->generation = ++generation;
-                               ret = do_tdb_write(conn, &key, &data, NULL,
-                                                  true);
+                               *is_corrupt |= do_tdb_write(conn, &key, &data,
+                                                           NULL, true);
                                talloc_free(data.dptr);
+                               if (do_tdb_delete(conn, &ta_key, NULL))
+                                       *is_corrupt = true;
                        } else {
-                               /*
-                                * A node having been created and later deleted
-                                * in this transaction will have no generation
-                                * information stored.
-                                */
-                               ret = (i->generation == NO_GENERATION)
-                                     ? 0 : do_tdb_delete(conn, &key, NULL);
-                       }
-                       if (ret)
-                               goto err;
-                       if (i->fire_watch) {
-                               fire_watches(conn, trans, i->node, NULL,
-                                            i->watch_exact,
-                                            i->perms.p ? &i->perms : NULL);
+                               *is_corrupt = true;
                        }
+               } else {
+                       /*
+                        * A node having been created and later deleted
+                        * in this transaction will have no generation
+                        * information stored.
+                        */
+                       *is_corrupt |= (i->generation == NO_GENERATION)
+                                      ? false
+                                      : do_tdb_delete(conn, &key, NULL);
                }
+               if (i->fire_watch)
+                       fire_watches(conn, trans, i->node, NULL, i->watch_exact,
+                                    i->perms.p ? &i->perms : NULL);
 
-               if (i->ta_node && do_tdb_delete(conn, &ta_key, NULL))
-                       goto err;
                list_del(&i->list);
                talloc_free(i);
        }
 
        return 0;
-
-err:
-       corrupt(conn, "Partial transaction");
-       return EIO;
 }
 
 static int destroy_transaction(void *_transaction)
 {
        struct transaction *trans = _transaction;
        struct accessed_node *i;
-       char *trans_name;
        TDB_DATA key;
 
        wrl_ntransactions--;
        trace_destroy(trans, "transaction");
        while ((i = list_top(&trans->accessed, struct accessed_node, list))) {
                if (i->ta_node) {
-                       trans_name = transaction_get_node_name(i, trans,
-                                                              i->node);
-                       if (trans_name) {
-                               set_tdb_key(trans_name, &key);
-                               do_tdb_delete(trans->conn, &key, NULL);
-                       }
+                       set_tdb_key(i->trans_name, &key);
+                       do_tdb_delete(trans->conn, &key, NULL);
                }
                list_del(&i->list);
                talloc_free(i);
@@ -556,6 +537,7 @@ int do_transaction_end(const void *ctx, struct connection 
*conn,
 {
        const char *arg = onearg(in);
        struct transaction *trans;
+       bool is_corrupt = false;
        int ret;
 
        if (!arg || (!streq(arg, "T") && !streq(arg, "F")))
@@ -579,13 +561,17 @@ int do_transaction_end(const void *ctx, struct connection 
*conn,
                ret = transaction_fix_domains(trans, false);
                if (ret)
                        return ret;
-               if (finalize_transaction(conn, trans))
-                       return EAGAIN;
+               ret = finalize_transaction(conn, trans, &is_corrupt);
+               if (ret)
+                       return ret;
 
                wrl_apply_debit_trans_commit(conn);
 
                /* fix domain entry for each changed domain */
                transaction_fix_domains(trans, true);
+
+               if (is_corrupt)
+                       corrupt(conn, "transaction inconsistency");
        }
        send_ack(conn, XS_TRANSACTION_END);
 
@@ -660,7 +646,7 @@ int check_transactions(struct hashtable *hash)
        struct connection *conn;
        struct transaction *trans;
        struct accessed_node *i;
-       char *tname, *tnode;
+       char *tname;
 
        list_for_each_entry(conn, &connections, list) {
                list_for_each_entry(trans, &conn->transaction_list, list) {
@@ -672,11 +658,8 @@ int check_transactions(struct hashtable *hash)
                        list_for_each_entry(i, &trans->accessed, list) {
                                if (!i->ta_node)
                                        continue;
-                               tnode = transaction_get_node_name(tname, trans,
-                                                                 i->node);
-                               if (!tnode || !remember_string(hash, tnode))
+                               if (!remember_string(hash, i->trans_name))
                                        goto nomem;
-                               talloc_free(tnode);
                        }
 
                        talloc_free(tname);
diff --git a/tools/xenstore/xenstored_transaction.h 
b/tools/xenstore/xenstored_transaction.h
index 39d7f81c51..3417303f94 100644
--- a/tools/xenstore/xenstored_transaction.h
+++ b/tools/xenstore/xenstored_transaction.h
@@ -48,8 +48,8 @@ int __must_check access_node(struct connection *conn, struct 
node *node,
 void queue_watches(struct connection *conn, const char *name, bool 
watch_exact);
 
 /* Prepend the transaction to name if appropriate. */
-int transaction_prepend(struct connection *conn, const char *name,
-                        TDB_DATA *key);
+void 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);
--
generated by git-patchbot for /home/xen/git/xen.git#staging-4.16



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.