1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
|
From 28ea39a4eb476f9105e1021bef1367c075feaa0b Mon Sep 17 00:00:00 2001
From: Julien Grall <jgrall@amazon.com>
Date: Tue, 13 Sep 2022 07:35:06 +0200
Subject: [PATCH 44/87] tools/xenstore: create_node: Don't defer work to undo
any changes on failure
XSA-115 extended destroy_node() to update the node accounting for the
connection. The implementation is assuming the connection is the parent
of the node, however all the nodes are allocated using a separate context
(see process_message()). This will result to crash (or corrupt) xenstored
as the pointer is wrongly used.
In case of an error, any changes to the database or update to the
accounting will now be reverted in create_node() by calling directly
destroy_node(). This has the nice advantage to remove the loop to unset
the destructors in case of success.
Take the opportunity to free the nodes right now as they are not
going to be reachable (the function returns NULL) and are just wasting
resources.
This is XSA-414 / CVE-2022-42309.
Fixes: 0bfb2101f243 ("tools/xenstore: fix node accounting after failed node creation")
Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
(cherry picked from commit 1cd3cc7ea27cda7640a8d895e09617b61c265697)
---
tools/xenstore/xenstored_core.c | 47 ++++++++++++++++++++++-----------
1 file changed, 32 insertions(+), 15 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 0c8ee276f837..29947c3020c3 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1088,9 +1088,8 @@ nomem:
return NULL;
}
-static int destroy_node(void *_node)
+static int destroy_node(struct connection *conn, struct node *node)
{
- struct node *node = _node;
TDB_DATA key;
if (streq(node->name, "/"))
@@ -1099,7 +1098,7 @@ static int destroy_node(void *_node)
set_tdb_key(node->name, &key);
tdb_delete(tdb_ctx, key);
- domain_entry_dec(talloc_parent(node), node);
+ domain_entry_dec(conn, node);
return 0;
}
@@ -1108,7 +1107,8 @@ static struct node *create_node(struct connection *conn, const void *ctx,
const char *name,
void *data, unsigned int datalen)
{
- struct node *node, *i;
+ struct node *node, *i, *j;
+ int ret;
node = construct_node(conn, ctx, name);
if (!node)
@@ -1130,23 +1130,40 @@ static struct node *create_node(struct connection *conn, const void *ctx,
/* i->parent is set for each new node, so check quota. */
if (i->parent &&
domain_entry(conn) >= quota_nb_entry_per_domain) {
- errno = ENOSPC;
- return NULL;
+ ret = ENOSPC;
+ goto err;
}
- if (write_node(conn, i, false))
- return NULL;
- /* Account for new node, set destructor for error case. */
- if (i->parent) {
+ ret = write_node(conn, i, false);
+ if (ret)
+ goto err;
+
+ /* Account for new node */
+ if (i->parent)
domain_entry_inc(conn, i);
- talloc_set_destructor(i, destroy_node);
- }
}
- /* OK, now remove destructors so they stay around */
- for (i = node; i->parent; i = i->parent)
- talloc_set_destructor(i, NULL);
return node;
+
+err:
+ /*
+ * We failed to update TDB for some of the nodes. Undo any work that
+ * have already been done.
+ */
+ for (j = node; j != i; j = j->parent)
+ destroy_node(conn, j);
+
+ /* We don't need to keep the nodes around, so free them. */
+ i = node;
+ while (i) {
+ j = i;
+ i = i->parent;
+ talloc_free(j);
+ }
+
+ errno = ret;
+
+ return NULL;
}
/* path, data... */
--
2.37.4
|
GitWeb