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, 0 insertions, 410 deletions
diff --git a/0085-tools-xenstore-harden-transaction-finalization-again.patch b/0085-tools-xenstore-harden-transaction-finalization-again.patch deleted file mode 100644 index 6718ae7..0000000 --- a/0085-tools-xenstore-harden-transaction-finalization-again.patch +++ /dev/null @@ -1,410 +0,0 @@ -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 - |