Sign Up
Log In
Log In
or
Sign Up
Places
All Projects
Status Monitor
Collapse sidebar
Please login to access the resource
SUSE:SLE-15-SP1:Update
xen
xsa421-02.patch
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File xsa421-02.patch of Package xen
From bb45167132b9988ef7282eef96beaa5c9845f9ad Mon Sep 17 00:00:00 2001 From: Juergen Gross <jgross@suse.com> Date: Tue, 13 Sep 2022 07:35:14 +0200 Subject: 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> --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -632,8 +632,7 @@ struct node *read_node(struct connection return NULL; } - if (transaction_prepend(conn, name, &key)) - return NULL; + transaction_prepend(conn, name, &key); data = tdb_fetch(tdb_ctx, key); @@ -746,10 +745,21 @@ int write_node_raw(struct connection *co 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; } enum xs_perm_type perm_for_conn(struct connection *conn, --- 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; @@ -205,25 +206,20 @@ static char *transaction_get_node_name(v * 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); } /* @@ -246,7 +242,6 @@ int access_node(struct connection *conn, struct accessed_node *i = NULL; struct transaction *trans; TDB_DATA local_key; - const char *trans_name = NULL; int ret; bool introduce = false; @@ -265,10 +260,6 @@ int access_node(struct connection *conn, 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 && @@ -279,9 +270,10 @@ int access_node(struct connection *conn, 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); @@ -308,7 +300,7 @@ int access_node(struct connection *conn, 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; @@ -327,7 +319,7 @@ int access_node(struct connection *conn, 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) @@ -339,7 +331,6 @@ int access_node(struct connection *conn, nomem: ret = ENOMEM; err: - talloc_free((void *)trans_name); talloc_free(i); trans->fail = true; errno = ret; @@ -377,100 +368,90 @@ void queue_watches(struct connection *co * 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); @@ -560,6 +541,7 @@ int do_transaction_end(const void *ctx, { const char *arg = onearg(in); struct transaction *trans; + bool is_corrupt = false; int ret; if (!arg || (!streq(arg, "T") && !streq(arg, "F"))) @@ -581,13 +563,17 @@ int do_transaction_end(const void *ctx, 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); @@ -661,7 +647,7 @@ int check_transactions(struct hashtable 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) { @@ -673,11 +659,8 @@ int check_transactions(struct hashtable 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); --- a/tools/xenstore/xenstored_transaction.h +++ b/tools/xenstore/xenstored_transaction.h @@ -48,8 +48,8 @@ int __must_check access_node(struct conn 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);
Locations
Projects
Search
Status Monitor
Help
OpenBuildService.org
Documentation
API Documentation
Code of Conduct
Contact
Support
@OBShq
Terms
openSUSE Build Service is sponsored by
The Open Build Service is an
openSUSE project
.
Sign Up
Log In
Places
Places
All Projects
Status Monitor
