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
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
319
320
321
322
323
324
325
326
327
328
329
330
331
332
333
334
335
336
|
From 8ee7ed7c1ef435f43edc08be07c036d81642d8e1 Mon Sep 17 00:00:00 2001
From: Juergen Gross <jgross@suse.com>
Date: Tue, 13 Sep 2022 07:35:08 +0200
Subject: [PATCH 092/126] tools/xenstore: simplify and fix per domain node
accounting
The accounting of nodes can be simplified now that each connection
holds the associated domid.
Fix the node accounting to cover nodes created for a domain before it
has been introduced. This requires to react properly to an allocation
failure inside domain_entry_inc() by returning an error code.
Especially in error paths the node accounting has to be fixed in some
cases.
This is part of XSA-326 / CVE-2022-42313.
Signed-off-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
(cherry picked from commit dbef1f7482894c572d90cd73d99ed689c891e863)
---
tools/xenstore/xenstored_core.c | 43 ++++++++--
tools/xenstore/xenstored_domain.c | 105 ++++++++++++++++---------
tools/xenstore/xenstored_domain.h | 4 +-
tools/xenstore/xenstored_transaction.c | 8 +-
4 files changed, 109 insertions(+), 51 deletions(-)
diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 6ea06e20df91..85c0d2f38fac 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -603,7 +603,7 @@ struct node *read_node(struct connection *conn, const void *ctx,
/* Permissions are struct xs_permissions. */
node->perms.p = hdr->perms;
- if (domain_adjust_node_perms(node)) {
+ if (domain_adjust_node_perms(conn, node)) {
talloc_free(node);
return NULL;
}
@@ -625,7 +625,7 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node,
void *p;
struct xs_tdb_record_hdr *hdr;
- if (domain_adjust_node_perms(node))
+ if (domain_adjust_node_perms(conn, node))
return errno;
data.dsize = sizeof(*hdr)
@@ -1238,13 +1238,17 @@ nomem:
return NULL;
}
-static int destroy_node(struct connection *conn, struct node *node)
+static void destroy_node_rm(struct node *node)
{
if (streq(node->name, "/"))
corrupt(NULL, "Destroying root node!");
tdb_delete(tdb_ctx, node->key);
+}
+static int destroy_node(struct connection *conn, struct node *node)
+{
+ destroy_node_rm(node);
domain_entry_dec(conn, node);
/*
@@ -1294,8 +1298,12 @@ static struct node *create_node(struct connection *conn, const void *ctx,
goto err;
/* Account for new node */
- if (i->parent)
- domain_entry_inc(conn, i);
+ if (i->parent) {
+ if (domain_entry_inc(conn, i)) {
+ destroy_node_rm(i);
+ return NULL;
+ }
+ }
}
return node;
@@ -1580,10 +1588,27 @@ static int do_set_perms(struct connection *conn, struct buffered_data *in)
old_perms = node->perms;
domain_entry_dec(conn, node);
node->perms = perms;
- domain_entry_inc(conn, node);
+ if (domain_entry_inc(conn, node)) {
+ node->perms = old_perms;
+ /*
+ * This should never fail because we had a reference on the
+ * domain before and Xenstored is single-threaded.
+ */
+ domain_entry_inc(conn, node);
+ return ENOMEM;
+ }
- if (write_node(conn, node, false))
+ if (write_node(conn, node, false)) {
+ int saved_errno = errno;
+
+ domain_entry_dec(conn, node);
+ node->perms = old_perms;
+ /* No failure possible as above. */
+ domain_entry_inc(conn, node);
+
+ errno = saved_errno;
return errno;
+ }
fire_watches(conn, in, name, node, false, &old_perms);
send_ack(conn, XS_SET_PERMS);
@@ -3003,7 +3028,9 @@ void read_state_node(const void *ctx, const void *state)
set_tdb_key(name, &key);
if (write_node_raw(NULL, &key, node, true))
barf("write node error restoring node");
- domain_entry_inc(&conn, node);
+
+ if (domain_entry_inc(&conn, node))
+ barf("node accounting error restoring node");
talloc_free(node);
}
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c
index 979f8c629835..3c27973fb836 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -16,6 +16,7 @@
along with this program; If not, see <http://www.gnu.org/licenses/>.
*/
+#include <assert.h>
#include <stdio.h>
#include <sys/mman.h>
#include <unistd.h>
@@ -369,6 +370,18 @@ static struct domain *find_or_alloc_domain(const void *ctx, unsigned int domid)
return domain ? : alloc_domain(ctx, domid);
}
+static struct domain *find_or_alloc_existing_domain(unsigned int domid)
+{
+ struct domain *domain;
+ xc_dominfo_t dominfo;
+
+ domain = find_domain_struct(domid);
+ if (!domain && get_domain_info(domid, &dominfo))
+ domain = alloc_domain(NULL, domid);
+
+ return domain;
+}
+
static int new_domain(struct domain *domain, int port, bool restore)
{
int rc;
@@ -788,30 +801,28 @@ void domain_deinit(void)
xenevtchn_unbind(xce_handle, virq_port);
}
-void domain_entry_inc(struct connection *conn, struct node *node)
+int domain_entry_inc(struct connection *conn, struct node *node)
{
struct domain *d;
+ unsigned int domid;
if (!conn)
- return;
+ return 0;
- if (node->perms.p && node->perms.p[0].id != conn->id) {
- if (conn->transaction) {
- transaction_entry_inc(conn->transaction,
- node->perms.p[0].id);
- } else {
- d = find_domain_by_domid(node->perms.p[0].id);
- if (d)
- d->nbentry++;
- }
- } else if (conn->domain) {
- if (conn->transaction) {
- transaction_entry_inc(conn->transaction,
- conn->domain->domid);
- } else {
- conn->domain->nbentry++;
- }
+ domid = node->perms.p ? node->perms.p[0].id : conn->id;
+
+ if (conn->transaction) {
+ transaction_entry_inc(conn->transaction, domid);
+ } else {
+ d = (domid == conn->id && conn->domain) ? conn->domain
+ : find_or_alloc_existing_domain(domid);
+ if (d)
+ d->nbentry++;
+ else
+ return ENOMEM;
}
+
+ return 0;
}
/*
@@ -847,7 +858,7 @@ static int chk_domain_generation(unsigned int domid, uint64_t gen)
* Remove permissions for no longer existing domains in order to avoid a new
* domain with the same domid inheriting the permissions.
*/
-int domain_adjust_node_perms(struct node *node)
+int domain_adjust_node_perms(struct connection *conn, struct node *node)
{
unsigned int i;
int ret;
@@ -857,8 +868,14 @@ int domain_adjust_node_perms(struct node *node)
return errno;
/* If the owner doesn't exist any longer give it to priv domain. */
- if (!ret)
+ if (!ret) {
+ /*
+ * In theory we'd need to update the number of dom0 nodes here,
+ * but we could be called for a read of the node. So better
+ * avoid the risk to overflow the node count of dom0.
+ */
node->perms.p[0].id = priv_domid;
+ }
for (i = 1; i < node->perms.num; i++) {
if (node->perms.p[i].perms & XS_PERM_IGNORE)
@@ -877,25 +894,25 @@ int domain_adjust_node_perms(struct node *node)
void domain_entry_dec(struct connection *conn, struct node *node)
{
struct domain *d;
+ unsigned int domid;
if (!conn)
return;
- if (node->perms.p && node->perms.p[0].id != conn->id) {
- if (conn->transaction) {
- transaction_entry_dec(conn->transaction,
- node->perms.p[0].id);
+ domid = node->perms.p ? node->perms.p[0].id : conn->id;
+
+ if (conn->transaction) {
+ transaction_entry_dec(conn->transaction, domid);
+ } else {
+ d = (domid == conn->id && conn->domain) ? conn->domain
+ : find_domain_struct(domid);
+ if (d) {
+ d->nbentry--;
} else {
- d = find_domain_by_domid(node->perms.p[0].id);
- if (d && d->nbentry)
- d->nbentry--;
- }
- } else if (conn->domain && conn->domain->nbentry) {
- if (conn->transaction) {
- transaction_entry_dec(conn->transaction,
- conn->domain->domid);
- } else {
- conn->domain->nbentry--;
+ errno = ENOENT;
+ corrupt(conn,
+ "Node \"%s\" owned by non-existing domain %u\n",
+ node->name, domid);
}
}
}
@@ -905,13 +922,23 @@ int domain_entry_fix(unsigned int domid, int num, bool update)
struct domain *d;
int cnt;
- d = find_domain_by_domid(domid);
- if (!d)
- return 0;
+ if (update) {
+ d = find_domain_struct(domid);
+ assert(d);
+ } else {
+ /*
+ * We are called first with update == false in order to catch
+ * any error. So do a possible allocation and check for error
+ * only in this case, as in the case of update == true nothing
+ * can go wrong anymore as the allocation already happened.
+ */
+ d = find_or_alloc_existing_domain(domid);
+ if (!d)
+ return -1;
+ }
cnt = d->nbentry + num;
- if (cnt < 0)
- cnt = 0;
+ assert(cnt >= 0);
if (update)
d->nbentry = cnt;
diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h
index 5757a6557146..cce13d14f016 100644
--- a/tools/xenstore/xenstored_domain.h
+++ b/tools/xenstore/xenstored_domain.h
@@ -58,10 +58,10 @@ bool domain_can_write(struct connection *conn);
bool domain_is_unprivileged(struct connection *conn);
/* Remove node permissions for no longer existing domains. */
-int domain_adjust_node_perms(struct node *node);
+int domain_adjust_node_perms(struct connection *conn, struct node *node);
/* Quota manipulation */
-void domain_entry_inc(struct connection *conn, struct node *);
+int domain_entry_inc(struct connection *conn, struct node *);
void domain_entry_dec(struct connection *conn, struct node *);
int domain_entry_fix(unsigned int domid, int num, bool update);
int domain_entry(struct connection *conn);
diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c
index ee1b09031a3b..86caf6c398be 100644
--- a/tools/xenstore/xenstored_transaction.c
+++ b/tools/xenstore/xenstored_transaction.c
@@ -519,8 +519,12 @@ static int transaction_fix_domains(struct transaction *trans, bool update)
list_for_each_entry(d, &trans->changed_domains, list) {
cnt = domain_entry_fix(d->domid, d->nbentry, update);
- if (!update && cnt >= quota_nb_entry_per_domain)
- return ENOSPC;
+ if (!update) {
+ if (cnt >= quota_nb_entry_per_domain)
+ return ENOSPC;
+ if (cnt < 0)
+ return ENOMEM;
+ }
}
return 0;
--
2.37.4
|