diff options
Diffstat (limited to '0085-tools-xenstore-harden-transaction-finalization-again.patch')
-rw-r--r-- | 0085-tools-xenstore-harden-transaction-finalization-again.patch | 410 |
1 files changed, 410 insertions, 0 deletions
diff --git a/0085-tools-xenstore-harden-transaction-finalization-again.patch b/0085-tools-xenstore-harden-transaction-finalization-again.patch new file mode 100644 index 0000000..6718ae7 --- /dev/null +++ b/0085-tools-xenstore-harden-transaction-finalization-again.patch @@ -0,0 +1,410 @@ +From 1bdd7c438b399e2ecce9e3c72bd7c1ae56df60f8 Mon Sep 17 00:00:00 2001 +From: Juergen Gross <jgross@suse.com> +Date: Tue, 13 Sep 2022 07:35:14 +0200 +Subject: [PATCH 85/87] 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@suse.com> +Reviewed-by: Julien Grall <jgrall@amazon.com> +Tested-by: Julien Grall <jgrall@amazon.com> +(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 041124d8b7a5..ccb7f0a92578 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 7ffe21bb5285..ac854197cadb 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 39d7f81c5127..3417303f9427 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); +-- +2.37.4 + |