Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified)) by mollari.NetBSD.org (Postfix) with ESMTPS id 469A51A921F for ; Thu, 17 Dec 2020 16:48:17 +0000 (UTC) Received: by mail.netbsd.org (Postfix, from userid 605) id A0F2984DD3; Thu, 17 Dec 2020 16:48:16 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by mail.netbsd.org (Postfix) with ESMTP id D76E284DD2 for ; Thu, 17 Dec 2020 16:48:15 +0000 (UTC) X-Virus-Scanned: amavisd-new at netbsd.org Received: from mail.netbsd.org ([127.0.0.1]) by localhost (mail.netbsd.org [127.0.0.1]) (amavisd-new, port 10025) with ESMTP id R7TTD5CGT3OY for ; Thu, 17 Dec 2020 16:48:12 +0000 (UTC) Received: from cvs.NetBSD.org (ivanova.NetBSD.org [IPv6:2001:470:a085:999:28c:faff:fe03:5984]) by mail.netbsd.org (Postfix) with ESMTP id 7185F84DC7 for ; Thu, 17 Dec 2020 16:48:12 +0000 (UTC) Received: by cvs.NetBSD.org (Postfix, from userid 500) id 688ADFA9D; Thu, 17 Dec 2020 16:48:12 +0000 (UTC) Content-Transfer-Encoding: 7bit Content-Type: multipart/mixed; boundary="_----------=_160822369248150" MIME-Version: 1.0 Date: Thu, 17 Dec 2020 16:48:12 +0000 From: "Manuel Bouyer" Subject: CVS commit: pkgsrc/sysutils/xentools411 To: pkgsrc-changes@NetBSD.org Reply-To: bouyer@netbsd.org X-Mailer: log_accum Message-Id: <20201217164812.688ADFA9D@cvs.NetBSD.org> Sender: pkgsrc-changes-owner@NetBSD.org List-Id: Precedence: bulk List-Unsubscribe: This is a multi-part message in MIME format. --_----------=_160822369248150 Content-Disposition: inline Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="US-ASCII" Module Name: pkgsrc Committed By: bouyer Date: Thu Dec 17 16:48:12 UTC 2020 Modified Files: pkgsrc/sysutils/xentools411: Makefile distinfo pkgsrc/sysutils/xentools411/patches: patch-tools_ocaml_xenstored_utils.ml Added Files: pkgsrc/sysutils/xentools411/patches: patch-XSA115-c patch-XSA115-o patch-XSA322-c patch-XSA322-o patch-XSA323 patch-XSA324 patch-XSA325 patch-XSA330 patch-XSA352 patch-XSA353 Log Message: Add upstream patches for a bunch of Xen security avisories, related to xenstore permissions. Bump PKGREVISION To generate a diff of this commit: cvs rdiff -u -r1.23 -r1.24 pkgsrc/sysutils/xentools411/Makefile cvs rdiff -u -r1.14 -r1.15 pkgsrc/sysutils/xentools411/distinfo cvs rdiff -u -r0 -r1.1 pkgsrc/sysutils/xentools411/patches/patch-XSA115-c \ pkgsrc/sysutils/xentools411/patches/patch-XSA115-o \ pkgsrc/sysutils/xentools411/patches/patch-XSA322-c \ pkgsrc/sysutils/xentools411/patches/patch-XSA322-o \ pkgsrc/sysutils/xentools411/patches/patch-XSA323 \ pkgsrc/sysutils/xentools411/patches/patch-XSA324 \ pkgsrc/sysutils/xentools411/patches/patch-XSA325 \ pkgsrc/sysutils/xentools411/patches/patch-XSA330 \ pkgsrc/sysutils/xentools411/patches/patch-XSA352 \ pkgsrc/sysutils/xentools411/patches/patch-XSA353 cvs rdiff -u -r1.1 -r1.2 \ pkgsrc/sysutils/xentools411/patches/patch-tools_ocaml_xenstored_utils.ml Please note that diffs are not public domain; they are subject to the copyright notices on the relevant files. --_----------=_160822369248150 Content-Disposition: inline Content-Length: 142666 Content-Transfer-Encoding: binary Content-Type: text/x-diff; charset=utf-8 Modified files: Index: pkgsrc/sysutils/xentools411/Makefile diff -u pkgsrc/sysutils/xentools411/Makefile:1.23 pkgsrc/sysutils/xentools411/Makefile:1.24 --- pkgsrc/sysutils/xentools411/Makefile:1.23 Mon Aug 31 18:11:37 2020 +++ pkgsrc/sysutils/xentools411/Makefile Thu Dec 17 16:48:12 2020 @@ -1,7 +1,7 @@ -# $NetBSD: Makefile,v 1.23 2020/08/31 18:11:37 wiz Exp $ +# $NetBSD: Makefile,v 1.24 2020/12/17 16:48:12 bouyer Exp $ # # VERSION is set in version.mk as it is shared with other packages -PKGREVISION= 1 +PKGREVISION= 2 .include "version.mk" DIST_IPXE= ipxe-git-${VERSION_IPXE}.tar.gz Index: pkgsrc/sysutils/xentools411/distinfo diff -u pkgsrc/sysutils/xentools411/distinfo:1.14 pkgsrc/sysutils/xentools411/distinfo:1.15 --- pkgsrc/sysutils/xentools411/distinfo:1.14 Mon Aug 24 10:33:27 2020 +++ pkgsrc/sysutils/xentools411/distinfo Thu Dec 17 16:48:12 2020 @@ -1,4 +1,4 @@ -$NetBSD: distinfo,v 1.14 2020/08/24 10:33:27 bouyer Exp $ +$NetBSD: distinfo,v 1.15 2020/12/17 16:48:12 bouyer Exp $ SHA1 (xen411/ipxe-git-356f6c1b64d7a97746d1816cef8ca22bdd8d0b5d.tar.gz) = 272b8c904dc0127690eca2c5c20c67479e40da34 RMD160 (xen411/ipxe-git-356f6c1b64d7a97746d1816cef8ca22bdd8d0b5d.tar.gz) = cfcb4a314c15da19b36132b27126f3bd9699d0e5 @@ -12,6 +12,16 @@ SHA1 (patch-.._ipxe_src_core_settings.c) SHA1 (patch-.._ipxe_src_net_fcels.c) = eda41b25c3d5f5bef33caa9a6af28c40cb91e66b SHA1 (patch-Config.mk) = c41005a60de2f94a72b0206030eb021c137653d3 SHA1 (patch-Makefile) = 2f3a5eafc5039b149c98dd5e59c39a3197fd9264 +SHA1 (patch-XSA115-c) = 7e3216a23c522fc73f47fa6deef8918c4dce7fae +SHA1 (patch-XSA115-o) = 6dc292060441c388b9a05e31ddc37835568a3e86 +SHA1 (patch-XSA322-c) = c48a10eeab29775b9c97a36848556120741a9c9d +SHA1 (patch-XSA322-o) = 943e2aee69ac278871925223478b11f6dfabc9d7 +SHA1 (patch-XSA323) = 98055b0c05ed0d0f5ebbe23d429a68a71d92f20f +SHA1 (patch-XSA324) = a1cdb872a79fd7d9234030ec2765d0a474f72fbb +SHA1 (patch-XSA325) = 59c7fba006588db4accee1068072612777620ac3 +SHA1 (patch-XSA330) = 03b4f1d9c14e11eaee5b863276d32cee0544e604 +SHA1 (patch-XSA352) = 7c4479c029d9bbbf6578ee148cb926bb2d849789 +SHA1 (patch-XSA353) = 6983aa18399dcf0ac1471ffdf7c27c1bc041f49c SHA1 (patch-always_inline) = 23201b2b63072e040630525416a0b61280492f93 SHA1 (patch-docs_man_xl-disk-configuration.pod.5) = 03ff4c22dde1e1b60ab8750c8971ea057e479151 SHA1 (patch-docs_man_xl.cfg.pod.5.in) = 951915037a9975b76cc5c41a0e1abe0a202a3696 @@ -56,7 +66,7 @@ SHA1 (patch-tools_ocaml_common.make) = 4 SHA1 (patch-tools_ocaml_libs_xentoollog_xentoollog__stubs.c) = adee03d87168e735cb0d42ce06d0c31a14315b8d SHA1 (patch-tools_ocaml_libs_xl__xenlight_stubs.c) = cc612908524670f650a294af133a5912f955f39e SHA1 (patch-tools_ocaml_xenstored_Makefile) = b267702cf4090c7b45bba530e60327fced24e3e5 -SHA1 (patch-tools_ocaml_xenstored_utils.ml) = fd951de732d6c31cae89bd4b58c5650108578d79 +SHA1 (patch-tools_ocaml_xenstored_utils.ml) = 96b69dd3b5adb10692d7646c1dbeb20d27e0e1a8 SHA1 (patch-tools_qemu-xen-traditional_Makefile) = 5fbb55bf84f9856043be301d5d06530190fe9a60 SHA1 (patch-tools_qemu-xen-traditional_block-raw-posix.c) = eb3efea4b0c7fd744f627f1926fca737ba826b99 SHA1 (patch-tools_qemu-xen-traditional_configure) = 6a42dcac010f90439a347c0f6e886b07185cb19a @@ -81,5 +91,4 @@ SHA1 (patch-tools_xenstore_xs_lib.c) = e SHA1 (patch-tools_xentrace_xentrace.c) = f964c7555f454358a39f28a2e75db8ee100a4243 SHA1 (patch-tools_xl_Makefile) = dd4fa8cc66c74eea8b022cd6129aa2831776f2a8 SHA1 (patch-xen_Rules.mk) = c743dc63f51fc280d529a7d9e08650292c171dac -SHA1 (patch-xen_common_lz4_dexompress.c) = 521a247c2d36980b3433c4be92c77308a2d3f3b9 SHA1 (patch-xen_tools_symbols.c) = 67b5a38312095029631e00457abc0e4bb633aaf8 Index: pkgsrc/sysutils/xentools411/patches/patch-tools_ocaml_xenstored_utils.ml diff -u pkgsrc/sysutils/xentools411/patches/patch-tools_ocaml_xenstored_utils.ml:1.1 pkgsrc/sysutils/xentools411/patches/patch-tools_ocaml_xenstored_utils.ml:1.2 --- pkgsrc/sysutils/xentools411/patches/patch-tools_ocaml_xenstored_utils.ml:1.1 Tue Jul 24 13:40:11 2018 +++ pkgsrc/sysutils/xentools411/patches/patch-tools_ocaml_xenstored_utils.ml Thu Dec 17 16:48:12 2020 @@ -1,13 +1,13 @@ -$NetBSD: patch-tools_ocaml_xenstored_utils.ml,v 1.1 2018/07/24 13:40:11 bouyer Exp $ +$NetBSD: patch-tools_ocaml_xenstored_utils.ml,v 1.2 2020/12/17 16:48:12 bouyer Exp $ ---- ./tools/ocaml/xenstored/utils.ml.orig 2018-07-09 15:47:19.000000000 +0200 -+++ ./tools/ocaml/xenstored/utils.ml 2018-07-16 13:50:03.000000000 +0200 +--- tools/ocaml/xenstored/utils.ml.orig 2020-12-17 15:47:15.866790468 +0100 ++++ tools/ocaml/xenstored/utils.ml 2020-12-17 15:53:47.618682147 +0100 @@ -86,7 +86,7 @@ let buf = Bytes.make 20 '\000' in let sz = Unix.read fd buf 0 20 in Unix.close fd; - int_of_string (Bytes.sub_string buf 0 sz) -+ int_of_string (String.trim (String.sub buf 0 sz)) ++ int_of_string (String.trim (Bytes.sub_string buf 0 sz)) - let path_complete path connection_path = - if String.get path 0 <> '/' then + (* @path may be guest data and needs its length validating. @connection_path + * is generated locally in xenstored and always of the form "/local/domain/$N/" *) Added files: Index: pkgsrc/sysutils/xentools411/patches/patch-XSA115-c diff -u /dev/null pkgsrc/sysutils/xentools411/patches/patch-XSA115-c:1.1 --- /dev/null Thu Dec 17 16:48:12 2020 +++ pkgsrc/sysutils/xentools411/patches/patch-XSA115-c Thu Dec 17 16:48:12 2020 @@ -0,0 +1,1755 @@ +$NetBSD: patch-XSA115-c,v 1.1 2020/12/17 16:48:12 bouyer Exp $ + +From e92f3dfeaae21a335e666c9247954424e34e5c56 Mon Sep 17 00:00:00 2001 +From: Juergen Gross +Date: Thu, 11 Jun 2020 16:12:37 +0200 +Subject: [PATCH 01/10] tools/xenstore: allow removing child of a node + exceeding quota + +An unprivileged user of Xenstore is not allowed to write nodes with a +size exceeding a global quota, while privileged users like dom0 are +allowed to write such nodes. The size of a node is the needed space +to store all node specific data, this includes the names of all +children of the node. + +When deleting a node its parent has to be modified by removing the +name of the to be deleted child from it. + +This results in the strange situation that an unprivileged owner of a +node might not succeed in deleting that node in case its parent is +exceeding the quota of that unprivileged user (it might have been +written by dom0), as the user is not allowed to write the updated +parent node. + +Fix that by not checking the quota when writing a node for the +purpose of removing a child's name only. + +The same applies to transaction handling: a node being read during a +transaction is written to the transaction specific area and it should +not be tested for exceeding the quota, as it might not be owned by +the reader and presumably the original write would have failed if the +node is owned by the reader. + +This is part of XSA-115. + +Signed-off-by: Juergen Gross +Reviewed-by: Julien Grall +Reviewed-by: Paul Durrant +--- + tools/xenstore/xenstored_core.c | 20 +++++++++++--------- + tools/xenstore/xenstored_core.h | 3 ++- + tools/xenstore/xenstored_transaction.c | 2 +- + 3 files changed, 14 insertions(+), 11 deletions(-) + +diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c +index 97ceabf9642d..b43e1018babd 100644 +--- tools/xenstore/xenstored_core.c.orig ++++ tools/xenstore/xenstored_core.c +@@ -417,7 +417,8 @@ static struct node *read_node(struct connection *conn, const void *ctx, + return node; + } + +-int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node) ++int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, ++ bool no_quota_check) + { + TDB_DATA data; + void *p; +@@ -427,7 +428,7 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node) + + node->num_perms*sizeof(node->perms[0]) + + node->datalen + node->childlen; + +- if (domain_is_unprivileged(conn) && ++ if (!no_quota_check && domain_is_unprivileged(conn) && + data.dsize >= quota_max_entry_size) { + errno = ENOSPC; + return errno; +@@ -455,14 +456,15 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node) + return 0; + } + +-static int write_node(struct connection *conn, struct node *node) ++static int write_node(struct connection *conn, struct node *node, ++ bool no_quota_check) + { + TDB_DATA key; + + if (access_node(conn, node, NODE_ACCESS_WRITE, &key)) + return errno; + +- return write_node_raw(conn, &key, node); ++ return write_node_raw(conn, &key, node, no_quota_check); + } + + static enum xs_perm_type perm_for_conn(struct connection *conn, +@@ -999,7 +1001,7 @@ static struct node *create_node(struct connection *conn, const void *ctx, + /* We write out the nodes down, setting destructor in case + * something goes wrong. */ + for (i = node; i; i = i->parent) { +- if (write_node(conn, i)) { ++ if (write_node(conn, i, false)) { + domain_entry_dec(conn, i); + return NULL; + } +@@ -1039,7 +1041,7 @@ static int do_write(struct connection *conn, struct buffered_data *in) + } else { + node->data = in->buffer + offset; + node->datalen = datalen; +- if (write_node(conn, node)) ++ if (write_node(conn, node, false)) + return errno; + } + +@@ -1115,7 +1117,7 @@ static int remove_child_entry(struct connection *conn, struct node *node, + size_t childlen = strlen(node->children + offset); + memdel(node->children, offset, childlen + 1, node->childlen); + node->childlen -= childlen + 1; +- return write_node(conn, node); ++ return write_node(conn, node, true); + } + + +@@ -1254,7 +1256,7 @@ static int do_set_perms(struct connection *conn, struct buffered_data *in) + node->num_perms = num; + domain_entry_inc(conn, node); + +- if (write_node(conn, node)) ++ if (write_node(conn, node, false)) + return errno; + + fire_watches(conn, in, name, false); +@@ -1514,7 +1516,7 @@ static void manual_node(const char *name, const char *child) + if (child) + node->childlen = strlen(child) + 1; + +- if (write_node(NULL, node)) ++ if (write_node(NULL, node, false)) + barf_perror("Could not create initial node %s", name); + talloc_free(node); + } +diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h +index 56a279cfbb47..3cb1c235a101 100644 +--- tools/xenstore/xenstored_core.h.orig ++++ tools/xenstore/xenstored_core.h +@@ -149,7 +149,8 @@ void send_ack(struct connection *conn, enum xsd_sockmsg_type type); + char *canonicalize(struct connection *conn, const void *ctx, const char *node); + + /* Write a node to the tdb data base. */ +-int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node); ++int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, ++ bool no_quota_check); + + /* Get this node, checking we have permissions. */ + struct node *get_node(struct connection *conn, +diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c +index 2824f7b359b8..e87897573469 100644 +--- tools/xenstore/xenstored_transaction.c.orig ++++ tools/xenstore/xenstored_transaction.c +@@ -276,7 +276,7 @@ int access_node(struct connection *conn, struct node *node, + i->check_gen = true; + if (node->generation != NO_GENERATION) { + set_tdb_key(trans_name, &local_key); +- ret = write_node_raw(conn, &local_key, node); ++ ret = write_node_raw(conn, &local_key, node, true); + if (ret) + goto err; + i->ta_node = true; +-- +2.17.1 + +From e8076f73de65c4816f69d6ebf75839c706145fcd Mon Sep 17 00:00:00 2001 +From: Juergen Gross +Date: Thu, 11 Jun 2020 16:12:38 +0200 +Subject: [PATCH 02/10] tools/xenstore: ignore transaction id for [un]watch + +Instead of ignoring the transaction id for XS_WATCH and XS_UNWATCH +commands as it is documented in docs/misc/xenstore.txt, it is tested +for validity today. + +Really ignore the transaction id for XS_WATCH and XS_UNWATCH. + +This is part of XSA-115. + +Signed-off-by: Juergen Gross +Reviewed-by: Julien Grall +Reviewed-by: Paul Durrant +--- + tools/xenstore/xenstored_core.c | 26 ++++++++++++++++---------- + 1 file changed, 16 insertions(+), 10 deletions(-) + +diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c +index b43e1018babd..bb2f9fd4e76e 100644 +--- tools/xenstore/xenstored_core.c.orig ++++ tools/xenstore/xenstored_core.c +@@ -1268,13 +1268,17 @@ static int do_set_perms(struct connection *conn, struct buffered_data *in) + static struct { + const char *str; + int (*func)(struct connection *conn, struct buffered_data *in); ++ unsigned int flags; ++#define XS_FLAG_NOTID (1U << 0) /* Ignore transaction id. */ + } const wire_funcs[XS_TYPE_COUNT] = { + [XS_CONTROL] = { "CONTROL", do_control }, + [XS_DIRECTORY] = { "DIRECTORY", send_directory }, + [XS_READ] = { "READ", do_read }, + [XS_GET_PERMS] = { "GET_PERMS", do_get_perms }, +- [XS_WATCH] = { "WATCH", do_watch }, +- [XS_UNWATCH] = { "UNWATCH", do_unwatch }, ++ [XS_WATCH] = ++ { "WATCH", do_watch, XS_FLAG_NOTID }, ++ [XS_UNWATCH] = ++ { "UNWATCH", do_unwatch, XS_FLAG_NOTID }, + [XS_TRANSACTION_START] = { "TRANSACTION_START", do_transaction_start }, + [XS_TRANSACTION_END] = { "TRANSACTION_END", do_transaction_end }, + [XS_INTRODUCE] = { "INTRODUCE", do_introduce }, +@@ -1296,7 +1300,7 @@ static struct { + + static const char *sockmsg_string(enum xsd_sockmsg_type type) + { +- if ((unsigned)type < XS_TYPE_COUNT && wire_funcs[type].str) ++ if ((unsigned int)type < ARRAY_SIZE(wire_funcs) && wire_funcs[type].str) + return wire_funcs[type].str; + + return "**UNKNOWN**"; +@@ -1311,7 +1315,14 @@ static void process_message(struct connection *conn, struct buffered_data *in) + enum xsd_sockmsg_type type = in->hdr.msg.type; + int ret; + +- trans = transaction_lookup(conn, in->hdr.msg.tx_id); ++ if ((unsigned int)type >= XS_TYPE_COUNT || !wire_funcs[type].func) { ++ eprintf("Client unknown operation %i", type); ++ send_error(conn, ENOSYS); ++ return; ++ } ++ ++ trans = (wire_funcs[type].flags & XS_FLAG_NOTID) ++ ? NULL : transaction_lookup(conn, in->hdr.msg.tx_id); + if (IS_ERR(trans)) { + send_error(conn, -PTR_ERR(trans)); + return; +@@ -1320,12 +1331,7 @@ static void process_message(struct connection *conn, struct buffered_data *in) + assert(conn->transaction == NULL); + conn->transaction = trans; + +- if ((unsigned)type < XS_TYPE_COUNT && wire_funcs[type].func) +- ret = wire_funcs[type].func(conn, in); +- else { +- eprintf("Client unknown operation %i", type); +- ret = ENOSYS; +- } ++ ret = wire_funcs[type].func(conn, in); + if (ret) + send_error(conn, ret); + +-- +2.17.1 + +From b8c6dbb67ebb449126023446a7d209eedf966537 Mon Sep 17 00:00:00 2001 +From: Juergen Gross +Date: Thu, 11 Jun 2020 16:12:39 +0200 +Subject: [PATCH 03/10] tools/xenstore: fix node accounting after failed node + creation + +When a node creation fails the number of nodes of the domain should be +the same as before the failed node creation. In case of failure when +trying to create a node requiring to create one or more intermediate +nodes as well (e.g. when /a/b/c/d is to be created, but /a/b isn't +existing yet) it might happen that the number of nodes of the creating +domain is not reset to the value it had before. + +So move the quota accounting out of construct_node() and into the node +write loop in create_node() in order to be able to undo the accounting +in case of an error in the intermediate node destructor. + +This is part of XSA-115. + +Signed-off-by: Juergen Gross +Reviewed-by: Paul Durrant +Acked-by: Julien Grall +--- + tools/xenstore/xenstored_core.c | 37 ++++++++++++++++++++++----------- + 1 file changed, 25 insertions(+), 12 deletions(-) + +diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c +index bb2f9fd4e76e..db9b9ca7957d 100644 +--- tools/xenstore/xenstored_core.c.orig ++++ tools/xenstore/xenstored_core.c +@@ -925,11 +925,6 @@ static struct node *construct_node(struct connection *conn, const void *ctx, + if (!parent) + return NULL; + +- if (domain_entry(conn) >= quota_nb_entry_per_domain) { +- errno = ENOSPC; +- return NULL; +- } +- + /* Add child to parent. */ + base = basename(name); + baselen = strlen(base) + 1; +@@ -962,7 +957,6 @@ static struct node *construct_node(struct connection *conn, const void *ctx, + node->children = node->data = NULL; + node->childlen = node->datalen = 0; + node->parent = parent; +- domain_entry_inc(conn, node); + return node; + + nomem: +@@ -982,6 +976,9 @@ static int destroy_node(void *_node) + key.dsize = strlen(node->name); + + tdb_delete(tdb_ctx, key); ++ ++ domain_entry_dec(talloc_parent(node), node); ++ + return 0; + } + +@@ -998,18 +995,34 @@ static struct node *create_node(struct connection *conn, const void *ctx, + node->data = data; + node->datalen = datalen; + +- /* We write out the nodes down, setting destructor in case +- * something goes wrong. */ ++ /* ++ * We write out the nodes bottom up. ++ * All new created nodes will have i->parent set, while the final ++ * node will be already existing and won't have i->parent set. ++ * New nodes are subject to quota handling. ++ * Initially set a destructor for all new nodes removing them from ++ * TDB again and undoing quota accounting for the case of an error ++ * during the write loop. ++ */ + for (i = node; i; i = i->parent) { +- if (write_node(conn, i, false)) { +- domain_entry_dec(conn, i); ++ /* 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; + } +- talloc_set_destructor(i, destroy_node); ++ if (write_node(conn, i, false)) ++ return NULL; ++ ++ /* Account for new node, set destructor for error case. */ ++ 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; i = i->parent) ++ for (i = node; i->parent; i = i->parent) + talloc_set_destructor(i, NULL); + return node; + } +-- +2.17.1 + +From 318aa75bd0c05423e717ad0b64adb204282025db Mon Sep 17 00:00:00 2001 +From: Juergen Gross +Date: Thu, 11 Jun 2020 16:12:40 +0200 +Subject: [PATCH 04/10] tools/xenstore: simplify and rename check_event_node() + +There is no path which allows to call check_event_node() without a +event name. So don't let the result depend on the name being NULL and +add an assert() covering that case. + +Rename the function to check_special_event() to better match the +semantics. + +This is part of XSA-115. + +Signed-off-by: Juergen Gross +Reviewed-by: Julien Grall +Reviewed-by: Paul Durrant +--- + tools/xenstore/xenstored_watch.c | 12 +++++------- + 1 file changed, 5 insertions(+), 7 deletions(-) + +diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c +index 7dedca60dfd6..f2f1bed47cc6 100644 +--- tools/xenstore/xenstored_watch.c.orig ++++ tools/xenstore/xenstored_watch.c +@@ -47,13 +47,11 @@ struct watch + char *node; + }; + +-static bool check_event_node(const char *node) ++static bool check_special_event(const char *name) + { +- if (!node || !strstarts(node, "@")) { +- errno = EINVAL; +- return false; +- } +- return true; ++ assert(name); ++ ++ return strstarts(name, "@"); + } + + /* Is child a subnode of parent, or equal? */ +@@ -87,7 +85,7 @@ static void add_event(struct connection *conn, + unsigned int len; + char *data; + +- if (!check_event_node(name)) { ++ if (!check_special_event(name)) { + /* Can this conn load node, or see that it doesn't exist? */ + struct node *node = get_node(conn, ctx, name, XS_PERM_READ); + /* +-- +2.17.1 + +From c625fae44aedc246776b52eb1173cf847a3d4d80 Mon Sep 17 00:00:00 2001 +From: Juergen Gross +Date: Thu, 11 Jun 2020 16:12:41 +0200 +Subject: [PATCH 05/10] tools/xenstore: check privilege for + XS_IS_DOMAIN_INTRODUCED + +The Xenstore command XS_IS_DOMAIN_INTRODUCED should be possible for +privileged domains only (the only user in the tree is the xenpaging +daemon). + +Instead of having the privilege test for each command introduce a +per-command flag for that purpose. + +This is part of XSA-115. + +Signed-off-by: Juergen Gross +Reviewed-by: Julien Grall +Reviewed-by: Paul Durrant +--- + tools/xenstore/xenstored_core.c | 24 ++++++++++++++++++------ + tools/xenstore/xenstored_domain.c | 7 ++----- + 2 files changed, 20 insertions(+), 11 deletions(-) + +diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c +index db9b9ca7957d..6afd58431111 100644 +--- tools/xenstore/xenstored_core.c.orig ++++ tools/xenstore/xenstored_core.c +@@ -1283,8 +1283,10 @@ static struct { + int (*func)(struct connection *conn, struct buffered_data *in); + unsigned int flags; + #define XS_FLAG_NOTID (1U << 0) /* Ignore transaction id. */ ++#define XS_FLAG_PRIV (1U << 1) /* Privileged domain only. */ + } const wire_funcs[XS_TYPE_COUNT] = { +- [XS_CONTROL] = { "CONTROL", do_control }, ++ [XS_CONTROL] = ++ { "CONTROL", do_control, XS_FLAG_PRIV }, + [XS_DIRECTORY] = { "DIRECTORY", send_directory }, + [XS_READ] = { "READ", do_read }, + [XS_GET_PERMS] = { "GET_PERMS", do_get_perms }, +@@ -1294,8 +1296,10 @@ static struct { + { "UNWATCH", do_unwatch, XS_FLAG_NOTID }, + [XS_TRANSACTION_START] = { "TRANSACTION_START", do_transaction_start }, + [XS_TRANSACTION_END] = { "TRANSACTION_END", do_transaction_end }, +- [XS_INTRODUCE] = { "INTRODUCE", do_introduce }, +- [XS_RELEASE] = { "RELEASE", do_release }, ++ [XS_INTRODUCE] = ++ { "INTRODUCE", do_introduce, XS_FLAG_PRIV }, ++ [XS_RELEASE] = ++ { "RELEASE", do_release, XS_FLAG_PRIV }, + [XS_GET_DOMAIN_PATH] = { "GET_DOMAIN_PATH", do_get_domain_path }, + [XS_WRITE] = { "WRITE", do_write }, + [XS_MKDIR] = { "MKDIR", do_mkdir }, +@@ -1304,9 +1308,11 @@ static struct { + [XS_WATCH_EVENT] = { "WATCH_EVENT", NULL }, + [XS_ERROR] = { "ERROR", NULL }, + [XS_IS_DOMAIN_INTRODUCED] = +- { "IS_DOMAIN_INTRODUCED", do_is_domain_introduced }, +- [XS_RESUME] = { "RESUME", do_resume }, +- [XS_SET_TARGET] = { "SET_TARGET", do_set_target }, ++ { "IS_DOMAIN_INTRODUCED", do_is_domain_introduced, XS_FLAG_PRIV }, ++ [XS_RESUME] = ++ { "RESUME", do_resume, XS_FLAG_PRIV }, ++ [XS_SET_TARGET] = ++ { "SET_TARGET", do_set_target, XS_FLAG_PRIV }, + [XS_RESET_WATCHES] = { "RESET_WATCHES", do_reset_watches }, + [XS_DIRECTORY_PART] = { "DIRECTORY_PART", send_directory_part }, + }; +@@ -1334,6 +1340,12 @@ static void process_message(struct connection *conn, struct buffered_data *in) + return; + } + ++ if ((wire_funcs[type].flags & XS_FLAG_PRIV) && ++ domain_is_unprivileged(conn)) { ++ send_error(conn, EACCES); ++ return; ++ } ++ + trans = (wire_funcs[type].flags & XS_FLAG_NOTID) + ? NULL : transaction_lookup(conn, in->hdr.msg.tx_id); + if (IS_ERR(trans)) { +diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c +index 1eae703ef680..0e2926e2a3d0 100644 +--- tools/xenstore/xenstored_domain.c.orig ++++ tools/xenstore/xenstored_domain.c +@@ -377,7 +377,7 @@ int do_introduce(struct connection *conn, struct buffered_data *in) + if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec)) + return EINVAL; + +- if (domain_is_unprivileged(conn) || !conn->can_write) ++ if (!conn->can_write) + return EACCES; + + domid = atoi(vec[0]); +@@ -445,7 +445,7 @@ int do_set_target(struct connection *conn, struct buffered_data *in) + if (get_strings(in, vec, ARRAY_SIZE(vec)) < ARRAY_SIZE(vec)) + return EINVAL; + +- if (domain_is_unprivileged(conn) || !conn->can_write) ++ if (!conn->can_write) + return EACCES; + + domid = atoi(vec[0]); +@@ -480,9 +480,6 @@ static struct domain *onearg_domain(struct connection *conn, + if (!domid) + return ERR_PTR(-EINVAL); + +- if (domain_is_unprivileged(conn)) +- return ERR_PTR(-EACCES); +- + return find_connected_domain(domid); + } + +-- +2.17.1 + +From 461c880600175c06e23a63e62d9f1ccab755d708 Mon Sep 17 00:00:00 2001 +From: Juergen Gross +Date: Thu, 11 Jun 2020 16:12:42 +0200 +Subject: [PATCH 06/10] tools/xenstore: rework node removal + +Today a Xenstore node is being removed by deleting it from the parent +first and then deleting itself and all its children. This results in +stale entries remaining in the data base in case e.g. a memory +allocation is failing during processing. This would result in the +rather strange behavior to be able to read a node (as its still in the +data base) while not being visible in the tree view of Xenstore. + +Fix that by deleting the nodes from the leaf side instead of starting +at the root. + +As fire_watches() is now called from _rm() the ctx parameter needs a +const attribute. + +This is part of XSA-115. + +Signed-off-by: Juergen Gross +Reviewed-by: Julien Grall +Reviewed-by: Paul Durrant +--- + tools/xenstore/xenstored_core.c | 99 ++++++++++++++++---------------- + tools/xenstore/xenstored_watch.c | 4 +- + tools/xenstore/xenstored_watch.h | 2 +- + 3 files changed, 54 insertions(+), 51 deletions(-) + +diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c +index 6afd58431111..1cb729a2cd5f 100644 +--- tools/xenstore/xenstored_core.c.orig ++++ tools/xenstore/xenstored_core.c +@@ -1087,74 +1087,76 @@ static int do_mkdir(struct connection *conn, struct buffered_data *in) + return 0; + } + +-static void delete_node(struct connection *conn, struct node *node) +-{ +- unsigned int i; +- char *name; +- +- /* Delete self, then delete children. If we crash, then the worst +- that can happen is the children will continue to take up space, but +- will otherwise be unreachable. */ +- delete_node_single(conn, node); +- +- /* Delete children, too. */ +- for (i = 0; i < node->childlen; i += strlen(node->children+i) + 1) { +- struct node *child; +- +- name = talloc_asprintf(node, "%s/%s", node->name, +- node->children + i); +- child = name ? read_node(conn, node, name) : NULL; +- if (child) { +- delete_node(conn, child); +- } +- else { +- trace("delete_node: Error deleting child '%s/%s'!\n", +- node->name, node->children + i); +- /* Skip it, we've already deleted the parent. */ +- } +- talloc_free(name); +- } +-} +- +- + /* Delete memory using memmove. */ + static void memdel(void *mem, unsigned off, unsigned len, unsigned total) + { + memmove(mem + off, mem + off + len, total - off - len); + } + +- +-static int remove_child_entry(struct connection *conn, struct node *node, +- size_t offset) ++static void remove_child_entry(struct connection *conn, struct node *node, ++ size_t offset) + { + size_t childlen = strlen(node->children + offset); ++ + memdel(node->children, offset, childlen + 1, node->childlen); + node->childlen -= childlen + 1; +- return write_node(conn, node, true); ++ if (write_node(conn, node, true)) ++ corrupt(conn, "Can't update parent node '%s'", node->name); + } + +- +-static int delete_child(struct connection *conn, +- struct node *node, const char *childname) ++static void delete_child(struct connection *conn, ++ struct node *node, const char *childname) + { + unsigned int i; + + for (i = 0; i < node->childlen; i += strlen(node->children+i) + 1) { + if (streq(node->children+i, childname)) { +- return remove_child_entry(conn, node, i); ++ remove_child_entry(conn, node, i); ++ return; + } + } + corrupt(conn, "Can't find child '%s' in %s", childname, node->name); +- return ENOENT; + } + ++static int delete_node(struct connection *conn, struct node *parent, ++ struct node *node) ++{ ++ char *name; ++ ++ /* Delete children. */ ++ while (node->childlen) { ++ struct node *child; ++ ++ name = talloc_asprintf(node, "%s/%s", node->name, ++ node->children); ++ child = name ? read_node(conn, node, name) : NULL; ++ if (child) { ++ if (delete_node(conn, node, child)) ++ return errno; ++ } else { ++ trace("delete_node: Error deleting child '%s/%s'!\n", ++ node->name, node->children); ++ /* Quit deleting. */ ++ errno = ENOMEM; ++ return errno; ++ } ++ talloc_free(name); ++ } ++ ++ delete_node_single(conn, node); ++ delete_child(conn, parent, basename(node->name)); ++ talloc_free(node); ++ ++ return 0; ++} + + static int _rm(struct connection *conn, const void *ctx, struct node *node, + const char *name) + { +- /* Delete from parent first, then if we crash, the worst that can +- happen is the child will continue to take up space, but will +- otherwise be unreachable. */ ++ /* ++ * Deleting node by node, so the result is always consistent even in ++ * case of a failure. ++ */ + struct node *parent; + char *parentname = get_parent(ctx, name); + +@@ -1165,11 +1167,13 @@ static int _rm(struct connection *conn, const void *ctx, struct node *node, + if (!parent) + return (errno == ENOMEM) ? ENOMEM : EINVAL; + +- if (delete_child(conn, parent, basename(name))) +- return EINVAL; +- +- delete_node(conn, node); +- return 0; ++ /* ++ * Fire the watches now, when we can still see the node permissions. ++ * This fine as we are single threaded and the next possible read will ++ * be handled only after the node has been really removed. ++ */ ++ fire_watches(conn, ctx, name, true); ++ return delete_node(conn, parent, node); + } + + +@@ -1207,7 +1211,6 @@ static int do_rm(struct connection *conn, struct buffered_data *in) + if (ret) + return ret; + +- fire_watches(conn, in, name, true); + send_ack(conn, XS_RM); + + return 0; +diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c +index f2f1bed47cc6..f0bbfe7a6dc6 100644 +--- tools/xenstore/xenstored_watch.c.orig ++++ tools/xenstore/xenstored_watch.c +@@ -77,7 +77,7 @@ static bool is_child(const char *child, const char *parent) + * Temporary memory allocations are done with ctx. + */ + static void add_event(struct connection *conn, +- void *ctx, ++ const void *ctx, + struct watch *watch, + const char *name) + { +@@ -121,7 +121,7 @@ static void add_event(struct connection *conn, + * Check whether any watch events are to be sent. + * Temporary memory allocations are done with ctx. + */ +-void fire_watches(struct connection *conn, void *ctx, const char *name, ++void fire_watches(struct connection *conn, const void *ctx, const char *name, + bool recurse) + { + struct connection *i; +diff --git a/tools/xenstore/xenstored_watch.h b/tools/xenstore/xenstored_watch.h +index c72ea6a68542..54d4ea7e0d41 100644 +--- tools/xenstore/xenstored_watch.h.orig ++++ tools/xenstore/xenstored_watch.h +@@ -25,7 +25,7 @@ int do_watch(struct connection *conn, struct buffered_data *in); + int do_unwatch(struct connection *conn, struct buffered_data *in); + + /* Fire all watches: recurse means all the children are affected (ie. rm). */ +-void fire_watches(struct connection *conn, void *tmp, const char *name, ++void fire_watches(struct connection *conn, const void *tmp, const char *name, + bool recurse); + + void conn_delete_all_watches(struct connection *conn); +-- +2.17.1 + +From 6ca2e14b43aecc79effc1a0cd528a4aceef44d42 Mon Sep 17 00:00:00 2001 +From: Juergen Gross +Date: Thu, 11 Jun 2020 16:12:43 +0200 +Subject: [PATCH 07/10] tools/xenstore: fire watches only when removing a + specific node + +Instead of firing all watches for removing a subtree in one go, do so +only when the related node is being removed. + +The watches for the top-most node being removed include all watches +including that node, while watches for nodes below that are only fired +if they are matching exactly. This avoids firing any watch more than +once when removing a subtree. + +This is part of XSA-115. + +Signed-off-by: Juergen Gross +Reviewed-by: Julien Grall +Reviewed-by: Paul Durrant +--- + tools/xenstore/xenstored_core.c | 11 ++++++----- + tools/xenstore/xenstored_watch.c | 13 ++++++++----- + tools/xenstore/xenstored_watch.h | 4 ++-- + 3 files changed, 16 insertions(+), 12 deletions(-) + +diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c +index 1cb729a2cd5f..d7c025616ead 100644 +--- tools/xenstore/xenstored_core.c.orig ++++ tools/xenstore/xenstored_core.c +@@ -1118,8 +1118,8 @@ static void delete_child(struct connection *conn, + corrupt(conn, "Can't find child '%s' in %s", childname, node->name); + } + +-static int delete_node(struct connection *conn, struct node *parent, +- struct node *node) ++static int delete_node(struct connection *conn, const void *ctx, ++ struct node *parent, struct node *node) + { + char *name; + +@@ -1131,7 +1131,7 @@ static int delete_node(struct connection *conn, struct node *parent, + node->children); + child = name ? read_node(conn, node, name) : NULL; + if (child) { +- if (delete_node(conn, node, child)) ++ if (delete_node(conn, ctx, node, child)) + return errno; + } else { + trace("delete_node: Error deleting child '%s/%s'!\n", +@@ -1143,6 +1143,7 @@ static int delete_node(struct connection *conn, struct node *parent, + talloc_free(name); + } + ++ fire_watches(conn, ctx, node->name, true); + delete_node_single(conn, node); + delete_child(conn, parent, basename(node->name)); + talloc_free(node); +@@ -1172,8 +1173,8 @@ static int _rm(struct connection *conn, const void *ctx, struct node *node, + * This fine as we are single threaded and the next possible read will + * be handled only after the node has been really removed. + */ +- fire_watches(conn, ctx, name, true); +- return delete_node(conn, parent, node); ++ fire_watches(conn, ctx, name, false); ++ return delete_node(conn, ctx, parent, node); + } + + +diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c +index f0bbfe7a6dc6..3836675459fa 100644 +--- tools/xenstore/xenstored_watch.c.orig ++++ tools/xenstore/xenstored_watch.c +@@ -122,7 +122,7 @@ static void add_event(struct connection *conn, + * Temporary memory allocations are done with ctx. + */ + void fire_watches(struct connection *conn, const void *ctx, const char *name, +- bool recurse) ++ bool exact) + { + struct connection *i; + struct watch *watch; +@@ -134,10 +134,13 @@ void fire_watches(struct connection *conn, const void *ctx, const char *name, + /* Create an event for each watch. */ + list_for_each_entry(i, &connections, list) { + list_for_each_entry(watch, &i->watches, list) { +- if (is_child(name, watch->node)) +- add_event(i, ctx, watch, name); +- else if (recurse && is_child(watch->node, name)) +- add_event(i, ctx, watch, watch->node); ++ if (exact) { ++ if (streq(name, watch->node)) ++ add_event(i, ctx, watch, name); ++ } else { ++ if (is_child(name, watch->node)) ++ add_event(i, ctx, watch, name); ++ } + } + } + } +diff --git a/tools/xenstore/xenstored_watch.h b/tools/xenstore/xenstored_watch.h +index 54d4ea7e0d41..1b3c80d3dda1 100644 +--- tools/xenstore/xenstored_watch.h.orig ++++ tools/xenstore/xenstored_watch.h +@@ -24,9 +24,9 @@ + int do_watch(struct connection *conn, struct buffered_data *in); + int do_unwatch(struct connection *conn, struct buffered_data *in); + +-/* Fire all watches: recurse means all the children are affected (ie. rm). */ ++/* Fire all watches: !exact means all the children are affected (ie. rm). */ + void fire_watches(struct connection *conn, const void *tmp, const char *name, +- bool recurse); ++ bool exact); + + void conn_delete_all_watches(struct connection *conn); + +-- +2.17.1 + +From 2d4f410899bf59e112c107f371c3d164f8a592f8 Mon Sep 17 00:00:00 2001 +From: Juergen Gross +Date: Thu, 11 Jun 2020 16:12:44 +0200 +Subject: [PATCH 08/10] tools/xenstore: introduce node_perms structure + +There are several places in xenstored using a permission array and the +size of that array. Introduce a new struct node_perms containing both. + +This is part of XSA-115. + +Signed-off-by: Juergen Gross +Acked-by: Julien Grall +Reviewed-by: Paul Durrant +--- + tools/xenstore/xenstored_core.c | 79 +++++++++++++++---------------- + tools/xenstore/xenstored_core.h | 8 +++- + tools/xenstore/xenstored_domain.c | 12 ++--- + 3 files changed, 50 insertions(+), 49 deletions(-) + +diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c +index d7c025616ead..fe9943113b9f 100644 +--- tools/xenstore/xenstored_core.c.orig ++++ tools/xenstore/xenstored_core.c +@@ -401,14 +401,14 @@ static struct node *read_node(struct connection *conn, const void *ctx, + /* Datalen, childlen, number of permissions */ + hdr = (void *)data.dptr; + node->generation = hdr->generation; +- node->num_perms = hdr->num_perms; ++ node->perms.num = hdr->num_perms; + node->datalen = hdr->datalen; + node->childlen = hdr->childlen; + + /* Permissions are struct xs_permissions. */ +- node->perms = hdr->perms; ++ node->perms.p = hdr->perms; + /* Data is binary blob (usually ascii, no nul). */ +- node->data = node->perms + node->num_perms; ++ node->data = node->perms.p + node->perms.num; + /* Children is strings, nul separated. */ + node->children = node->data + node->datalen; + +@@ -425,7 +425,7 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, + struct xs_tdb_record_hdr *hdr; + + data.dsize = sizeof(*hdr) +- + node->num_perms*sizeof(node->perms[0]) ++ + node->perms.num * sizeof(node->perms.p[0]) + + node->datalen + node->childlen; + + if (!no_quota_check && domain_is_unprivileged(conn) && +@@ -437,12 +437,13 @@ int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, + data.dptr = talloc_size(node, data.dsize); + hdr = (void *)data.dptr; + hdr->generation = node->generation; +- hdr->num_perms = node->num_perms; ++ hdr->num_perms = node->perms.num; + hdr->datalen = node->datalen; + hdr->childlen = node->childlen; + +- memcpy(hdr->perms, node->perms, node->num_perms*sizeof(node->perms[0])); +- p = hdr->perms + node->num_perms; ++ memcpy(hdr->perms, node->perms.p, ++ node->perms.num * sizeof(*node->perms.p)); ++ p = hdr->perms + node->perms.num; + memcpy(p, node->data, node->datalen); + p += node->datalen; + memcpy(p, node->children, node->childlen); +@@ -468,8 +469,7 @@ static int write_node(struct connection *conn, struct node *node, + } + + static enum xs_perm_type perm_for_conn(struct connection *conn, +- struct xs_permissions *perms, +- unsigned int num) ++ const struct node_perms *perms) + { + unsigned int i; + enum xs_perm_type mask = XS_PERM_READ|XS_PERM_WRITE|XS_PERM_OWNER; +@@ -478,16 +478,16 @@ static enum xs_perm_type perm_for_conn(struct connection *conn, + mask &= ~XS_PERM_WRITE; + + /* Owners and tools get it all... */ +- if (!domain_is_unprivileged(conn) || perms[0].id == conn->id +- || (conn->target && perms[0].id == conn->target->id)) ++ if (!domain_is_unprivileged(conn) || perms->p[0].id == conn->id ++ || (conn->target && perms->p[0].id == conn->target->id)) + return (XS_PERM_READ|XS_PERM_WRITE|XS_PERM_OWNER) & mask; + +- for (i = 1; i < num; i++) +- if (perms[i].id == conn->id +- || (conn->target && perms[i].id == conn->target->id)) +- return perms[i].perms & mask; ++ for (i = 1; i < perms->num; i++) ++ if (perms->p[i].id == conn->id ++ || (conn->target && perms->p[i].id == conn->target->id)) ++ return perms->p[i].perms & mask; + +- return perms[0].perms & mask; ++ return perms->p[0].perms & mask; + } + + /* +@@ -534,7 +534,7 @@ static int ask_parents(struct connection *conn, const void *ctx, + return 0; + } + +- *perm = perm_for_conn(conn, node->perms, node->num_perms); ++ *perm = perm_for_conn(conn, &node->perms); + return 0; + } + +@@ -580,8 +580,7 @@ struct node *get_node(struct connection *conn, + node = read_node(conn, ctx, name); + /* If we don't have permission, we don't have node. */ + if (node) { +- if ((perm_for_conn(conn, node->perms, node->num_perms) & perm) +- != perm) { ++ if ((perm_for_conn(conn, &node->perms) & perm) != perm) { + errno = EACCES; + node = NULL; + } +@@ -757,16 +756,15 @@ const char *onearg(struct buffered_data *in) + return in->buffer; + } + +-static char *perms_to_strings(const void *ctx, +- struct xs_permissions *perms, unsigned int num, ++static char *perms_to_strings(const void *ctx, const struct node_perms *perms, + unsigned int *len) + { + unsigned int i; + char *strings = NULL; + char buffer[MAX_STRLEN(unsigned int) + 1]; + +- for (*len = 0, i = 0; i < num; i++) { +- if (!xs_perm_to_string(&perms[i], buffer, sizeof(buffer))) ++ for (*len = 0, i = 0; i < perms->num; i++) { ++ if (!xs_perm_to_string(&perms->p[i], buffer, sizeof(buffer))) + return NULL; + + strings = talloc_realloc(ctx, strings, char, +@@ -945,13 +943,13 @@ static struct node *construct_node(struct connection *conn, const void *ctx, + goto nomem; + + /* Inherit permissions, except unprivileged domains own what they create */ +- node->num_perms = parent->num_perms; +- node->perms = talloc_memdup(node, parent->perms, +- node->num_perms * sizeof(node->perms[0])); +- if (!node->perms) ++ node->perms.num = parent->perms.num; ++ node->perms.p = talloc_memdup(node, parent->perms.p, ++ node->perms.num * sizeof(*node->perms.p)); ++ if (!node->perms.p) + goto nomem; + if (domain_is_unprivileged(conn)) +- node->perms[0].id = conn->id; ++ node->perms.p[0].id = conn->id; + + /* No children, no data */ + node->children = node->data = NULL; +@@ -1228,7 +1226,7 @@ static int do_get_perms(struct connection *conn, struct buffered_data *in) + if (!node) + return errno; + +- strings = perms_to_strings(node, node->perms, node->num_perms, &len); ++ strings = perms_to_strings(node, &node->perms, &len); + if (!strings) + return errno; + +@@ -1239,13 +1237,12 @@ static int do_get_perms(struct connection *conn, struct buffered_data *in) + + static int do_set_perms(struct connection *conn, struct buffered_data *in) + { +- unsigned int num; +- struct xs_permissions *perms; ++ struct node_perms perms; + char *name, *permstr; + struct node *node; + +- num = xs_count_strings(in->buffer, in->used); +- if (num < 2) ++ perms.num = xs_count_strings(in->buffer, in->used); ++ if (perms.num < 2) + return EINVAL; + + /* First arg is node name. */ +@@ -1256,21 +1253,21 @@ static int do_set_perms(struct connection *conn, struct buffered_data *in) + return errno; + + permstr = in->buffer + strlen(in->buffer) + 1; +- num--; ++ perms.num--; + +- perms = talloc_array(node, struct xs_permissions, num); +- if (!perms) ++ perms.p = talloc_array(node, struct xs_permissions, perms.num); ++ if (!perms.p) + return ENOMEM; +- if (!xs_strings_to_perms(perms, num, permstr)) ++ if (!xs_strings_to_perms(perms.p, perms.num, permstr)) + return errno; + + /* Unprivileged domains may not change the owner. */ +- if (domain_is_unprivileged(conn) && perms[0].id != node->perms[0].id) ++ if (domain_is_unprivileged(conn) && ++ perms.p[0].id != node->perms.p[0].id) + return EPERM; + + domain_entry_dec(conn, node); + node->perms = perms; +- node->num_perms = num; + domain_entry_inc(conn, node); + + if (write_node(conn, node, false)) +@@ -1545,8 +1542,8 @@ static void manual_node(const char *name, const char *child) + barf_perror("Could not allocate initial node %s", name); + + node->name = name; +- node->perms = &perms; +- node->num_perms = 1; ++ node->perms.p = &perms; ++ node->perms.num = 1; + node->children = (char *)child; + if (child) + node->childlen = strlen(child) + 1; +diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h +index 3cb1c235a101..193d93142636 100644 +--- tools/xenstore/xenstored_core.h.orig ++++ tools/xenstore/xenstored_core.h +@@ -109,6 +109,11 @@ struct connection + }; + extern struct list_head connections; + ++struct node_perms { ++ unsigned int num; ++ struct xs_permissions *p; ++}; ++ + struct node { + const char *name; + +@@ -120,8 +125,7 @@ struct node { + #define NO_GENERATION ~((uint64_t)0) + + /* Permissions. */ +- unsigned int num_perms; +- struct xs_permissions *perms; ++ struct node_perms perms; + + /* Contents. */ + unsigned int datalen; +diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c +index 0e2926e2a3d0..dc51cdfa9aa7 100644 +--- tools/xenstore/xenstored_domain.c.orig ++++ tools/xenstore/xenstored_domain.c +@@ -657,12 +657,12 @@ void domain_entry_inc(struct connection *conn, struct node *node) + if (!conn) + return; + +- if (node->perms && node->perms[0].id != conn->id) { ++ if (node->perms.p && node->perms.p[0].id != conn->id) { + if (conn->transaction) { + transaction_entry_inc(conn->transaction, +- node->perms[0].id); ++ node->perms.p[0].id); + } else { +- d = find_domain_by_domid(node->perms[0].id); ++ d = find_domain_by_domid(node->perms.p[0].id); + if (d) + d->nbentry++; + } +@@ -683,12 +683,12 @@ void domain_entry_dec(struct connection *conn, struct node *node) + if (!conn) + return; + +- if (node->perms && node->perms[0].id != conn->id) { ++ if (node->perms.p && node->perms.p[0].id != conn->id) { + if (conn->transaction) { + transaction_entry_dec(conn->transaction, +- node->perms[0].id); ++ node->perms.p[0].id); + } else { +- d = find_domain_by_domid(node->perms[0].id); ++ d = find_domain_by_domid(node->perms.p[0].id); + if (d && d->nbentry) + d->nbentry--; + } +-- +2.17.1 + +From cddf74031b3c8a108e8fd7db0bf56e9c2809d3e2 Mon Sep 17 00:00:00 2001 +From: Juergen Gross +Date: Thu, 11 Jun 2020 16:12:45 +0200 +Subject: [PATCH 09/10] tools/xenstore: allow special watches for privileged + callers only + +The special watches "@introduceDomain" and "@releaseDomain" should be +allowed for privileged callers only, as they allow to gain information +about presence of other guests on the host. So send watch events for +those watches via privileged connections only. + +In order to allow for disaggregated setups where e.g. driver domains +need to make use of those special watches add support for calling +"set permissions" for those special nodes, too. + +This is part of XSA-115. + +Signed-off-by: Juergen Gross +Reviewed-by: Julien Grall +Reviewed-by: Paul Durrant +--- + docs/misc/xenstore.txt | 5 +++ + tools/xenstore/xenstored_core.c | 27 ++++++++------ + tools/xenstore/xenstored_core.h | 2 ++ + tools/xenstore/xenstored_domain.c | 60 +++++++++++++++++++++++++++++++ + tools/xenstore/xenstored_domain.h | 5 +++ + tools/xenstore/xenstored_watch.c | 4 +++ + 6 files changed, 93 insertions(+), 10 deletions(-) + +diff --git a/docs/misc/xenstore.txt b/docs/misc/xenstore.txt +index 6f8569d5760f..32969eb3fecd 100644 +--- docs/misc/xenstore.txt.orig ++++ docs/misc/xenstore.txt +@@ -170,6 +170,9 @@ SET_PERMS ||+? + n no access + See http://wiki.xen.org/wiki/XenBus section + `Permissions' for details of the permissions system. ++ It is possible to set permissions for the special watch paths ++ "@introduceDomain" and "@releaseDomain" to enable receiving those ++ watches in unprivileged domains. + + ---------- Watches ---------- + +@@ -194,6 +197,8 @@ WATCH ||? + @releaseDomain occurs on any domain crash or + shutdown, and also on RELEASE + and domain destruction ++ events are sent to privileged callers or explicitly ++ via SET_PERMS enabled domains only. + + When a watch is first set up it is triggered once straight + away, with equal to . Watches may be triggered +diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c +index fe9943113b9f..720bec269dd3 100644 +--- tools/xenstore/xenstored_core.c.orig ++++ tools/xenstore/xenstored_core.c +@@ -468,8 +468,8 @@ static int write_node(struct connection *conn, struct node *node, + return write_node_raw(conn, &key, node, no_quota_check); + } + +-static enum xs_perm_type perm_for_conn(struct connection *conn, +- const struct node_perms *perms) ++enum xs_perm_type perm_for_conn(struct connection *conn, ++ const struct node_perms *perms) + { + unsigned int i; + enum xs_perm_type mask = XS_PERM_READ|XS_PERM_WRITE|XS_PERM_OWNER; +@@ -1245,22 +1245,29 @@ static int do_set_perms(struct connection *conn, struct buffered_data *in) + if (perms.num < 2) + return EINVAL; + +- /* First arg is node name. */ +- /* We must own node to do this (tools can do this too). */ +- node = get_node_canonicalized(conn, in, in->buffer, &name, +- XS_PERM_WRITE | XS_PERM_OWNER); +- if (!node) +- return errno; +- + permstr = in->buffer + strlen(in->buffer) + 1; + perms.num--; + +- perms.p = talloc_array(node, struct xs_permissions, perms.num); ++ perms.p = talloc_array(in, struct xs_permissions, perms.num); + if (!perms.p) + return ENOMEM; + if (!xs_strings_to_perms(perms.p, perms.num, permstr)) + return errno; + ++ /* First arg is node name. */ ++ if (strstarts(in->buffer, "@")) { ++ if (set_perms_special(conn, in->buffer, &perms)) ++ return errno; ++ send_ack(conn, XS_SET_PERMS); ++ return 0; ++ } ++ ++ /* We must own node to do this (tools can do this too). */ ++ node = get_node_canonicalized(conn, in, in->buffer, &name, ++ XS_PERM_WRITE | XS_PERM_OWNER); ++ if (!node) ++ return errno; ++ + /* Unprivileged domains may not change the owner. */ + if (domain_is_unprivileged(conn) && + perms.p[0].id != node->perms.p[0].id) +diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h +index 193d93142636..f3da6bbc943d 100644 +--- tools/xenstore/xenstored_core.h.orig ++++ tools/xenstore/xenstored_core.h +@@ -165,6 +165,8 @@ struct node *get_node(struct connection *conn, + struct connection *new_connection(connwritefn_t *write, connreadfn_t *read); + void check_store(void); + void corrupt(struct connection *conn, const char *fmt, ...); ++enum xs_perm_type perm_for_conn(struct connection *conn, ++ const struct node_perms *perms); + + /* Is this a valid node name? */ + bool is_valid_nodename(const char *node); +diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c +index dc51cdfa9aa7..7afabe0ae084 100644 +--- tools/xenstore/xenstored_domain.c.orig ++++ tools/xenstore/xenstored_domain.c +@@ -41,6 +41,9 @@ static evtchn_port_t virq_port; + + xenevtchn_handle *xce_handle = NULL; + ++static struct node_perms dom_release_perms; ++static struct node_perms dom_introduce_perms; ++ + struct domain + { + struct list_head list; +@@ -589,6 +592,59 @@ void restore_existing_connections(void) + { + } + ++static int set_dom_perms_default(struct node_perms *perms) ++{ ++ perms->num = 1; ++ perms->p = talloc_array(NULL, struct xs_permissions, perms->num); ++ if (!perms->p) ++ return -1; ++ perms->p->id = 0; ++ perms->p->perms = XS_PERM_NONE; ++ ++ return 0; ++} ++ ++static struct node_perms *get_perms_special(const char *name) ++{ ++ if (!strcmp(name, "@releaseDomain")) ++ return &dom_release_perms; ++ if (!strcmp(name, "@introduceDomain")) ++ return &dom_introduce_perms; ++ return NULL; ++} ++ ++int set_perms_special(struct connection *conn, const char *name, ++ struct node_perms *perms) ++{ ++ struct node_perms *p; ++ ++ p = get_perms_special(name); ++ if (!p) ++ return EINVAL; ++ ++ if ((perm_for_conn(conn, p) & (XS_PERM_WRITE | XS_PERM_OWNER)) != ++ (XS_PERM_WRITE | XS_PERM_OWNER)) ++ return EACCES; ++ ++ p->num = perms->num; ++ talloc_free(p->p); ++ p->p = perms->p; ++ talloc_steal(NULL, perms->p); ++ ++ return 0; ++} ++ ++bool check_perms_special(const char *name, struct connection *conn) ++{ ++ struct node_perms *p; ++ ++ p = get_perms_special(name); ++ if (!p) ++ return false; ++ ++ return perm_for_conn(conn, p) & XS_PERM_READ; ++} ++ + static int dom0_init(void) + { + evtchn_port_t port; +@@ -610,6 +666,10 @@ static int dom0_init(void) + + xenevtchn_notify(xce_handle, dom0->port); + ++ if (set_dom_perms_default(&dom_release_perms) || ++ set_dom_perms_default(&dom_introduce_perms)) ++ return -1; ++ + return 0; + } + +diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h +index 56ae01597475..259183962a9c 100644 +--- tools/xenstore/xenstored_domain.h.orig ++++ tools/xenstore/xenstored_domain.h +@@ -65,6 +65,11 @@ void domain_watch_inc(struct connection *conn); + void domain_watch_dec(struct connection *conn); + int domain_watch(struct connection *conn); + ++/* Special node permission handling. */ ++int set_perms_special(struct connection *conn, const char *name, ++ struct node_perms *perms); ++bool check_perms_special(const char *name, struct connection *conn); ++ + /* Write rate limiting */ + + #define WRL_FACTOR 1000 /* for fixed-point arithmetic */ +diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c +index 3836675459fa..f4e289362eb6 100644 +--- tools/xenstore/xenstored_watch.c.orig ++++ tools/xenstore/xenstored_watch.c +@@ -133,6 +133,10 @@ void fire_watches(struct connection *conn, const void *ctx, const char *name, + + /* Create an event for each watch. */ + list_for_each_entry(i, &connections, list) { ++ /* introduce/release domain watches */ ++ if (check_special_event(name) && !check_perms_special(name, i)) ++ continue; ++ + list_for_each_entry(watch, &i->watches, list) { + if (exact) { + if (streq(name, watch->node)) +-- +2.17.1 + +From e57b7687b43b033fe45e755e285efbe67bc71921 Mon Sep 17 00:00:00 2001 +From: Juergen Gross +Date: Thu, 11 Jun 2020 16:12:46 +0200 +Subject: [PATCH 10/10] tools/xenstore: avoid watch events for nodes without + access + +Today watch events are sent regardless of the access rights of the +node the event is sent for. This enables any guest to e.g. setup a +watch for "/" in order to have a detailed record of all Xenstore +modifications. + +Modify that by sending only watch events for nodes that the watcher +has a chance to see otherwise (either via direct reads or by querying +the children of a node). This includes cases where the visibility of +a node for a watcher is changing (permissions being removed). + +This is part of XSA-115. + +Signed-off-by: Juergen Gross +[julieng: Handle rebase conflict] +Reviewed-by: Julien Grall +Reviewed-by: Paul Durrant +--- + tools/xenstore/xenstored_core.c | 28 +++++----- + tools/xenstore/xenstored_core.h | 15 ++++-- + tools/xenstore/xenstored_domain.c | 6 +-- + tools/xenstore/xenstored_transaction.c | 21 +++++++- + tools/xenstore/xenstored_watch.c | 75 +++++++++++++++++++------- + tools/xenstore/xenstored_watch.h | 2 +- + 6 files changed, 104 insertions(+), 43 deletions(-) + +diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c +index 720bec269dd3..1c2845454560 100644 +--- tools/xenstore/xenstored_core.c.orig ++++ tools/xenstore/xenstored_core.c +@@ -358,8 +358,8 @@ static void initialize_fds(int sock, int *p_sock_pollfd_idx, + * If it fails, returns NULL and sets errno. + * Temporary memory allocations will be done with ctx. + */ +-static struct node *read_node(struct connection *conn, const void *ctx, +- const char *name) ++struct node *read_node(struct connection *conn, const void *ctx, ++ const char *name) + { + TDB_DATA key, data; + struct xs_tdb_record_hdr *hdr; +@@ -494,7 +494,7 @@ enum xs_perm_type perm_for_conn(struct connection *conn, + * Get name of node parent. + * Temporary memory allocations are done with ctx. + */ +-static char *get_parent(const void *ctx, const char *node) ++char *get_parent(const void *ctx, const char *node) + { + char *parent; + char *slash = strrchr(node + 1, '/'); +@@ -566,10 +566,10 @@ static int errno_from_parents(struct connection *conn, const void *ctx, + * If it fails, returns NULL and sets errno. + * Temporary memory allocations are done with ctx. + */ +-struct node *get_node(struct connection *conn, +- const void *ctx, +- const char *name, +- enum xs_perm_type perm) ++static struct node *get_node(struct connection *conn, ++ const void *ctx, ++ const char *name, ++ enum xs_perm_type perm) + { + struct node *node; + +@@ -1056,7 +1056,7 @@ static int do_write(struct connection *conn, struct buffered_data *in) + return errno; + } + +- fire_watches(conn, in, name, false); ++ fire_watches(conn, in, name, node, false, NULL); + send_ack(conn, XS_WRITE); + + return 0; +@@ -1078,7 +1078,7 @@ static int do_mkdir(struct connection *conn, struct buffered_data *in) + node = create_node(conn, in, name, NULL, 0); + if (!node) + return errno; +- fire_watches(conn, in, name, false); ++ fire_watches(conn, in, name, node, false, NULL); + } + send_ack(conn, XS_MKDIR); + +@@ -1141,7 +1141,7 @@ static int delete_node(struct connection *conn, const void *ctx, + talloc_free(name); + } + +- fire_watches(conn, ctx, node->name, true); ++ fire_watches(conn, ctx, node->name, node, true, NULL); + delete_node_single(conn, node); + delete_child(conn, parent, basename(node->name)); + talloc_free(node); +@@ -1165,13 +1165,14 @@ static int _rm(struct connection *conn, const void *ctx, struct node *node, + parent = read_node(conn, ctx, parentname); + if (!parent) + return (errno == ENOMEM) ? ENOMEM : EINVAL; ++ node->parent = parent; + + /* + * Fire the watches now, when we can still see the node permissions. + * This fine as we are single threaded and the next possible read will + * be handled only after the node has been really removed. + */ +- fire_watches(conn, ctx, name, false); ++ fire_watches(conn, ctx, name, node, false, NULL); + return delete_node(conn, ctx, parent, node); + } + +@@ -1237,7 +1238,7 @@ static int do_get_perms(struct connection *conn, struct buffered_data *in) + + static int do_set_perms(struct connection *conn, struct buffered_data *in) + { +- struct node_perms perms; ++ struct node_perms perms, old_perms; + char *name, *permstr; + struct node *node; + +@@ -1273,6 +1274,7 @@ static int do_set_perms(struct connection *conn, struct buffered_data *in) + perms.p[0].id != node->perms.p[0].id) + return EPERM; + ++ old_perms = node->perms; + domain_entry_dec(conn, node); + node->perms = perms; + domain_entry_inc(conn, node); +@@ -1280,7 +1282,7 @@ static int do_set_perms(struct connection *conn, struct buffered_data *in) + if (write_node(conn, node, false)) + return errno; + +- fire_watches(conn, in, name, false); ++ fire_watches(conn, in, name, node, false, &old_perms); + send_ack(conn, XS_SET_PERMS); + + return 0; +diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h +index f3da6bbc943d..e050b27cbdde 100644 +--- tools/xenstore/xenstored_core.h.orig ++++ tools/xenstore/xenstored_core.h +@@ -152,15 +152,17 @@ void send_ack(struct connection *conn, enum xsd_sockmsg_type type); + /* Canonicalize this path if possible. */ + char *canonicalize(struct connection *conn, const void *ctx, const char *node); + ++/* Get access permissions. */ ++enum xs_perm_type perm_for_conn(struct connection *conn, ++ const struct node_perms *perms); ++ + /* Write a node to the tdb data base. */ + int write_node_raw(struct connection *conn, TDB_DATA *key, struct node *node, + bool no_quota_check); + +-/* Get this node, checking we have permissions. */ +-struct node *get_node(struct connection *conn, +- const void *ctx, +- const char *name, +- enum xs_perm_type perm); ++/* Get a node from the tdb data base. */ ++struct node *read_node(struct connection *conn, const void *ctx, ++ const char *name); + + struct connection *new_connection(connwritefn_t *write, connreadfn_t *read); + void check_store(void); +@@ -171,6 +173,9 @@ enum xs_perm_type perm_for_conn(struct connection *conn, + /* Is this a valid node name? */ + bool is_valid_nodename(const char *node); + ++/* Get name of parent node. */ ++char *get_parent(const void *ctx, const char *node); ++ + /* Tracing infrastructure. */ + void trace_create(const void *data, const char *type); + void trace_destroy(const void *data, const char *type); +diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c +index 7afabe0ae084..711a11b18ad6 100644 +--- tools/xenstore/xenstored_domain.c.orig ++++ tools/xenstore/xenstored_domain.c +@@ -206,7 +206,7 @@ static int destroy_domain(void *_domain) + unmap_interface(domain->interface); + } + +- fire_watches(NULL, domain, "@releaseDomain", false); ++ fire_watches(NULL, domain, "@releaseDomain", NULL, false, NULL); + + wrl_domain_destroy(domain); + +@@ -244,7 +244,7 @@ static void domain_cleanup(void) + } + + if (notify) +- fire_watches(NULL, NULL, "@releaseDomain", false); ++ fire_watches(NULL, NULL, "@releaseDomain", NULL, false, NULL); + } + + /* We scan all domains rather than use the information given here. */ +@@ -410,7 +410,7 @@ int do_introduce(struct connection *conn, struct buffered_data *in) + /* Now domain belongs to its connection. */ + talloc_steal(domain->conn, domain); + +- fire_watches(NULL, in, "@introduceDomain", false); ++ fire_watches(NULL, in, "@introduceDomain", NULL, false, NULL); + } else if ((domain->mfn == mfn) && (domain->conn != conn)) { + /* Use XS_INTRODUCE for recreating the xenbus event-channel. */ + if (domain->port) +diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c +index e87897573469..a7d8c5d475ec 100644 +--- tools/xenstore/xenstored_transaction.c.orig ++++ tools/xenstore/xenstored_transaction.c +@@ -114,6 +114,9 @@ struct accessed_node + /* Generation count (or NO_GENERATION) for conflict checking. */ + uint64_t generation; + ++ /* Original node permissions. */ ++ struct node_perms perms; ++ + /* Generation count checking required? */ + bool check_gen; + +@@ -260,6 +263,15 @@ int access_node(struct connection *conn, struct node *node, + i->node = talloc_strdup(i, node->name); + if (!i->node) + goto nomem; ++ if (node->generation != NO_GENERATION && node->perms.num) { ++ i->perms.p = talloc_array(i, struct xs_permissions, ++ node->perms.num); ++ if (!i->perms.p) ++ goto nomem; ++ i->perms.num = node->perms.num; ++ memcpy(i->perms.p, node->perms.p, ++ i->perms.num * sizeof(*i->perms.p)); ++ } + + introduce = true; + i->ta_node = false; +@@ -368,9 +380,14 @@ static int finalize_transaction(struct connection *conn, + talloc_free(data.dptr); + if (ret) + goto err; +- } else if (tdb_delete(tdb_ctx, key)) ++ fire_watches(conn, trans, i->node, NULL, false, ++ i->perms.p ? &i->perms : NULL); ++ } else { ++ fire_watches(conn, trans, i->node, NULL, false, ++ i->perms.p ? &i->perms : NULL); ++ if (tdb_delete(tdb_ctx, key)) + goto err; +- fire_watches(conn, trans, i->node, false); ++ } + } + + if (i->ta_node && tdb_delete(tdb_ctx, ta_key)) +diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c +index f4e289362eb6..71c108ea99f1 100644 +--- tools/xenstore/xenstored_watch.c.orig ++++ tools/xenstore/xenstored_watch.c +@@ -85,22 +85,6 @@ static void add_event(struct connection *conn, + unsigned int len; + char *data; + +- if (!check_special_event(name)) { +- /* Can this conn load node, or see that it doesn't exist? */ +- struct node *node = get_node(conn, ctx, name, XS_PERM_READ); +- /* +- * XXX We allow EACCES here because otherwise a non-dom0 +- * backend driver cannot watch for disappearance of a frontend +- * xenstore directory. When the directory disappears, we +- * revert to permissions of the parent directory for that path, +- * which will typically disallow access for the backend. +- * But this breaks device-channel teardown! +- * Really we should fix this better... +- */ +- if (!node && errno != ENOENT && errno != EACCES) +- return; +- } +- + if (watch->relative_path) { + name += strlen(watch->relative_path); + if (*name == '/') /* Could be "" */ +@@ -117,12 +101,60 @@ static void add_event(struct connection *conn, + talloc_free(data); + } + ++/* ++ * Check permissions of a specific watch to fire: ++ * Either the node itself or its parent have to be readable by the connection ++ * the watch has been setup for. In case a watch event is created due to ++ * changed permissions we need to take the old permissions into account, too. ++ */ ++static bool watch_permitted(struct connection *conn, const void *ctx, ++ const char *name, struct node *node, ++ struct node_perms *perms) ++{ ++ enum xs_perm_type perm; ++ struct node *parent; ++ char *parent_name; ++ ++ if (perms) { ++ perm = perm_for_conn(conn, perms); ++ if (perm & XS_PERM_READ) ++ return true; ++ } ++ ++ if (!node) { ++ node = read_node(conn, ctx, name); ++ if (!node) ++ return false; ++ } ++ ++ perm = perm_for_conn(conn, &node->perms); ++ if (perm & XS_PERM_READ) ++ return true; ++ ++ parent = node->parent; ++ if (!parent) { ++ parent_name = get_parent(ctx, node->name); ++ if (!parent_name) ++ return false; ++ parent = read_node(conn, ctx, parent_name); ++ if (!parent) ++ return false; ++ } ++ ++ perm = perm_for_conn(conn, &parent->perms); ++ ++ return perm & XS_PERM_READ; ++} ++ + /* + * Check whether any watch events are to be sent. + * Temporary memory allocations are done with ctx. ++ * We need to take the (potential) old permissions of the node into account ++ * as a watcher losing permissions to access a node should receive the ++ * watch event, too. + */ + void fire_watches(struct connection *conn, const void *ctx, const char *name, +- bool exact) ++ struct node *node, bool exact, struct node_perms *perms) + { + struct connection *i; + struct watch *watch; +@@ -134,8 +166,13 @@ void fire_watches(struct connection *conn, const void *ctx, const char *name, + /* Create an event for each watch. */ + list_for_each_entry(i, &connections, list) { + /* introduce/release domain watches */ +- if (check_special_event(name) && !check_perms_special(name, i)) +- continue; ++ if (check_special_event(name)) { ++ if (!check_perms_special(name, i)) ++ continue; ++ } else { ++ if (!watch_permitted(i, ctx, name, node, perms)) ++ continue; ++ } + + list_for_each_entry(watch, &i->watches, list) { + if (exact) { +diff --git a/tools/xenstore/xenstored_watch.h b/tools/xenstore/xenstored_watch.h +index 1b3c80d3dda1..03094374f379 100644 +--- tools/xenstore/xenstored_watch.h.orig ++++ tools/xenstore/xenstored_watch.h +@@ -26,7 +26,7 @@ int do_unwatch(struct connection *conn, struct buffered_data *in); + + /* Fire all watches: !exact means all the children are affected (ie. rm). */ + void fire_watches(struct connection *conn, const void *tmp, const char *name, +- bool exact); ++ struct node *node, bool exact, struct node_perms *perms); + + void conn_delete_all_watches(struct connection *conn); + +-- +2.17.1 + Index: pkgsrc/sysutils/xentools411/patches/patch-XSA115-o diff -u /dev/null pkgsrc/sysutils/xentools411/patches/patch-XSA115-o:1.1 --- /dev/null Thu Dec 17 16:48:12 2020 +++ pkgsrc/sysutils/xentools411/patches/patch-XSA115-o Thu Dec 17 16:48:12 2020 @@ -0,0 +1,694 @@ +$NetBSD: patch-XSA115-o,v 1.1 2020/12/17 16:48:12 bouyer Exp $ + +From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= +Subject: tools/ocaml/xenstored: ignore transaction id for [un]watch +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Instead of ignoring the transaction id for XS_WATCH and XS_UNWATCH +commands as it is documented in docs/misc/xenstore.txt, it is tested +for validity today. + +Really ignore the transaction id for XS_WATCH and XS_UNWATCH. + +This is part of XSA-115. + +Signed-off-by: Edwin Török +Acked-by: Christian Lindig +Reviewed-by: Andrew Cooper + +diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml +index 74c69f869c..0a0e43d1f0 100644 +--- tools/ocaml/xenstored/process.ml.orig ++++ tools/ocaml/xenstored/process.ml +@@ -492,12 +492,19 @@ let retain_op_in_history ty = + | Xenbus.Xb.Op.Reset_watches + | Xenbus.Xb.Op.Invalid -> false + ++let maybe_ignore_transaction = function ++ | Xenbus.Xb.Op.Watch | Xenbus.Xb.Op.Unwatch -> fun tid -> ++ if tid <> Transaction.none then ++ debug "Ignoring transaction ID %d for watch/unwatch" tid; ++ Transaction.none ++ | _ -> fun x -> x ++ + (** + * Nothrow guarantee. + *) + let process_packet ~store ~cons ~doms ~con ~req = + let ty = req.Packet.ty in +- let tid = req.Packet.tid in ++ let tid = maybe_ignore_transaction ty req.Packet.tid in + let rid = req.Packet.rid in + try + let fct = function_of_type ty in +From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= +Subject: tools/ocaml/xenstored: check privilege for XS_IS_DOMAIN_INTRODUCED +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +The Xenstore command XS_IS_DOMAIN_INTRODUCED should be possible for privileged +domains only (the only user in the tree is the xenpaging daemon). + +This is part of XSA-115. + +Signed-off-by: Edwin Török +Acked-by: Christian Lindig +Reviewed-by: Andrew Cooper + +diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml +index 0a0e43d1f0..f374abe998 100644 +--- tools/ocaml/xenstored/process.ml.orig ++++ tools/ocaml/xenstored/process.ml +@@ -166,7 +166,9 @@ let do_setperms con t domains cons data = + let do_error con t domains cons data = + raise Define.Unknown_operation + +-let do_isintroduced con t domains cons data = ++let do_isintroduced con _t domains _cons data = ++ if not (Connection.is_dom0 con) ++ then raise Define.Permission_denied; + let domid = + match (split None '\000' data) with + | domid :: _ -> int_of_string domid +From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= +Subject: tools/ocaml/xenstored: unify watch firing +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +This will make it easier insert additional checks in a follow-up patch. +All watches are now fired from a single function. + +This is part of XSA-115. + +Signed-off-by: Edwin Török +Acked-by: Christian Lindig +Reviewed-by: Andrew Cooper + +diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml +index be9c62f27f..d7432c6597 100644 +--- tools/ocaml/xenstored/connection.ml.orig ++++ tools/ocaml/xenstored/connection.ml +@@ -210,8 +210,7 @@ let fire_watch watch path = + end else + path + in +- let data = Utils.join_by_null [ new_path; watch.token; "" ] in +- send_reply watch.con Transaction.none 0 Xenbus.Xb.Op.Watchevent data ++ fire_single_watch { watch with path = new_path } + + (* Search for a valid unused transaction id. *) + let rec valid_transaction_id con proposed_id = +From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= +Subject: tools/ocaml/xenstored: introduce permissions for special watches +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +The special watches "@introduceDomain" and "@releaseDomain" should be +allowed for privileged callers only, as they allow to gain information +about presence of other guests on the host. So send watch events for +those watches via privileged connections only. + +Start to address this by treating the special watches as regular nodes +in the tree, which gives them normal semantics for permissions. A later +change will restrict the handling, so that they can't be listed, etc. + +This is part of XSA-115. + +Signed-off-by: Edwin Török +Acked-by: Christian Lindig +Reviewed-by: Andrew Cooper + +diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml +index f374abe998..c3c8ea2f4b 100644 +--- tools/ocaml/xenstored/process.ml.orig ++++ tools/ocaml/xenstored/process.ml +@@ -414,7 +414,7 @@ let do_introduce con t domains cons data = + else try + let ndom = Domains.create domains domid mfn port in + Connections.add_domain cons ndom; +- Connections.fire_spec_watches cons "@introduceDomain"; ++ Connections.fire_spec_watches cons Store.Path.introduce_domain; + ndom + with _ -> raise Invalid_Cmd_Args + in +@@ -433,7 +433,7 @@ let do_release con t domains cons data = + Domains.del domains domid; + Connections.del_domain cons domid; + if fire_spec_watches +- then Connections.fire_spec_watches cons "@releaseDomain" ++ then Connections.fire_spec_watches cons Store.Path.release_domain + else raise Invalid_Cmd_Args + + let do_resume con t domains cons data = +diff --git a/tools/ocaml/xenstored/store.ml b/tools/ocaml/xenstored/store.ml +index 6375a1c889..98d368d52f 100644 +--- tools/ocaml/xenstored/store.ml.orig ++++ tools/ocaml/xenstored/store.ml +@@ -214,6 +214,11 @@ let rec lookup node path fct = + + let apply rnode path fct = + lookup rnode path fct ++ ++let introduce_domain = "@introduceDomain" ++let release_domain = "@releaseDomain" ++let specials = List.map of_string [ introduce_domain; release_domain ] ++ + end + + (* The Store.t type *) +diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml +index b252db799b..e8c9fe4e94 100644 +--- tools/ocaml/xenstored/utils.ml.orig ++++ tools/ocaml/xenstored/utils.ml +@@ -88,19 +88,17 @@ let read_file_single_integer filename = + Unix.close fd; + int_of_string (Bytes.sub_string buf 0 sz) + +-let path_complete path connection_path = +- if String.get path 0 <> '/' then +- connection_path ^ path +- else +- path +- ++(* @path may be guest data and needs its length validating. @connection_path ++ * is generated locally in xenstored and always of the form "/local/domain/$N/" *) + let path_validate path connection_path = +- if String.length path = 0 || String.length path > 1024 then +- raise Define.Invalid_path +- else +- let cpath = path_complete path connection_path in +- if String.get cpath 0 <> '/' then +- raise Define.Invalid_path +- else +- cpath ++ let len = String.length path in ++ ++ if len = 0 || len > 1024 then raise Define.Invalid_path; ++ ++ let abs_path = ++ match String.get path 0 with ++ | '/' | '@' -> path ++ | _ -> connection_path ^ path ++ in + ++ abs_path +diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml +index 49fc18bf19..32c3b1c0f1 100644 +--- tools/ocaml/xenstored/xenstored.ml.orig ++++ tools/ocaml/xenstored/xenstored.ml +@@ -287,6 +287,8 @@ let _ = + let quit = ref false in + + Logging.init_xenstored_log(); ++ List.iter (fun path -> ++ Store.write store Perms.Connection.full_rights path "") Store.Path.specials; + + let filename = Paths.xen_run_stored ^ "/db" in + if cf.restart && Sys.file_exists filename then ( +@@ -339,7 +341,7 @@ let _ = + let (notify, deaddom) = Domains.cleanup domains in + List.iter (Connections.del_domain cons) deaddom; + if deaddom <> [] || notify then +- Connections.fire_spec_watches cons "@releaseDomain" ++ Connections.fire_spec_watches cons Store.Path.release_domain + ) + else + let c = Connections.find_domain_by_port cons port in +From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= +Subject: tools/ocaml/xenstored: avoid watch events for nodes without access +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Today watch events are sent regardless of the access rights of the +node the event is sent for. This enables any guest to e.g. setup a +watch for "/" in order to have a detailed record of all Xenstore +modifications. + +Modify that by sending only watch events for nodes that the watcher +has a chance to see otherwise (either via direct reads or by querying +the children of a node). This includes cases where the visibility of +a node for a watcher is changing (permissions being removed). + +Permissions for nodes are looked up either in the old (pre +transaction/command) or current trees (post transaction). If +permissions are changed multiple times in a transaction only the final +version is checked, because considering a transaction atomic the +individual permission changes would not be noticable to an outside +observer. + +Two trees are only needed for set_perms: here we can either notice the +node disappearing (if we loose permission), appearing +(if we gain permission), or changing (if we preserve permission). + +RM needs to only look at the old tree: in the new tree the node would be +gone, or could have different permissions if it was recreated (the +recreation would get its own watch fired). + +Inside a tree we lookup the watch path's parent, and then the watch path +child itself. This gets us 4 sets of permissions in worst case, and if +either of these allows a watch, then we permit it to fire. The +permission lookups are done without logging the failures, otherwise we'd +get confusing errors about permission denied for some paths, but a watch +still firing. The actual result is logged in xenstored-access log: + + 'w event ...' as usual if watch was fired + 'w notfired...' if the watch was not fired, together with path and + permission set to help in troubleshooting + +Adding a watch bypasses permission checks and always fires the watch +once immediately. This is consistent with the specification, and no +information is gained (the watch is fired both if the path exists or +doesn't, and both if you have or don't have access, i.e. it reflects the +path a domain gave it back to that domain). + +There are some semantic changes here: + + * Write+rm in a single transaction of the same path is unobservable + now via watches: both before and after a transaction the path + doesn't exist, thus both tree lookups come up with the empty + permission set, and noone, not even Dom0 can see this. This is + consistent with transaction atomicity though. + * Similar to above if we temporarily grant and then revoke permission + on a path any watches fired inbetween are ignored as well + * There is a new log event (w notfired) which shows the permission set + of the path, and the path. + * Watches on paths that a domain doesn't have access to are now not + seen, which is the purpose of the security fix. + +This is part of XSA-115. + +Signed-off-by: Edwin Török +Acked-by: Christian Lindig +Reviewed-by: Andrew Cooper + +diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml +index d7432c6597..1389d971c2 100644 +--- tools/ocaml/xenstored/connection.ml.orig ++++ tools/ocaml/xenstored/connection.ml +@@ -196,11 +196,36 @@ let list_watches con = + con.watches [] in + List.concat ll + +-let fire_single_watch watch = ++let dbg fmt = Logging.debug "connection" fmt ++let info fmt = Logging.info "connection" fmt ++ ++let lookup_watch_perm path = function ++| None -> [] ++| Some root -> ++ try Store.Path.apply root path @@ fun parent name -> ++ Store.Node.get_perms parent :: ++ try [Store.Node.get_perms (Store.Node.find parent name)] ++ with Not_found -> [] ++ with Define.Invalid_path | Not_found -> [] ++ ++let lookup_watch_perms oldroot root path = ++ lookup_watch_perm path oldroot @ lookup_watch_perm path (Some root) ++ ++let fire_single_watch_unchecked watch = + let data = Utils.join_by_null [watch.path; watch.token; ""] in + send_reply watch.con Transaction.none 0 Xenbus.Xb.Op.Watchevent data + +-let fire_watch watch path = ++let fire_single_watch (oldroot, root) watch = ++ let abspath = get_watch_path watch.con watch.path |> Store.Path.of_string in ++ let perms = lookup_watch_perms oldroot root abspath in ++ if List.exists (Perms.has watch.con.perm READ) perms then ++ fire_single_watch_unchecked watch ++ else ++ let perms = perms |> List.map (Perms.Node.to_string ~sep:" ") |> String.concat ", " in ++ let con = get_domstr watch.con in ++ Logging.watch_not_fired ~con perms (Store.Path.to_string abspath) ++ ++let fire_watch roots watch path = + let new_path = + if watch.is_relative && path.[0] = '/' + then begin +@@ -210,7 +235,7 @@ let fire_watch watch path = + end else + path + in +- fire_single_watch { watch with path = new_path } ++ fire_single_watch roots { watch with path = new_path } + + (* Search for a valid unused transaction id. *) + let rec valid_transaction_id con proposed_id = +diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml +index ae7692819d..020b875dcd 100644 +--- tools/ocaml/xenstored/connections.ml.orig ++++ tools/ocaml/xenstored/connections.ml +@@ -135,25 +135,26 @@ let del_watch cons con path token = + watch + + (* path is absolute *) +-let fire_watches cons path recurse = ++let fire_watches ?oldroot root cons path recurse = + let key = key_of_path path in + let path = Store.Path.to_string path in ++ let roots = oldroot, root in + let fire_watch _ = function + | None -> () +- | Some watches -> List.iter (fun w -> Connection.fire_watch w path) watches ++ | Some watches -> List.iter (fun w -> Connection.fire_watch roots w path) watches + in + let fire_rec x = function + | None -> () + | Some watches -> +- List.iter (fun w -> Connection.fire_single_watch w) watches ++ List.iter (Connection.fire_single_watch roots) watches + in + Trie.iter_path fire_watch cons.watches key; + if recurse then + Trie.iter fire_rec (Trie.sub cons.watches key) + +-let fire_spec_watches cons specpath = ++let fire_spec_watches root cons specpath = + iter cons (fun con -> +- List.iter (fun w -> Connection.fire_single_watch w) (Connection.get_watches con specpath)) ++ List.iter (Connection.fire_single_watch (None, root)) (Connection.get_watches con specpath)) + + let set_target cons domain target_domain = + let con = find_domain cons domain in +diff --git a/tools/ocaml/xenstored/logging.ml b/tools/ocaml/xenstored/logging.ml +index ea6033195d..99c7bc5e13 100644 +--- tools/ocaml/xenstored/logging.ml.orig ++++ tools/ocaml/xenstored/logging.ml +@@ -161,6 +161,8 @@ let xenstored_log_nb_lines = ref 13215 + let xenstored_log_nb_chars = ref (-1) + let xenstored_logger = ref (None: logger option) + ++let debug_enabled () = !xenstored_log_level = Debug ++ + let set_xenstored_log_destination s = + xenstored_log_destination := log_destination_of_string s + +@@ -204,6 +206,7 @@ type access_type = + | Commit + | Newconn + | Endconn ++ | Watch_not_fired + | XbOp of Xenbus.Xb.Op.operation + + let string_of_tid ~con tid = +@@ -217,6 +220,7 @@ let string_of_access_type = function + | Commit -> "commit " + | Newconn -> "newconn " + | Endconn -> "endconn " ++ | Watch_not_fired -> "w notfired" + + | XbOp op -> match op with + | Xenbus.Xb.Op.Debug -> "debug " +@@ -331,3 +335,7 @@ let xb_answer ~tid ~con ~ty data = + | _ -> false, Debug + in + if print then access_logging ~tid ~con ~data (XbOp ty) ~level ++ ++let watch_not_fired ~con perms path = ++ let data = Printf.sprintf "EPERM perms=[%s] path=%s" perms path in ++ access_logging ~tid:0 ~con ~data Watch_not_fired ~level:Info +diff --git a/tools/ocaml/xenstored/perms.ml b/tools/ocaml/xenstored/perms.ml +index 3ea193ea14..23b80aba3d 100644 +--- tools/ocaml/xenstored/perms.ml.orig ++++ tools/ocaml/xenstored/perms.ml +@@ -79,9 +79,9 @@ let of_string s = + let string_of_perm perm = + Printf.sprintf "%c%u" (char_of_permty (snd perm)) (fst perm) + +-let to_string permvec = ++let to_string ?(sep="\000") permvec = + let l = ((permvec.owner, permvec.other) :: permvec.acl) in +- String.concat "\000" (List.map string_of_perm l) ++ String.concat sep (List.map string_of_perm l) + + end + +@@ -132,8 +132,8 @@ let check_owner (connection:Connection.t) (node:Node.t) = + then Connection.is_owner connection (Node.get_owner node) + else true + +-(* check if the current connection has the requested perm on the current node *) +-let check (connection:Connection.t) request (node:Node.t) = ++(* check if the current connection lacks the requested perm on the current node *) ++let lacks (connection:Connection.t) request (node:Node.t) = + let check_acl domainid = + let perm = + if List.mem_assoc domainid (Node.get_acl node) +@@ -154,11 +154,19 @@ let check (connection:Connection.t) request (node:Node.t) = + info "Permission denied: Domain %d has write only access" domainid; + false + in +- if !activate ++ !activate + && not (Connection.is_dom0 connection) + && not (check_owner connection node) + && not (List.exists check_acl (Connection.get_owners connection)) ++ ++(* check if the current connection has the requested perm on the current node. ++* Raises an exception if it doesn't. *) ++let check connection request node = ++ if lacks connection request node + then raise Define.Permission_denied + ++(* check if the current connection has the requested perm on the current node *) ++let has connection request node = not (lacks connection request node) ++ + let equiv perm1 perm2 = + (Node.to_string perm1) = (Node.to_string perm2) +diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml +index c3c8ea2f4b..3cd0097db9 100644 +--- tools/ocaml/xenstored/process.ml.orig ++++ tools/ocaml/xenstored/process.ml +@@ -56,15 +56,17 @@ let split_one_path data con = + | path :: "" :: [] -> Store.Path.create path (Connection.get_path con) + | _ -> raise Invalid_Cmd_Args + +-let process_watch ops cons = ++let process_watch t cons = ++ let oldroot = t.Transaction.oldroot in ++ let newroot = Store.get_root t.store in ++ let ops = Transaction.get_paths t |> List.rev in + let do_op_watch op cons = +- let recurse = match (fst op) with +- | Xenbus.Xb.Op.Write -> false +- | Xenbus.Xb.Op.Mkdir -> false +- | Xenbus.Xb.Op.Rm -> true +- | Xenbus.Xb.Op.Setperms -> false ++ let recurse, oldroot, root = match (fst op) with ++ | Xenbus.Xb.Op.Write|Xenbus.Xb.Op.Mkdir -> false, None, newroot ++ | Xenbus.Xb.Op.Rm -> true, None, oldroot ++ | Xenbus.Xb.Op.Setperms -> false, Some oldroot, newroot + | _ -> raise (Failure "huh ?") in +- Connections.fire_watches cons (snd op) recurse in ++ Connections.fire_watches ?oldroot root cons (snd op) recurse in + List.iter (fun op -> do_op_watch op cons) ops + + let create_implicit_path t perm path = +@@ -205,7 +207,7 @@ let reply_ack fct con t doms cons data = + fct con t doms cons data; + Packet.Ack (fun () -> + if Transaction.get_id t = Transaction.none then +- process_watch (Transaction.get_paths t) cons ++ process_watch t cons + ) + + let reply_data fct con t doms cons data = +@@ -353,14 +355,17 @@ let transaction_replay c t doms cons = + Connection.end_transaction c tid None + ) + +-let do_watch con t domains cons data = ++let do_watch con t _domains cons data = + let (node, token) = + match (split None '\000' data) with + | [node; token; ""] -> node, token + | _ -> raise Invalid_Cmd_Args + in + let watch = Connections.add_watch cons con node token in +- Packet.Ack (fun () -> Connection.fire_single_watch watch) ++ Packet.Ack (fun () -> ++ (* xenstore.txt says this watch is fired immediately, ++ implying even if path doesn't exist or is unreadable *) ++ Connection.fire_single_watch_unchecked watch) + + let do_unwatch con t domains cons data = + let (node, token) = +@@ -391,7 +396,7 @@ let do_transaction_end con t domains cons data = + if not success then + raise Transaction_again; + if commit then begin +- process_watch (List.rev (Transaction.get_paths t)) cons; ++ process_watch t cons; + match t.Transaction.ty with + | Transaction.No -> + () (* no need to record anything *) +@@ -414,7 +419,7 @@ let do_introduce con t domains cons data = + else try + let ndom = Domains.create domains domid mfn port in + Connections.add_domain cons ndom; +- Connections.fire_spec_watches cons Store.Path.introduce_domain; ++ Connections.fire_spec_watches (Transaction.get_root t) cons Store.Path.introduce_domain; + ndom + with _ -> raise Invalid_Cmd_Args + in +@@ -433,7 +438,7 @@ let do_release con t domains cons data = + Domains.del domains domid; + Connections.del_domain cons domid; + if fire_spec_watches +- then Connections.fire_spec_watches cons Store.Path.release_domain ++ then Connections.fire_spec_watches (Transaction.get_root t) cons Store.Path.release_domain + else raise Invalid_Cmd_Args + + let do_resume con t domains cons data = +@@ -501,6 +506,8 @@ let maybe_ignore_transaction = function + Transaction.none + | _ -> fun x -> x + ++ ++let () = Printexc.record_backtrace true + (** + * Nothrow guarantee. + *) +@@ -542,7 +549,8 @@ let process_packet ~store ~cons ~doms ~con ~req = + (* Put the response on the wire *) + send_response ty con t rid response + with exn -> +- error "process packet: %s" (Printexc.to_string exn); ++ let bt = Printexc.get_backtrace () in ++ error "process packet: %s. %s" (Printexc.to_string exn) bt; + Connection.send_error con tid rid "EIO" + + let do_input store cons doms con = +diff --git a/tools/ocaml/xenstored/transaction.ml b/tools/ocaml/xenstored/transaction.ml +index 23e7ccff1b..9e9e28db9b 100644 +--- tools/ocaml/xenstored/transaction.ml.orig ++++ tools/ocaml/xenstored/transaction.ml +@@ -82,6 +82,7 @@ type t = { + start_count: int64; + store: Store.t; (* This is the store that we change in write operations. *) + quota: Quota.t; ++ oldroot: Store.Node.t; + mutable paths: (Xenbus.Xb.Op.operation * Store.Path.t) list; + mutable operations: (Packet.request * Packet.response) list; + mutable read_lowpath: Store.Path.t option; +@@ -123,6 +124,7 @@ let make ?(internal=false) id store = + start_count = !counter; + store = if id = none then store else Store.copy store; + quota = Quota.copy store.Store.quota; ++ oldroot = Store.get_root store; + paths = []; + operations = []; + read_lowpath = None; +@@ -137,6 +139,8 @@ let make ?(internal=false) id store = + let get_store t = t.store + let get_paths t = t.paths + ++let get_root t = Store.get_root t.store ++ + let is_read_only t = t.paths = [] + let add_wop t ty path = t.paths <- (ty, path) :: t.paths + let add_operation ~perm t request response = +diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml +index 32c3b1c0f1..e9f471846f 100644 +--- tools/ocaml/xenstored/xenstored.ml.orig ++++ tools/ocaml/xenstored/xenstored.ml +@@ -341,7 +341,9 @@ let _ = + let (notify, deaddom) = Domains.cleanup domains in + List.iter (Connections.del_domain cons) deaddom; + if deaddom <> [] || notify then +- Connections.fire_spec_watches cons Store.Path.release_domain ++ Connections.fire_spec_watches ++ (Store.get_root store) ++ cons Store.Path.release_domain + ) + else + let c = Connections.find_domain_by_port cons port in +From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= +Subject: tools/ocaml/xenstored: add xenstored.conf flag to turn off watch + permission checks +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +There are flags to turn off quotas and the permission system, so add one +that turns off the newly introduced watch permission checks as well. + +This is part of XSA-115. + +Signed-off-by: Edwin Török +Acked-by: Christian Lindig +Reviewed-by: Andrew Cooper + +diff --git a/tools/ocaml/xenstored/connection.ml b/tools/ocaml/xenstored/connection.ml +index 1389d971c2..698f721345 100644 +--- tools/ocaml/xenstored/connection.ml.orig ++++ tools/ocaml/xenstored/connection.ml +@@ -218,7 +218,7 @@ let fire_single_watch_unchecked watch = + let fire_single_watch (oldroot, root) watch = + let abspath = get_watch_path watch.con watch.path |> Store.Path.of_string in + let perms = lookup_watch_perms oldroot root abspath in +- if List.exists (Perms.has watch.con.perm READ) perms then ++ if Perms.can_fire_watch watch.con.perm perms then + fire_single_watch_unchecked watch + else + let perms = perms |> List.map (Perms.Node.to_string ~sep:" ") |> String.concat ", " in +diff --git a/tools/ocaml/xenstored/oxenstored.conf.in b/tools/ocaml/xenstored/oxenstored.conf.in +index 6579b84448..d5d4f00de8 100644 +--- tools/ocaml/xenstored/oxenstored.conf.in.orig ++++ tools/ocaml/xenstored/oxenstored.conf.in +@@ -44,6 +44,16 @@ conflict-rate-limit-is-aggregate = true + # Activate node permission system + perms-activate = true + ++# Activate the watch permission system ++# When this is enabled unprivileged guests can only get watch events ++# for xenstore entries that they would've been able to read. ++# ++# When this is disabled unprivileged guests may get watch events ++# for xenstore entries that they cannot read. The watch event contains ++# only the entry name, not the value. ++# This restores behaviour prior to XSA-115. ++perms-watch-activate = true ++ + # Activate quota + quota-activate = true + quota-maxentity = 1000 +diff --git a/tools/ocaml/xenstored/perms.ml b/tools/ocaml/xenstored/perms.ml +index 23b80aba3d..ee7fee6bda 100644 +--- tools/ocaml/xenstored/perms.ml.orig ++++ tools/ocaml/xenstored/perms.ml +@@ -20,6 +20,7 @@ let info fmt = Logging.info "perms" fmt + open Stdext + + let activate = ref true ++let watch_activate = ref true + + type permty = READ | WRITE | RDWR | NONE + +@@ -168,5 +169,9 @@ let check connection request node = + (* check if the current connection has the requested perm on the current node *) + let has connection request node = not (lacks connection request node) + ++let can_fire_watch connection perms = ++ not !watch_activate ++ || List.exists (has connection READ) perms ++ + let equiv perm1 perm2 = + (Node.to_string perm1) = (Node.to_string perm2) +diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml +index e9f471846f..30fc874327 100644 +--- tools/ocaml/xenstored/xenstored.ml.orig ++++ tools/ocaml/xenstored/xenstored.ml +@@ -95,6 +95,7 @@ let parse_config filename = + ("conflict-max-history-seconds", Config.Set_float Define.conflict_max_history_seconds); + ("conflict-rate-limit-is-aggregate", Config.Set_bool Define.conflict_rate_limit_is_aggregate); + ("perms-activate", Config.Set_bool Perms.activate); ++ ("perms-watch-activate", Config.Set_bool Perms.watch_activate); + ("quota-activate", Config.Set_bool Quota.activate); + ("quota-maxwatch", Config.Set_int Define.maxwatch); + ("quota-transaction", Config.Set_int Define.maxtransaction); Index: pkgsrc/sysutils/xentools411/patches/patch-XSA322-c diff -u /dev/null pkgsrc/sysutils/xentools411/patches/patch-XSA322-c:1.1 --- /dev/null Thu Dec 17 16:48:12 2020 +++ pkgsrc/sysutils/xentools411/patches/patch-XSA322-c Thu Dec 17 16:48:12 2020 @@ -0,0 +1,536 @@ +$NetBSD: patch-XSA322-c,v 1.1 2020/12/17 16:48:12 bouyer Exp $ + +From: Juergen Gross +Subject: tools/xenstore: revoke access rights for removed domains + +Access rights of Xenstore nodes are per domid. Unfortunately existing +granted access rights are not removed when a domain is being destroyed. +This means that a new domain created with the same domid will inherit +the access rights to Xenstore nodes from the previous domain(s) with +the same domid. + +This can be avoided by adding a generation counter to each domain. +The generation counter of the domain is set to the global generation +counter when a domain structure is being allocated. When reading or +writing a node all permissions of domains which are younger than the +node itself are dropped. This is done by flagging the related entry +as invalid in order to avoid modifying permissions in a way the user +could detect. + +A special case has to be considered: for a new domain the first +Xenstore entries are already written before the domain is officially +introduced in Xenstore. In order not to drop the permissions for the +new domain a domain struct is allocated even before introduction if +the hypervisor is aware of the domain. This requires adding another +bool "introduced" to struct domain in xenstored. In order to avoid +additional padding holes convert the shutdown flag to bool, too. + +As verifying permissions has its price regarding runtime add a new +quota for limiting the number of permissions an unprivileged domain +can set for a node. The default for that new quota is 5. + +This is part of XSA-322. + +Signed-off-by: Juergen Gross +Reviewed-by: Paul Durrant +Acked-by: Julien Grall + +diff --git a/tools/xenstore/include/xenstore_lib.h b/tools/xenstore/include/xenstore_lib.h +index 0ffbae9eb574..4c9b6d16858d 100644 +--- tools/xenstore/include/xenstore_lib.h.orig ++++ tools/xenstore/include/xenstore_lib.h +@@ -34,6 +34,7 @@ enum xs_perm_type { + /* Internal use. */ + XS_PERM_ENOENT_OK = 4, + XS_PERM_OWNER = 8, ++ XS_PERM_IGNORE = 16, + }; + + struct xs_permissions +diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c +index 2a86c4aa5bce..4fbe5c759c1b 100644 +--- tools/xenstore/xenstored_core.c.orig ++++ tools/xenstore/xenstored_core.c +@@ -101,6 +101,7 @@ int quota_nb_entry_per_domain = 1000; + int quota_nb_watch_per_domain = 128; + int quota_max_entry_size = 2048; /* 2K */ + int quota_max_transaction = 10; ++int quota_nb_perms_per_node = 5; + + void trace(const char *fmt, ...) + { +@@ -407,8 +408,13 @@ 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)) { ++ talloc_free(node); ++ return NULL; ++ } ++ + /* Data is binary blob (usually ascii, no nul). */ +- node->data = node->perms.p + node->perms.num; ++ node->data = node->perms.p + hdr->num_perms; + /* Children is strings, nul separated. */ + node->children = node->data + node->datalen; + +@@ -424,6 +430,9 @@ 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)) ++ return errno; ++ + data.dsize = sizeof(*hdr) + + node->perms.num * sizeof(node->perms.p[0]) + + node->datalen + node->childlen; +@@ -483,8 +492,9 @@ enum xs_perm_type perm_for_conn(struct connection *conn, + return (XS_PERM_READ|XS_PERM_WRITE|XS_PERM_OWNER) & mask; + + for (i = 1; i < perms->num; i++) +- if (perms->p[i].id == conn->id +- || (conn->target && perms->p[i].id == conn->target->id)) ++ if (!(perms->p[i].perms & XS_PERM_IGNORE) && ++ (perms->p[i].id == conn->id || ++ (conn->target && perms->p[i].id == conn->target->id))) + return perms->p[i].perms & mask; + + return perms->p[0].perms & mask; +@@ -1246,8 +1256,12 @@ static int do_set_perms(struct connection *conn, struct buffered_data *in) + if (perms.num < 2) + return EINVAL; + +- permstr = in->buffer + strlen(in->buffer) + 1; + perms.num--; ++ if (domain_is_unprivileged(conn) && ++ perms.num > quota_nb_perms_per_node) ++ return ENOSPC; ++ ++ permstr = in->buffer + strlen(in->buffer) + 1; + + perms.p = talloc_array(in, struct xs_permissions, perms.num); + if (!perms.p) +@@ -1919,6 +1933,7 @@ static void usage(void) + " -S, --entry-size limit the size of entry per domain, and\n" + " -W, --watch-nb limit the number of watches per domain,\n" + " -t, --transaction limit the number of transaction allowed per domain,\n" ++" -A, --perm-nb limit the number of permissions per node,\n" + " -R, --no-recovery to request that no recovery should be attempted when\n" + " the store is corrupted (debug only),\n" + " -I, --internal-db store database in memory, not on disk\n" +@@ -1939,6 +1954,7 @@ static struct option options[] = { + { "entry-size", 1, NULL, 'S' }, + { "trace-file", 1, NULL, 'T' }, + { "transaction", 1, NULL, 't' }, ++ { "perm-nb", 1, NULL, 'A' }, + { "no-recovery", 0, NULL, 'R' }, + { "internal-db", 0, NULL, 'I' }, + { "verbose", 0, NULL, 'V' }, +@@ -1961,7 +1977,7 @@ int main(int argc, char *argv[]) + int timeout; + + +- while ((opt = getopt_long(argc, argv, "DE:F:HNPS:t:T:RVW:", options, ++ while ((opt = getopt_long(argc, argv, "DE:F:HNPS:t:A:T:RVW:", options, + NULL)) != -1) { + switch (opt) { + case 'D': +@@ -2003,6 +2019,9 @@ int main(int argc, char *argv[]) + case 'W': + quota_nb_watch_per_domain = strtol(optarg, NULL, 10); + break; ++ case 'A': ++ quota_nb_perms_per_node = strtol(optarg, NULL, 10); ++ break; + case 'e': + dom0_event = strtol(optarg, NULL, 10); + break; +diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c +index 0b2f49ac7d4c..f5e7af46e8aa 100644 +--- tools/xenstore/xenstored_domain.c.orig ++++ tools/xenstore/xenstored_domain.c +@@ -71,8 +71,14 @@ struct domain + /* The connection associated with this. */ + struct connection *conn; + ++ /* Generation count at domain introduction time. */ ++ uint64_t generation; ++ + /* Have we noticed that this domain is shutdown? */ +- int shutdown; ++ bool shutdown; ++ ++ /* Has domain been officially introduced? */ ++ bool introduced; + + /* number of entry from this domain in the store */ + int nbentry; +@@ -200,6 +206,9 @@ static int destroy_domain(void *_domain) + + list_del(&domain->list); + ++ if (!domain->introduced) ++ return 0; ++ + if (domain->port) { + if (xenevtchn_unbind(xce_handle, domain->port) == -1) + eprintf("> Unbinding port %i failed!\n", domain->port); +@@ -230,20 +230,33 @@ + return 0; + } + ++static bool get_domain_info(unsigned int domid, xc_dominfo_t *dominfo) ++{ ++ return xc_domain_getinfo(*xc_handle, domid, 1, dominfo) == 1 && ++ dominfo->domid == domid; ++} ++ + static void domain_cleanup(void) + { + xc_dominfo_t dominfo; + struct domain *domain; + int notify = 0; ++ bool dom_valid; + + again: + list_for_each_entry(domain, &domains, list) { +- if (xc_domain_getinfo(*xc_handle, domain->domid, 1, +- &dominfo) == 1 && +- dominfo.domid == domain->domid) { ++ dom_valid = get_domain_info(domain->domid, &dominfo); ++ if (!domain->introduced) { ++ if (!dom_valid) { ++ talloc_free(domain); ++ goto again; ++ } ++ continue; ++ } ++ if (dom_valid) { + if ((dominfo.crashed || dominfo.shutdown) + && !domain->shutdown) { +- domain->shutdown = 1; ++ domain->shutdown = true; + notify = 1; + } + if (!dominfo.dying) +@@ -301,58 +323,84 @@ static char *talloc_domain_path(void *context, unsigned int domid) + return talloc_asprintf(context, "/local/domain/%u", domid); + } + +-static struct domain *new_domain(void *context, unsigned int domid, +- int port) ++static struct domain *find_domain_struct(unsigned int domid) ++{ ++ struct domain *i; ++ ++ list_for_each_entry(i, &domains, list) { ++ if (i->domid == domid) ++ return i; ++ } ++ return NULL; ++} ++ ++static struct domain *alloc_domain(void *context, unsigned int domid) + { + struct domain *domain; +- int rc; + + domain = talloc(context, struct domain); +- if (!domain) ++ if (!domain) { ++ errno = ENOMEM; + return NULL; ++ } + +- domain->port = 0; +- domain->shutdown = 0; + domain->domid = domid; +- domain->path = talloc_domain_path(domain, domid); +- if (!domain->path) +- return NULL; ++ domain->generation = generation; ++ domain->introduced = false; + +- wrl_domain_new(domain); ++ talloc_set_destructor(domain, destroy_domain); + + list_add(&domain->list, &domains); +- talloc_set_destructor(domain, destroy_domain); ++ ++ return domain; ++} ++ ++static int new_domain(struct domain *domain, int port) ++{ ++ int rc; ++ ++ domain->port = 0; ++ domain->shutdown = false; ++ domain->path = talloc_domain_path(domain, domain->domid); ++ if (!domain->path) { ++ errno = ENOMEM; ++ return errno; ++ } ++ ++ wrl_domain_new(domain); + + /* Tell kernel we're interested in this event. */ +- rc = xenevtchn_bind_interdomain(xce_handle, domid, port); ++ rc = xenevtchn_bind_interdomain(xce_handle, domain->domid, port); + if (rc == -1) +- return NULL; ++ return errno; + domain->port = rc; + ++ domain->introduced = true; ++ + domain->conn = new_connection(writechn, readchn); +- if (!domain->conn) +- return NULL; ++ if (!domain->conn) { ++ errno = ENOMEM; ++ return errno; ++ } + + domain->conn->domain = domain; +- domain->conn->id = domid; ++ domain->conn->id = domain->domid; + + domain->remote_port = port; + domain->nbentry = 0; + domain->nbwatch = 0; + +- return domain; ++ return 0; + } + + + static struct domain *find_domain_by_domid(unsigned int domid) + { +- struct domain *i; ++ struct domain *d; + +- list_for_each_entry(i, &domains, list) { +- if (i->domid == domid) +- return i; +- } +- return NULL; ++ d = find_domain_struct(domid); ++ ++ return (d && d->introduced) ? d : NULL; + } + + static void domain_conn_reset(struct domain *domain) +@@ -399,15 +447,21 @@ int do_introduce(struct connection *conn, struct buffered_data *in) + if (port <= 0) + return EINVAL; + +- domain = find_domain_by_domid(domid); ++ domain = find_domain_struct(domid); + + if (domain == NULL) { ++ /* Hang domain off "in" until we're finished. */ ++ domain = alloc_domain(in, domid); ++ if (domain == NULL) ++ return ENOMEM; ++ } ++ ++ if (!domain->introduced) { + interface = map_interface(domid, mfn); + if (!interface) + return errno; + /* Hang domain off "in" until we're finished. */ +- domain = new_domain(in, domid, port); +- if (!domain) { ++ if (new_domain(domain, port)) { + rc = errno; + unmap_interface(interface); + return rc; +@@ -518,8 +572,8 @@ int do_resume(struct connection *conn, struct buffered_data *in) + if (IS_ERR(domain)) + return -PTR_ERR(domain); + +- domain->shutdown = 0; +- ++ domain->shutdown = false; ++ + send_ack(conn, XS_RESUME); + + return 0; +@@ -662,8 +716,10 @@ static int dom0_init(void) + if (port == -1) + return -1; + +- dom0 = new_domain(NULL, xenbus_master_domid(), port); +- if (dom0 == NULL) ++ dom0 = alloc_domain(NULL, xenbus_master_domid()); ++ if (!dom0) ++ return -1; ++ if (new_domain(dom0, port)) + return -1; + + dom0->interface = xenbus_map(); +@@ -744,6 +800,66 @@ void domain_entry_inc(struct connection *conn, struct node *node) + } + } + ++/* ++ * Check whether a domain was created before or after a specific generation ++ * count (used for testing whether a node permission is older than a domain). ++ * ++ * Return values: ++ * -1: error ++ * 0: domain has higher generation count (it is younger than a node with the ++ * given count), or domain isn't existing any longer ++ * 1: domain is older than the node ++ */ ++static int chk_domain_generation(unsigned int domid, uint64_t gen) ++{ ++ struct domain *d; ++ xc_dominfo_t dominfo; ++ ++ if (!xc_handle && domid == 0) ++ return 1; ++ ++ d = find_domain_struct(domid); ++ if (d) ++ return (d->generation <= gen) ? 1 : 0; ++ ++ if (!get_domain_info(domid, &dominfo)) ++ return 0; ++ ++ d = alloc_domain(NULL, domid); ++ return d ? 1 : -1; ++} ++ ++/* ++ * 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) ++{ ++ unsigned int i; ++ int ret; ++ ++ ret = chk_domain_generation(node->perms.p[0].id, node->generation); ++ if (ret < 0) ++ return errno; ++ ++ /* If the owner doesn't exist any longer give it to priv domain. */ ++ if (!ret) ++ node->perms.p[0].id = priv_domid; ++ ++ for (i = 1; i < node->perms.num; i++) { ++ if (node->perms.p[i].perms & XS_PERM_IGNORE) ++ continue; ++ ret = chk_domain_generation(node->perms.p[i].id, ++ node->generation); ++ if (ret < 0) ++ return errno; ++ if (!ret) ++ node->perms.p[i].perms |= XS_PERM_IGNORE; ++ } ++ ++ return 0; ++} ++ + void domain_entry_dec(struct connection *conn, struct node *node) + { + struct domain *d; +diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h +index 259183962a9c..5e00087206c7 100644 +--- tools/xenstore/xenstored_domain.h.orig ++++ tools/xenstore/xenstored_domain.h +@@ -56,6 +56,9 @@ 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); ++ + /* Quota manipulation */ + void domain_entry_inc(struct connection *conn, struct node *); + void domain_entry_dec(struct connection *conn, struct node *); +diff --git a/tools/xenstore/xenstored_transaction.c b/tools/xenstore/xenstored_transaction.c +index 36793b9b1af3..9fcb4c9ba986 100644 +--- tools/xenstore/xenstored_transaction.c.orig ++++ tools/xenstore/xenstored_transaction.c +@@ -47,7 +47,12 @@ + * transaction. + * Each time the global generation count is copied to either a node or a + * transaction it is incremented. This ensures all nodes and/or transactions +- * are having a unique generation count. ++ * are having a unique generation count. The increment is done _before_ the ++ * copy as that is needed for checking whether a domain was created before ++ * or after a node has been written (the domain's generation is set with the ++ * actual generation count without incrementing it, in order to support ++ * writing a node for a domain before the domain has been officially ++ * introduced). + * + * Transaction conflicts are detected by checking the generation count of all + * nodes read in the transaction to match with the generation count in the +@@ -161,7 +166,7 @@ struct transaction + }; + + extern int quota_max_transaction; +-static uint64_t generation; ++uint64_t generation; + + static void set_tdb_key(const char *name, TDB_DATA *key) + { +@@ -237,7 +242,7 @@ int access_node(struct connection *conn, struct node *node, + bool introduce = false; + + if (type != NODE_ACCESS_READ) { +- node->generation = generation++; ++ node->generation = ++generation; + if (conn && !conn->transaction) + wrl_apply_debit_direct(conn); + } +@@ -374,7 +379,7 @@ static int finalize_transaction(struct connection *conn, + if (!data.dptr) + goto err; + hdr = (void *)data.dptr; +- hdr->generation = generation++; ++ hdr->generation = ++generation; + ret = tdb_store(tdb_ctx, key, data, + TDB_REPLACE); + talloc_free(data.dptr); +@@ -462,7 +467,7 @@ int do_transaction_start(struct connection *conn, struct buffered_data *in) + INIT_LIST_HEAD(&trans->accessed); + INIT_LIST_HEAD(&trans->changed_domains); + trans->fail = false; +- trans->generation = generation++; ++ trans->generation = ++generation; + + /* Pick an unused transaction identifier. */ + do { +diff --git a/tools/xenstore/xenstored_transaction.h b/tools/xenstore/xenstored_transaction.h +index 3386bac56508..43a162bea3f3 100644 +--- tools/xenstore/xenstored_transaction.h.orig ++++ tools/xenstore/xenstored_transaction.h +@@ -27,6 +27,8 @@ enum node_access_type { + + struct transaction; + ++extern uint64_t generation; ++ + int do_transaction_start(struct connection *conn, struct buffered_data *node); + int do_transaction_end(struct connection *conn, struct buffered_data *in); + +diff --git a/tools/xenstore/xs_lib.c b/tools/xenstore/xs_lib.c +index 3e43f8809d42..d407d5713aff 100644 +--- tools/xenstore/xs_lib.c.orig ++++ tools/xenstore/xs_lib.c +@@ -152,7 +152,7 @@ bool xs_strings_to_perms(struct xs_permissions *perms, unsigned int num, + bool xs_perm_to_string(const struct xs_permissions *perm, + char *buffer, size_t buf_len) + { +- switch ((int)perm->perms) { ++ switch ((int)perm->perms & ~XS_PERM_IGNORE) { + case XS_PERM_WRITE: + *buffer = 'w'; + break; +-- +2.17.1 + Index: pkgsrc/sysutils/xentools411/patches/patch-XSA322-o diff -u /dev/null pkgsrc/sysutils/xentools411/patches/patch-XSA322-o:1.1 --- /dev/null Thu Dec 17 16:48:12 2020 +++ pkgsrc/sysutils/xentools411/patches/patch-XSA322-o Thu Dec 17 16:48:12 2020 @@ -0,0 +1,112 @@ +$NetBSD: patch-XSA322-o,v 1.1 2020/12/17 16:48:12 bouyer Exp $ + +From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= +Subject: tools/ocaml/xenstored: clean up permissions for dead domains +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +domain ids are prone to wrapping (15-bits), and with sufficient number +of VMs in a reboot loop it is possible to trigger it. Xenstore entries +may linger after a domain dies, until a toolstack cleans it up. During +this time there is a window where a wrapped domid could access these +xenstore keys (that belonged to another VM). + +To prevent this do a cleanup when a domain dies: + * walk the entire xenstore tree and update permissions for all nodes + * if the dead domain had an ACL entry: remove it + * if the dead domain was the owner: change the owner to Dom0 + +This is done without quota checks or a transaction. Quota checks would +be a no-op (either the domain is dead, or it is Dom0 where they are not +enforced). Transactions are not needed, because this is all done +atomically by oxenstored's single thread. + +The xenstore entries owned by the dead domain are not deleted, because +that could confuse a toolstack / backends that are still bound to it +(or generate unexpected watch events). It is the responsibility of a +toolstack to remove the xenstore entries themselves. + +This is part of XSA-322. + +Signed-off-by: Edwin Török +Acked-by: Christian Lindig + +diff --git a/tools/ocaml/xenstored/perms.ml b/tools/ocaml/xenstored/perms.ml +index ee7fee6bda..e8a16221f8 100644 +--- tools/ocaml/xenstored/perms.ml.orig ++++ tools/ocaml/xenstored/perms.ml +@@ -58,6 +58,15 @@ let get_other perms = perms.other + let get_acl perms = perms.acl + let get_owner perm = perm.owner + ++(** [remote_domid ~domid perm] removes all ACLs for [domid] from perm. ++* If [domid] was the owner then it is changed to Dom0. ++* This is used for cleaning up after dead domains. ++* *) ++let remove_domid ~domid perm = ++ let acl = List.filter (fun (acl_domid, _) -> acl_domid <> domid) perm.acl in ++ let owner = if perm.owner = domid then 0 else perm.owner in ++ { perm with acl; owner } ++ + let default0 = create 0 NONE [] + + let perm_of_string s = +diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml +index 3cd0097db9..6a998f8764 100644 +--- tools/ocaml/xenstored/process.ml.orig ++++ tools/ocaml/xenstored/process.ml +@@ -437,6 +437,7 @@ let do_release con t domains cons data = + let fire_spec_watches = Domains.exist domains domid in + Domains.del domains domid; + Connections.del_domain cons domid; ++ Store.reset_permissions (Transaction.get_store t) domid; + if fire_spec_watches + then Connections.fire_spec_watches (Transaction.get_root t) cons Store.Path.release_domain + else raise Invalid_Cmd_Args +diff --git a/tools/ocaml/xenstored/store.ml b/tools/ocaml/xenstored/store.ml +index 0ce6f68e8d..101c094715 100644 +--- tools/ocaml/xenstored/store.ml.orig ++++ tools/ocaml/xenstored/store.ml +@@ -89,6 +89,13 @@ let check_owner node connection = + + let rec recurse fct node = fct node; List.iter (recurse fct) node.children + ++(** [recurse_map f tree] applies [f] on each node in the tree recursively *) ++let recurse_map f = ++ let rec walk node = ++ f { node with children = List.rev_map walk node.children |> List.rev } ++ in ++ walk ++ + let unpack node = (Symbol.to_string node.name, node.perms, node.value) + + end +@@ -405,6 +412,15 @@ let setperms store perm path nperms = + Quota.del_entry store.quota old_owner; + Quota.add_entry store.quota new_owner + ++let reset_permissions store domid = ++ Logging.info "store|node" "Cleaning up xenstore ACLs for domid %d" domid; ++ store.root <- Node.recurse_map (fun node -> ++ let perms = Perms.Node.remove_domid ~domid node.perms in ++ if perms <> node.perms then ++ Logging.debug "store|node" "Changed permissions for node %s" (Node.get_name node); ++ { node with perms } ++ ) store.root ++ + type ops = { + store: t; + write: Path.t -> string -> unit; +diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml +index 30fc874327..183dd2754b 100644 +--- tools/ocaml/xenstored/xenstored.ml.orig ++++ tools/ocaml/xenstored/xenstored.ml +@@ -340,6 +340,7 @@ let _ = + finally (fun () -> + if Some port = eventchn.Event.virq_port then ( + let (notify, deaddom) = Domains.cleanup domains in ++ List.iter (Store.reset_permissions store) deaddom; + List.iter (Connections.del_domain cons) deaddom; + if deaddom <> [] || notify then + Connections.fire_spec_watches Index: pkgsrc/sysutils/xentools411/patches/patch-XSA323 diff -u /dev/null pkgsrc/sysutils/xentools411/patches/patch-XSA323:1.1 --- /dev/null Thu Dec 17 16:48:12 2020 +++ pkgsrc/sysutils/xentools411/patches/patch-XSA323 Thu Dec 17 16:48:12 2020 @@ -0,0 +1,142 @@ +$NetBSD: patch-XSA323,v 1.1 2020/12/17 16:48:12 bouyer Exp $ + +From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= +Subject: tools/ocaml/xenstored: Fix path length validation +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Currently, oxenstored checks the length of paths against 1024, then +prepends "/local/domain/$DOMID/" to relative paths. This allows a domU +to create paths which can't subsequently be read by anyone, even dom0. +This also interferes with listing directories, etc. + +Define a new oxenstored.conf entry: quota-path-max, defaulting to 1024 +as before. For paths that begin with "/local/domain/$DOMID/" check the +relative path length against this quota. For all other paths check the +entire path length. + +This ensures that if the domid changes (and thus the length of a prefix +changes) a path that used to be valid stays valid (e.g. after a +live-migration). It also ensures that regardless how the client tries +to access a path (domid-relative or absolute) it will get consistent +results, since the limit is always applied on the final canonicalized +path. + +Delete the unused Domain.get_path to avoid it being confused with +Connection.get_path (which differs by a trailing slash only). + +Rewrite Util.path_validate to apply the appropriate length restriction +based on whether the path is relative or not. Remove the check for +connection_path being absolute, because it is not guest controlled data. + +This is part of XSA-323. + +Signed-off-by: Andrew Cooper +Signed-off-by: Edwin Török +Acked-by: Christian Lindig + +diff --git a/tools/ocaml/libs/xb/partial.ml b/tools/ocaml/libs/xb/partial.ml +index d4d1c7bdec..b6e2a716e2 100644 +--- tools/ocaml/libs/xb/partial.ml.orig ++++ tools/ocaml/libs/xb/partial.ml +@@ -28,6 +28,7 @@ external header_of_string_internal: string -> int * int * int * int + = "stub_header_of_string" + + let xenstore_payload_max = 4096 (* xen/include/public/io/xs_wire.h *) ++let xenstore_rel_path_max = 2048 (* xen/include/public/io/xs_wire.h *) + + let of_string s = + let tid, rid, opint, dlen = header_of_string_internal s in +diff --git a/tools/ocaml/libs/xb/partial.mli b/tools/ocaml/libs/xb/partial.mli +index 359a75e88d..b9216018f5 100644 +--- tools/ocaml/libs/xb/partial.mli.orig ++++ tools/ocaml/libs/xb/partial.mli +@@ -9,6 +9,7 @@ external header_size : unit -> int = "stub_header_size" + external header_of_string_internal : string -> int * int * int * int + = "stub_header_of_string" + val xenstore_payload_max : int ++val xenstore_rel_path_max : int + val of_string : string -> pkt + val append : pkt -> string -> int -> unit + val to_complete : pkt -> int +diff --git a/tools/ocaml/xenstored/define.ml b/tools/ocaml/xenstored/define.ml +index ea9e1b7620..ebe18b8e31 100644 +--- tools/ocaml/xenstored/define.ml.orig ++++ tools/ocaml/xenstored/define.ml +@@ -31,6 +31,8 @@ let conflict_rate_limit_is_aggregate = ref true + + let domid_self = 0x7FF0 + ++let path_max = ref Xenbus.Partial.xenstore_rel_path_max ++ + exception Not_a_directory of string + exception Not_a_value of string + exception Already_exist +diff --git a/tools/ocaml/xenstored/domain.ml b/tools/ocaml/xenstored/domain.ml +index aeb185ff7e..81cb59b8f1 100644 +--- tools/ocaml/xenstored/domain.ml.orig ++++ tools/ocaml/xenstored/domain.ml +@@ -38,7 +38,6 @@ type t = + } + + let is_dom0 d = d.id = 0 +-let get_path dom = "/local/domain/" ^ (sprintf "%u" dom.id) + let get_id domain = domain.id + let get_interface d = d.interface + let get_mfn d = d.mfn +diff --git a/tools/ocaml/xenstored/oxenstored.conf.in b/tools/ocaml/xenstored/oxenstored.conf.in +index f843482981..4ae48e42d4 100644 +--- tools/ocaml/xenstored/oxenstored.conf.in.orig ++++ tools/ocaml/xenstored/oxenstored.conf.in +@@ -61,6 +61,7 @@ quota-maxsize = 2048 + quota-maxwatch = 100 + quota-transaction = 10 + quota-maxrequests = 1024 ++quota-path-max = 1024 + + # Activate filed base backend + persistent = false +diff --git a/tools/ocaml/xenstored/utils.ml b/tools/ocaml/xenstored/utils.ml +index e8c9fe4e94..eb79bf0146 100644 +--- tools/ocaml/xenstored/utils.ml.orig ++++ tools/ocaml/xenstored/utils.ml +@@ -93,7 +93,7 @@ let read_file_single_integer filename = + let path_validate path connection_path = + let len = String.length path in + +- if len = 0 || len > 1024 then raise Define.Invalid_path; ++ if len = 0 then raise Define.Invalid_path; + + let abs_path = + match String.get path 0 with +@@ -101,4 +101,17 @@ let path_validate path connection_path = + | _ -> connection_path ^ path + in + ++ (* Regardless whether client specified absolute or relative path, ++ canonicalize it (above) and, for domain-relative paths, check the ++ length of the relative part. ++ ++ This prevents paths becoming invalid across migrate when the length ++ of the domid changes in @param connection_path. ++ *) ++ let len = String.length abs_path in ++ let on_absolute _ _ = len in ++ let on_relative _ offset = len - offset in ++ let len = Scanf.ksscanf abs_path on_absolute "/local/domain/%d/%n" on_relative in ++ if len > !Define.path_max then raise Define.Invalid_path; ++ + abs_path +diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml +index ff9fbbbac2..39d6d767e4 100644 +--- tools/ocaml/xenstored/xenstored.ml.orig ++++ tools/ocaml/xenstored/xenstored.ml +@@ -102,6 +102,7 @@ let parse_config filename = + ("quota-maxentity", Config.Set_int Quota.maxent); + ("quota-maxsize", Config.Set_int Quota.maxsize); + ("quota-maxrequests", Config.Set_int Define.maxrequests); ++ ("quota-path-max", Config.Set_int Define.path_max); + ("test-eagain", Config.Set_bool Transaction.test_eagain); + ("persistent", Config.Set_bool Disk.enable); + ("xenstored-log-file", Config.String Logging.set_xenstored_log_destination); Index: pkgsrc/sysutils/xentools411/patches/patch-XSA324 diff -u /dev/null pkgsrc/sysutils/xentools411/patches/patch-XSA324:1.1 --- /dev/null Thu Dec 17 16:48:12 2020 +++ pkgsrc/sysutils/xentools411/patches/patch-XSA324 Thu Dec 17 16:48:12 2020 @@ -0,0 +1,50 @@ +$NetBSD: patch-XSA324,v 1.1 2020/12/17 16:48:12 bouyer Exp $ + +From: Juergen Gross +Subject: tools/xenstore: drop watch event messages exceeding maximum size + +By setting a watch with a very large tag it is possible to trick +xenstored to send watch event messages exceeding the maximum allowed +payload size. This might in turn lead to a crash of xenstored as the +resulting error can cause dereferencing a NULL pointer in case there +is no active request being handled by the guest the watch event is +being sent to. + +Fix that by just dropping such watch events. Additionally modify the +error handling to test the pointer to be not NULL before dereferencing +it. + +This is XSA-324. + +Signed-off-by: Juergen Gross +Acked-by: Julien Grall + +diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c +index 33f95dcf3c..3d74dbbb40 100644 +--- tools/xenstore/xenstored_core.c.orig ++++ tools/xenstore/xenstored_core.c +@@ -674,6 +674,9 @@ void send_reply(struct connection *conn, enum xsd_sockmsg_type type, + /* Replies reuse the request buffer, events need a new one. */ + if (type != XS_WATCH_EVENT) { + bdata = conn->in; ++ /* Drop asynchronous responses, e.g. errors for watch events. */ ++ if (!bdata) ++ return; + bdata->inhdr = true; + bdata->used = 0; + conn->in = NULL; +diff --git a/tools/xenstore/xenstored_watch.c b/tools/xenstore/xenstored_watch.c +index 71c108ea99..9ff20690c0 100644 +--- tools/xenstore/xenstored_watch.c.orig ++++ tools/xenstore/xenstored_watch.c +@@ -92,6 +92,10 @@ static void add_event(struct connection *conn, + } + + len = strlen(name) + 1 + strlen(watch->token) + 1; ++ /* Don't try to send over-long events. */ ++ if (len > XENSTORE_PAYLOAD_MAX) ++ return; ++ + data = talloc_array(ctx, char, len); + if (!data) + return; Index: pkgsrc/sysutils/xentools411/patches/patch-XSA325 diff -u /dev/null pkgsrc/sysutils/xentools411/patches/patch-XSA325:1.1 --- /dev/null Thu Dec 17 16:48:12 2020 +++ pkgsrc/sysutils/xentools411/patches/patch-XSA325 Thu Dec 17 16:48:12 2020 @@ -0,0 +1,194 @@ +$NetBSD: patch-XSA325,v 1.1 2020/12/17 16:48:12 bouyer Exp $ + +From: Harsha Shamsundara Havanur +Subject: tools/xenstore: Preserve bad client until they are destroyed + +XenStored will kill any connection that it thinks has misbehaved, +this is currently happening in two places: + * In `handle_input()` if the sanity check on the ring and the message + fails. + * In `handle_output()` when failing to write the response in the ring. + +As the domain structure is a child of the connection, XenStored will +destroy its view of the domain when killing the connection. This will +result in sending @releaseDomain event to all the watchers. + +As the watch event doesn't carry which domain has been released, +the watcher (such as XenStored) will generally go through the list of +domains registers and check if one of them is shutting down/dying. +In the case of a client misbehaving, the domain will likely to be +running, so no action will be performed. + +When the domain is effectively destroyed, XenStored will not be aware of +the domain anymore. So the watch event is not going to be sent. +By consequence, the watchers of the event will not release mappings +they may have on the domain. This will result in a zombie domain. + +In order to send @releaseDomain event at the correct time, we want +to keep the domain structure until the domain is effectively +shutting-down/dying. + +We also want to keep the connection around so we could possibly revive +the connection in the future. + +A new flag 'is_ignored' is added to mark whether a connection should be +ignored when checking if there are work to do. Additionally any +transactions, watches, buffers associated to the connection will be +freed as you can't do much with them (restarting the connection will +likely need a reset). + +As a side note, when the device model were running in a stubdomain, a +guest would have been able to introduce a use-after-free because there +is two parents for a guest connection. + +This is XSA-325. + +Reported-by: Pawel Wieczorkiewicz +Signed-off-by: Harsha Shamsundara Havanur +Signed-off-by: Julien Grall +Reviewed-by: Juergen Gross +Reviewed-by: Paul Durrant + +diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c +index af3d17004b3f..27d8f15b6b76 100644 +--- tools/xenstore/xenstored_core.c.orig ++++ tools/xenstore/xenstored_core.c +@@ -1355,6 +1355,32 @@ static struct { + [XS_DIRECTORY_PART] = { "DIRECTORY_PART", send_directory_part }, + }; + ++/* ++ * Keep the connection alive but stop processing any new request or sending ++ * reponse. This is to allow sending @releaseDomain watch event at the correct ++ * moment and/or to allow the connection to restart (not yet implemented). ++ * ++ * All watches, transactions, buffers will be freed. ++ */ ++static void ignore_connection(struct connection *conn) ++{ ++ struct buffered_data *out, *tmp; ++ ++ trace("CONN %p ignored\n", conn); ++ ++ conn->is_ignored = true; ++ conn_delete_all_watches(conn); ++ conn_delete_all_transactions(conn); ++ ++ list_for_each_entry_safe(out, tmp, &conn->out_list, list) { ++ list_del(&out->list); ++ talloc_free(out); ++ } ++ ++ talloc_free(conn->in); ++ conn->in = NULL; ++} ++ + static const char *sockmsg_string(enum xsd_sockmsg_type type) + { + if ((unsigned int)type < ARRAY_SIZE(wire_funcs) && wire_funcs[type].str) +@@ -1413,8 +1439,10 @@ static void consider_message(struct connection *conn) + assert(conn->in == NULL); + } + +-/* Errors in reading or allocating here mean we get out of sync, so we +- * drop the whole client connection. */ ++/* ++ * Errors in reading or allocating here means we get out of sync, so we mark ++ * the connection as ignored. ++ */ + static void handle_input(struct connection *conn) + { + int bytes; +@@ -1471,14 +1499,14 @@ static void handle_input(struct connection *conn) + return; + + bad_client: +- /* Kill it. */ +- talloc_free(conn); ++ ignore_connection(conn); + } + + static void handle_output(struct connection *conn) + { ++ /* Ignore the connection if an error occured */ + if (!write_messages(conn)) +- talloc_free(conn); ++ ignore_connection(conn); + } + + struct connection *new_connection(connwritefn_t *write, connreadfn_t *read) +@@ -1494,6 +1522,7 @@ struct connection *new_connection(connwritefn_t *write, connreadfn_t *read) + new->write = write; + new->read = read; + new->can_write = true; ++ new->is_ignored = false; + new->transaction_started = 0; + INIT_LIST_HEAD(&new->out_list); + INIT_LIST_HEAD(&new->watches); +@@ -2186,8 +2215,9 @@ int main(int argc, char *argv[]) + if (fds[conn->pollfd_idx].revents + & ~(POLLIN|POLLOUT)) + talloc_free(conn); +- else if (fds[conn->pollfd_idx].revents +- & POLLIN) ++ else if ((fds[conn->pollfd_idx].revents ++ & POLLIN) && ++ !conn->is_ignored) + handle_input(conn); + } + if (talloc_free(conn) == 0) +@@ -2199,8 +2229,9 @@ int main(int argc, char *argv[]) + if (fds[conn->pollfd_idx].revents + & ~(POLLIN|POLLOUT)) + talloc_free(conn); +- else if (fds[conn->pollfd_idx].revents +- & POLLOUT) ++ else if ((fds[conn->pollfd_idx].revents ++ & POLLOUT) && ++ !conn->is_ignored) + handle_output(conn); + } + if (talloc_free(conn) == 0) +diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h +index eb19b71f5f46..196a6fd2b0be 100644 +--- tools/xenstore/xenstored_core.h.orig ++++ tools/xenstore/xenstored_core.h +@@ -80,6 +80,9 @@ struct connection + /* Is this a read-only connection? */ + bool can_write; + ++ /* Is this connection ignored? */ ++ bool is_ignored; ++ + /* Buffered incoming data. */ + struct buffered_data *in; + +diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c +index dc635e9be30c..d5e1e3e9d42d 100644 +--- tools/xenstore/xenstored_domain.c.orig ++++ tools/xenstore/xenstored_domain.c +@@ -286,6 +286,10 @@ bool domain_can_read(struct connection *conn) + + if (domain_is_unprivileged(conn) && conn->domain->wrl_credit < 0) + return false; ++ ++ if (conn->is_ignored) ++ return false; ++ + return (intf->req_cons != intf->req_prod); + } + +@@ -303,6 +307,10 @@ bool domain_is_unprivileged(struct connection *conn) + bool domain_can_write(struct connection *conn) + { + struct xenstore_domain_interface *intf = conn->domain->interface; ++ ++ if (conn->is_ignored) ++ return false; ++ + return ((intf->rsp_prod - intf->rsp_cons) != XENSTORE_RING_SIZE); + } + +-- +2.17.1 + Index: pkgsrc/sysutils/xentools411/patches/patch-XSA330 diff -u /dev/null pkgsrc/sysutils/xentools411/patches/patch-XSA330:1.1 --- /dev/null Thu Dec 17 16:48:12 2020 +++ pkgsrc/sysutils/xentools411/patches/patch-XSA330 Thu Dec 17 16:48:12 2020 @@ -0,0 +1,68 @@ +$NetBSD: patch-XSA330,v 1.1 2020/12/17 16:48:12 bouyer Exp $ + +From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= +Subject: tools/ocaml/xenstored: delete watch from trie too when resetting + watches +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +c/s f8c72b526129 "oxenstored: implement XS_RESET_WATCHES" from Xen 4.6 +introduced reset watches support in oxenstored by mirroring the change +in cxenstored. + +However the OCaml version has some additional data structures to +optimize watch firing, and just resetting the watches in one of the data +structures creates a security bug where a malicious guest kernel can +exceed its watch quota, driving oxenstored into OOM: + * create watches + * reset watches (this still keeps the watches lingering in another data + structure, using memory) + * create some more watches + * loop until oxenstored dies + +The guest kernel doesn't necessarily have to be malicious to trigger +this: + * if control/platform-feature-xs_reset_watches is set + * the guest kexecs (e.g. because it crashes) + * on boot more watches are set up + * this will slowly "leak" memory for watches in oxenstored, driving it + towards OOM. + +This is XSA-330. + +Fixes: f8c72b526129 ("oxenstored: implement XS_RESET_WATCHES") +Signed-off-by: Edwin Török +Acked-by: Christian Lindig +Reviewed-by: Andrew Cooper + +diff --git a/tools/ocaml/xenstored/connections.ml b/tools/ocaml/xenstored/connections.ml +index 020b875dcd..4e69de1d42 100644 +--- tools/ocaml/xenstored/connections.ml.orig ++++ tools/ocaml/xenstored/connections.ml +@@ -134,6 +134,10 @@ let del_watch cons con path token = + cons.watches <- Trie.set cons.watches key watches; + watch + ++let del_watches cons con = ++ Connection.del_watches con; ++ cons.watches <- Trie.map (del_watches_of_con con) cons.watches ++ + (* path is absolute *) + let fire_watches ?oldroot root cons path recurse = + let key = key_of_path path in +diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml +index 6a998f8764..12ad66fce6 100644 +--- tools/ocaml/xenstored/process.ml.orig ++++ tools/ocaml/xenstored/process.ml +@@ -179,8 +179,8 @@ let do_isintroduced con _t domains _cons data = + if domid = Define.domid_self || Domains.exist domains domid then "T\000" else "F\000" + + (* only in xen >= 4.2 *) +-let do_reset_watches con t domains cons data = +- Connection.del_watches con; ++let do_reset_watches con _t _domains cons _data = ++ Connections.del_watches cons con; + Connection.del_transactions con + + (* only in >= xen3.3 *) Index: pkgsrc/sysutils/xentools411/patches/patch-XSA352 diff -u /dev/null pkgsrc/sysutils/xentools411/patches/patch-XSA352:1.1 --- /dev/null Thu Dec 17 16:48:12 2020 +++ pkgsrc/sysutils/xentools411/patches/patch-XSA352 Thu Dec 17 16:48:12 2020 @@ -0,0 +1,44 @@ +$NetBSD: patch-XSA352,v 1.1 2020/12/17 16:48:12 bouyer Exp $ + +From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= +Subject: tools/ocaml/xenstored: only Dom0 can change node owner +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +Otherwise we can give quota away to another domain, either causing it to run +out of quota, or in case of Dom0 use unbounded amounts of memory and bypass +the quota system entirely. + +This was fixed in the C version of xenstored in 2006 (c/s db34d2aaa5f5, +predating the XSA process by 5 years). + +It was also fixed in the mirage version of xenstore in 2012, with a unit test +demonstrating the vulnerability: + + https://github.com/mirage/ocaml-xenstore/commit/6b91f3ac46b885d0530a51d57a9b3a57d64923a7 + https://github.com/mirage/ocaml-xenstore/commit/22ee5417c90b8fda905c38de0d534506152eace6 + +but possibly without realising that the vulnerability still affected the +in-tree oxenstored (added c/s f44af660412 in 2010). + +This is XSA-352. + +Signed-off-by: Edwin Török +Acked-by: Christian Lindig +Reviewed-by: Andrew Cooper + +diff --git a/tools/ocaml/xenstored/store.ml b/tools/ocaml/xenstored/store.ml +index 3b05128f1b..5f915f2bbe 100644 +--- tools/ocaml/xenstored/store.ml.orig ++++ tools/ocaml/xenstored/store.ml +@@ -407,7 +407,8 @@ let setperms store perm path nperms = + | Some node -> + let old_owner = Node.get_owner node in + let new_owner = Perms.Node.get_owner nperms in +- if not ((old_owner = new_owner) || (Perms.Connection.is_dom0 perm)) then Quota.check store.quota new_owner 0; ++ if not ((old_owner = new_owner) || (Perms.Connection.is_dom0 perm)) then ++ raise Define.Permission_denied; + store.root <- path_setperms store perm path nperms; + Quota.del_entry store.quota old_owner; + Quota.add_entry store.quota new_owner Index: pkgsrc/sysutils/xentools411/patches/patch-XSA353 diff -u /dev/null pkgsrc/sysutils/xentools411/patches/patch-XSA353:1.1 --- /dev/null Thu Dec 17 16:48:12 2020 +++ pkgsrc/sysutils/xentools411/patches/patch-XSA353 Thu Dec 17 16:48:12 2020 @@ -0,0 +1,91 @@ +$NetBSD: patch-XSA353,v 1.1 2020/12/17 16:48:12 bouyer Exp $ + +From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= +Subject: tools/ocaml/xenstored: do permission checks on xenstore root +MIME-Version: 1.0 +Content-Type: text/plain; charset=UTF-8 +Content-Transfer-Encoding: 8bit + +This was lacking in a disappointing number of places. + +The xenstore root node is treated differently from all other nodes, because it +doesn't have a parent, and mutation requires changing the parent. + +Unfortunately this lead to open-coding the special case for root into every +single xenstore operation, and out of all the xenstore operations only read +did a permission check when handling the root node. + +This means that an unprivileged guest can: + + * xenstore-chmod / to its liking and subsequently write new arbitrary nodes + there (subject to quota) + * xenstore-rm -r / deletes almost the entire xenstore tree (xenopsd quickly + refills some, but you are left with a broken system) + * DIRECTORY on / lists all children when called through python + bindings (xenstore-ls stops at /local because it tries to list recursively) + * get-perms on / works too, but that is just a minor information leak + +Add the missing permission checks, but this should really be refactored to do +the root handling and permission checks on the node only once from a single +function, instead of getting it wrong nearly everywhere. + +This is XSA-353. + +Signed-off-by: Edwin Török +Acked-by: Christian Lindig +Reviewed-by: Andrew Cooper + +diff --git a/tools/ocaml/xenstored/store.ml b/tools/ocaml/xenstored/store.ml +index f299ec6461..92b6289b5e 100644 +--- tools/ocaml/xenstored/store.ml.orig ++++ tools/ocaml/xenstored/store.ml +@@ -273,15 +273,17 @@ let path_rm store perm path = + Node.del_childname node name + with Not_found -> + raise Define.Doesnt_exist in +- if path = [] then ++ if path = [] then ( ++ Node.check_perm store.root perm Perms.WRITE; + Node.del_all_children store.root +- else ++ ) else + Path.apply_modify store.root path do_rm + + let path_setperms store perm path perms = +- if path = [] then ++ if path = [] then ( ++ Node.check_perm store.root perm Perms.WRITE; + Node.set_perms store.root perms +- else ++ ) else + let do_setperms node name = + let c = Node.find node name in + Node.check_owner c perm; +@@ -313,9 +315,10 @@ let read store perm path = + + let ls store perm path = + let children = +- if path = [] then +- (Node.get_children store.root) +- else ++ if path = [] then ( ++ Node.check_perm store.root perm Perms.READ; ++ Node.get_children store.root ++ ) else + let do_ls node name = + let cnode = Node.find node name in + Node.check_perm cnode perm Perms.READ; +@@ -324,9 +327,10 @@ let ls store perm path = + List.rev (List.map (fun n -> Symbol.to_string n.Node.name) children) + + let getperms store perm path = +- if path = [] then +- (Node.get_perms store.root) +- else ++ if path = [] then ( ++ Node.check_perm store.root perm Perms.READ; ++ Node.get_perms store.root ++ ) else + let fct n name = + let c = Node.find n name in + Node.check_perm c perm Perms.READ; --_----------=_160822369248150--