There is one corner case of XSA-417 which wasn't handled completely with the patches back then. The XSA-417 fixes tried to solve the problem, that a new domU would inherit access permissions to access Xenstore entries with that domid listed in the access rights. In order not to make it easy for a domU to query existence of a domid, adding permission for a non-existing domain is not rejected by Xenstore. The XSA-417 patches solved that problem by adding a flag to a permission entry referencing a not existing domain, indicating that the permission should not be effective for Xenstore. One corner case was not handled: Consider guest 1 and guest 2 running. Guest 1 adds guest 2 to be able to access a Xenstore entry. Now guest 2 is removed from the system and a new guest 3 with the same domid as guest 2 had is being created. When guest 3 would try now to access the Xenstore entry, it would fail, as Xenstore would see that the Xenstore entry is older than guest 3. But if guest 1 is modifying the permissions of the Xenstore entry again, e.g. by adding another domain, the permission entry for guest 2 would lose its "special flag", resulting in guest 3 now really gaining access to the Xenstore entry. This series is fixing this problem by the following means: - In order to allow guests to know that a Xenstore entry permission might have gone stale, allow unprivileged guests to receive @releaseDomain watch events. This doesn't open a security hole, as the only knowledge which can by gathered from that change is that a domain is gone, not that a domain with a specific domid is existing. - When a domain is removed, remove all permissions relating to this domain from all Xenstore entries. Note that this issue was discussed by the Xen security team and we decided not to issue an XSA, as there are no known use cases where one unprivileged guest would grant access to its Xenstore nodes to more than one other unprivileged guests. We decided to delay this patch series until the watch depth feature has been committed, as with that feature available it is now possible for a guest to handle the death of a specific domain in a sane way. Juergen Gross (4): xen/public: introduce DOMID_ANY tools/xenstored: add support for "all domains" node permission tools/xenstored: allow @releaseDomain watch for all domains tools/xenstored: remove permissions related to dead domain docs/man/xl.cfg.5.pod.in | 4 ++ tools/xenstored/core.c | 45 ++++++++++++++----- tools/xenstored/domain.c | 77 +++++++++++++++++++++------------ tools/xenstored/domain.h | 3 +- xen/include/public/io/xs_wire.h | 2 + xen/include/public/xen.h | 7 +++ 6 files changed, 99 insertions(+), 39 deletions(-) -- 2.53.0 Add DOMID_ANY to xen/include/public/xen.h meant to be a wildcard for domids. Signed-off-by: Denis Mukhin <dmukhin@ford.com> Signed-off-by: Juergen Gross <jgross@suse.com> --- This is based on Denis Mukhin's patch "xen/domain: introduce DOMID_ANY". As my series is another use case for DOMID_ANY and it is a backport candidate, I've split out the definition of DOMID_ANY from Denis' patch in order to make progress for my series. --- xen/include/public/xen.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h index XXXXXXX..XXXXXXX 100644 --- a/xen/include/public/xen.h +++ b/xen/include/public/xen.h @@ -XXX,XX +XXX,XX @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t); /* DOMID_INVALID is used to identify pages with unknown owner. */ #define DOMID_INVALID xen_mk_uint(0x7FF4) +/* + * DOMID_ANY is used to signal no specific domain ID requested. + * Handler should pick a valid ID, or handle it as a broadcast value + * depending on the context. + */ +#define DOMID_ANY xen_mk_uint(0x7FF5) + /* Idle domain. */ #define DOMID_IDLE xen_mk_uint(0x7FFF) -- 2.53.0 Add support for using DOMID_ANY in node permissions to indicate that all domains are allowed to access the node. Add a new feature bit for indicating the support of DOMID_ANY. Signed-off-by: Juergen Gross <jgross@suse.com> --- docs/man/xl.cfg.5.pod.in | 4 ++++ tools/xenstored/core.c | 19 ++++++++++++++----- tools/xenstored/domain.c | 16 ++++++++++++++-- tools/xenstored/domain.h | 3 ++- xen/include/public/io/xs_wire.h | 2 ++ 5 files changed, 36 insertions(+), 8 deletions(-) diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index XXXXXXX..XXXXXXX 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -XXX,XX +XXX,XX @@ Xenstore supports to set watches with a limited depth (depth 0 matches only the watched node, depth 1 matches the node and its direct children, etc.). +=item B<0x00000008> + +Xenstore supports the B<all domains> node access permission. + =back The features supported by the running Xenstore instance can be retrieved diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c index XXXXXXX..XXXXXXX 100644 --- a/tools/xenstored/core.c +++ b/tools/xenstored/core.c @@ -XXX,XX +XXX,XX @@ static int write_node(struct connection *conn, struct node *node, return ret; } +/* Check one node permission to match a connection. */ +static bool perm_allows_conn(const struct connection *conn, + const struct xs_permissions *p) +{ + if (p->id == conn->id || (conn->target && p->id == conn->target->id)) + return true; + + return p->id == DOMID_ANY; +} + unsigned int perm_for_conn(struct connection *conn, const struct node_perms *perms) { @@ -XXX,XX +XXX,XX @@ unsigned int perm_for_conn(struct connection *conn, unsigned int mask = XS_PERM_READ|XS_PERM_WRITE|XS_PERM_OWNER; /* Owners and tools get it all... */ - if (!domain_is_unprivileged(conn) || perms->p[0].id == conn->id - || (conn->target && perms->p[0].id == conn->target->id)) + if (!domain_is_unprivileged(conn) || + perm_allows_conn(conn, perms->p)) return (XS_PERM_READ|XS_PERM_WRITE|XS_PERM_OWNER) & mask; for (i = 1; i < perms->num; i++) if (!(perms->p[i].perms & XS_PERM_IGNORE) && - (perms->p[i].id == conn->id || - (conn->target && perms->p[i].id == conn->target->id))) + perm_allows_conn(conn, perms->p + i)) return perms->p[i].perms & mask; return perms->p[0].perms & mask; @@ -XXX,XX +XXX,XX @@ static int do_set_perms(const void *ctx, struct connection *conn, if (!xenstore_strings_to_perms(perms.p, perms.num, permstr)) return errno; - if (domain_alloc_permrefs(&perms)) + if (domain_alloc_permrefs(conn, &perms)) return ENOMEM; if (perms.p[0].perms & XS_PERM_IGNORE) return ENOENT; diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c index XXXXXXX..XXXXXXX 100644 --- a/tools/xenstored/domain.c +++ b/tools/xenstored/domain.c @@ -XXX,XX +XXX,XX @@ #endif #define XENSTORE_FEATURES (XENSTORE_SERVER_FEATURE_ERROR | \ - XENSTORE_SERVER_FEATURE_WATCHDEPTH) + XENSTORE_SERVER_FEATURE_WATCHDEPTH | \ + XENSTORE_SERVER_FEATURE_DOMID_ANY) static xenmanage_handle *xm_handle; xengnttab_handle **xgt_handle; @@ -XXX,XX +XXX,XX @@ static bool chk_domain_generation(unsigned int domid, uint64_t gen) * Allocate all missing struct domain referenced by a permission set. * Any permission entries for not existing domains will be marked to be * ignored. + * An DOMID_ANY entry will be marked to be ignored, if the writing + * domain doesn't have the XENSTORE_SERVER_FEATURE_DOMID_ANY enabled. Note + * that Xen tools will never set DOMID_ANY for a guest owned node. */ -int domain_alloc_permrefs(struct node_perms *perms) +int domain_alloc_permrefs(const struct connection *conn, + struct node_perms *perms) { unsigned int i, domid; struct domain *d; @@ -XXX,XX +XXX,XX @@ int domain_alloc_permrefs(struct node_perms *perms) for (i = 0; i < perms->num; i++) { domid = perms->p[i].id; + if (domid == DOMID_ANY) { + if (!(conn->domain->features & + XENSTORE_SERVER_FEATURE_DOMID_ANY)) + perms->p[i].perms |= XS_PERM_IGNORE; + continue; + } d = find_domain_struct(domid); if (!d) { if (xenmanage_get_domain_info(xm_handle, domid, NULL, @@ -XXX,XX +XXX,XX @@ int domain_adjust_node_perms(struct node *node) for (i = 1; i < node->hdr.num_perms; i++) { if ((perms[i].perms & XS_PERM_IGNORE) || + perms[i].id == DOMID_ANY || chk_domain_generation(perms[i].id, node->hdr.generation)) continue; diff --git a/tools/xenstored/domain.h b/tools/xenstored/domain.h index XXXXXXX..XXXXXXX 100644 --- a/tools/xenstored/domain.h +++ b/tools/xenstored/domain.h @@ -XXX,XX +XXX,XX @@ const char *get_implicit_path(const struct connection *conn); */ int domain_adjust_node_perms(struct node *node); -int domain_alloc_permrefs(struct node_perms *perms); +int domain_alloc_permrefs(const struct connection *conn, + struct node_perms *perms); /* Quota manipulation */ int domain_nbentry_inc(struct connection *conn, unsigned int domid); diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h index XXXXXXX..XXXXXXX 100644 --- a/xen/include/public/io/xs_wire.h +++ b/xen/include/public/io/xs_wire.h @@ -XXX,XX +XXX,XX @@ struct xenstore_domain_interface { #define XENSTORE_SERVER_FEATURE_ERROR 2 /* The XS_WATCH command can be used with a <depth> parameter */ #define XENSTORE_SERVER_FEATURE_WATCHDEPTH 4 +/* The capability to use DOMID_ANY for node permissions */ +#define XENSTORE_SERVER_FEATURE_DOMID_ANY 8 /* Valid values for the connection field */ #define XENSTORE_CONNECTED 0 /* the steady-state */ -- 2.53.0 Currently the @releaseDomain watch is allowed for dom0 only. This is problematic for guests which want to give other domains access to Xenstore entries, as they have no simple way to tell when such a domain is stopped. Allow @releaseDomain to be usable by all domains as the default. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenstored/core.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c index XXXXXXX..XXXXXXX 100644 --- a/tools/xenstored/core.c +++ b/tools/xenstored/core.c @@ -XXX,XX +XXX,XX @@ struct connection *get_connection_by_id(unsigned int conn_id) } /* We create initial nodes manually. */ -static void manual_node(const char *name, const char *child) +static void manual_node_perms(const char *name, const char *child, + struct xs_permissions *perms, + unsigned int n_perms) { struct node *node; - struct xs_permissions perms = { .id = priv_domid, - .perms = XS_PERM_NONE }; node = talloc_zero(NULL, struct node); if (!node) barf_perror("Could not allocate initial node %s", name); node->name = name; - node->perms = &perms; - node->hdr.num_perms = 1; + node->perms = perms; + node->hdr.num_perms = n_perms; node->children = (char *)child; if (child) node->hdr.childlen = strlen(child) + 1; @@ -XXX,XX +XXX,XX @@ static void manual_node(const char *name, const char *child) talloc_free(node); } +static void manual_node(const char *name, const char *child) +{ + struct xs_permissions perms = { .id = priv_domid, + .perms = XS_PERM_NONE }; + + manual_node_perms(name, child, &perms, 1); +} + static unsigned int hash_from_key_fn(const void *k) { const char *str = k; @@ -XXX,XX +XXX,XX @@ static int keys_equal_fn(const void *key1, const void *key2) void setup_structure(bool live_update) { + struct xs_permissions perms[] = { + { .id = priv_domid, .perms = XS_PERM_NONE }, + { .id = DOMID_ANY, .perms = XS_PERM_READ }, + }; + nodes = create_hashtable(NULL, "nodes", hash_from_key_fn, keys_equal_fn, HASHTABLE_FREE_KEY | HASHTABLE_FREE_VALUE); if (!nodes) @@ -XXX,XX +XXX,XX @@ void setup_structure(bool live_update) manual_node("/", "tool"); manual_node("/tool", "xenstored"); manual_node("/tool/xenstored", NULL); - manual_node("@releaseDomain", NULL); + manual_node_perms("@releaseDomain", NULL, + perms, ARRAY_SIZE(perms)); manual_node("@introduceDomain", NULL); domain_nbentry_fix(priv_domid, 5); } -- 2.53.0 Wit unprivileged domains now capable to use the @releaseDomain watch, there is no reason not to remove any node permissions which relate to a domain which has been removed. This resolves a complex scenario where a new domain could inherit the permissions of an old one with the same domid. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenstored/domain.c | 61 ++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 25 deletions(-) diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c index XXXXXXX..XXXXXXX 100644 --- a/tools/xenstored/domain.c +++ b/tools/xenstored/domain.c @@ -XXX,XX +XXX,XX @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn, struct node *node, void *arg) { struct domain *domain = arg; - int ret = WALK_TREE_OK; - - if (node->perms[0].id != domain->domid) - return WALK_TREE_OK; + bool node_changed = false; + unsigned int i; - if (keep_orphans) { - domain_nbentry_dec(NULL, domain->domid); - node->perms[0].id = priv_domid; - node->acc.memory = 0; - domain_nbentry_inc(NULL, priv_domid); - if (write_node_raw(NULL, node->name, node, NODE_MODIFY, true)) { - /* That's unfortunate. We only can try to continue. */ - syslog(LOG_ERR, - "error when moving orphaned node %s to dom0\n", - node->name); - } else - trace("orphaned node %s moved to dom0\n", node->name); - } else { + if (node->perms[0].id == domain->domid && !keep_orphans) { if (rm_node(NULL, ctx, node->name)) { /* That's unfortunate. We only can try to continue. */ syslog(LOG_ERR, @@ -XXX,XX +XXX,XX @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn, trace("orphaned node %s deleted\n", node->name); /* Skip children in all cases in order to avoid more errors. */ - ret = WALK_TREE_SKIP_CHILDREN; + return WALK_TREE_SKIP_CHILDREN; } - return domain->acc_val[ACC_NODES] ? ret : WALK_TREE_SUCCESS_STOP; + if (node->perms[0].id == domain->domid) { + domain_nbentry_dec(NULL, domain->domid); + node->perms[0].id = priv_domid; + node->acc.memory = 0; + domain_nbentry_inc(NULL, priv_domid); + trace("moving orphaned node %s to dom0\n", node->name); + node_changed = true; + } + + for (i = 1; i < node->hdr.num_perms; i++) { + if (node->perms[i].id != domain->domid) + continue; + memmove(node->perms + i, node->perms + i + 1, + sizeof(*node->perms) * (node->hdr.num_perms - i - 1)); + node->hdr.num_perms--; + i--; + node_changed = true; + } + + if (node_changed) { + if (write_node_raw(NULL, node->name, node, NODE_MODIFY, true)) { + /* That's unfortunate. We only can try to continue. */ + syslog(LOG_ERR, + "error when writing modified node %s\n", + node->name); + } + } + + return WALK_TREE_OK; } static void domain_tree_remove(struct domain *domain) @@ -XXX,XX +XXX,XX @@ static void domain_tree_remove(struct domain *domain) int ret; struct walk_funcs walkfuncs = { .enter = domain_tree_remove_sub }; - if (domain->acc_val[ACC_NODES]) { - ret = walk_node_tree(domain, NULL, "/", &walkfuncs, domain); - if (ret == WALK_TREE_ERROR_STOP) - syslog(LOG_ERR, - "error when looking for orphaned nodes\n"); - } + ret = walk_node_tree(domain, NULL, "/", &walkfuncs, domain); + if (ret == WALK_TREE_ERROR_STOP) + syslog(LOG_ERR, "error when looking for orphaned nodes\n"); walk_node_tree(domain, NULL, "@releaseDomain", &walkfuncs, domain); walk_node_tree(domain, NULL, "@introduceDomain", &walkfuncs, domain); -- 2.53.0 There is one corner case of XSA-417 which wasn't handled completely with the patches back then. The XSA-417 fixes tried to solve the problem, that a new domU would inherit access permissions to access Xenstore entries with that domid listed in the access rights. In order not to make it easy for a domU to query existence of a domid, adding permission for a non-existing domain is not rejected by Xenstore. The XSA-417 patches solved that problem by adding a flag to a permission entry referencing a not existing domain, indicating that the permission should not be effective for Xenstore. One corner case was not handled: Consider guest 1 and guest 2 running. Guest 1 adds guest 2 to be able to access a Xenstore entry. Now guest 2 is removed from the system and a new guest 3 with the same domid as guest 2 had is being created. When guest 3 would try now to access the Xenstore entry, it would fail, as Xenstore would see that the Xenstore entry is older than guest 3. But if guest 1 is modifying the permissions of the Xenstore entry again, e.g. by adding another domain, the permission entry for guest 2 would lose its "special flag", resulting in guest 3 now really gaining access to the Xenstore entry. This series is fixing this problem by the following means: - In order to allow guests to know that a Xenstore entry permission might have gone stale, allow unprivileged guests to receive @releaseDomain watch events. This doesn't open a security hole, as the only knowledge which can by gathered from that change is that a domain is gone, not that a domain with a specific domid is existing. - When a domain is removed, remove all permissions relating to this domain from all Xenstore entries. Note that this issue was discussed by the Xen security team and we decided not to issue an XSA, as there are no known use cases where one unprivileged guest would grant access to its Xenstore nodes to more than one other unprivileged guests. We decided to delay this patch series until the watch depth feature has been committed, as with that feature available it is now possible for a guest to handle the death of a specific domain in a sane way. Changes in V2: - some minor comments addressed Denis Mukhin (1): xen/public: introduce DOMID_ANY Juergen Gross (3): tools/xenstored: add support for "all domains" node permission tools/xenstored: allow @releaseDomain watch for all domains tools/xenstored: remove permissions related to dead domain docs/man/xl.cfg.5.pod.in | 4 ++ tools/xenstored/core.c | 45 ++++++++++++++----- tools/xenstored/domain.c | 78 +++++++++++++++++++++------------ tools/xenstored/domain.h | 3 +- xen/include/public/io/xs_wire.h | 2 + xen/include/public/xen.h | 7 +++ 6 files changed, 100 insertions(+), 39 deletions(-) -- 2.53.0 From: Denis Mukhin <dmukhin@ford.com> Add DOMID_ANY to xen/include/public/xen.h meant to be a wildcard for domids. Signed-off-by: Denis Mukhin <dmukhin@ford.com> Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> --- This is based on Denis Mukhin's patch "xen/domain: introduce DOMID_ANY". As my series is another use case for DOMID_ANY and it is a backport candidate, I've split out the definition of DOMID_ANY from Denis' patch in order to make progress for my series. V2: update comment (Jason Andryuk) --- xen/include/public/xen.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h index XXXXXXX..XXXXXXX 100644 --- a/xen/include/public/xen.h +++ b/xen/include/public/xen.h @@ -XXX,XX +XXX,XX @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t); /* DOMID_INVALID is used to identify pages with unknown owner. */ #define DOMID_INVALID xen_mk_uint(0x7FF4) +/* + * DOMID_ANY is used to signal no specific domain ID requested. + * Handler should pick a valid ID, or handle it as a wildcard value + * depending on the context. + */ +#define DOMID_ANY xen_mk_uint(0x7FF5) + /* Idle domain. */ #define DOMID_IDLE xen_mk_uint(0x7FFF) -- 2.53.0 Add support for using DOMID_ANY in node permissions to indicate that all domains are allowed to access the node. Add a new feature bit for indicating the support of DOMID_ANY. Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> --- docs/man/xl.cfg.5.pod.in | 4 ++++ tools/xenstored/core.c | 19 ++++++++++++++----- tools/xenstored/domain.c | 16 ++++++++++++++-- tools/xenstored/domain.h | 3 ++- xen/include/public/io/xs_wire.h | 2 ++ 5 files changed, 36 insertions(+), 8 deletions(-) diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in index XXXXXXX..XXXXXXX 100644 --- a/docs/man/xl.cfg.5.pod.in +++ b/docs/man/xl.cfg.5.pod.in @@ -XXX,XX +XXX,XX @@ Xenstore supports to set watches with a limited depth (depth 0 matches only the watched node, depth 1 matches the node and its direct children, etc.). +=item B<0x00000008> + +Xenstore supports the B<all domains> node access permission. + =back The features supported by the running Xenstore instance can be retrieved diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c index XXXXXXX..XXXXXXX 100644 --- a/tools/xenstored/core.c +++ b/tools/xenstored/core.c @@ -XXX,XX +XXX,XX @@ static int write_node(struct connection *conn, struct node *node, return ret; } +/* Check one node permission to match a connection. */ +static bool perm_allows_conn(const struct connection *conn, + const struct xs_permissions *p) +{ + if (p->id == conn->id || (conn->target && p->id == conn->target->id)) + return true; + + return p->id == DOMID_ANY; +} + unsigned int perm_for_conn(struct connection *conn, const struct node_perms *perms) { @@ -XXX,XX +XXX,XX @@ unsigned int perm_for_conn(struct connection *conn, unsigned int mask = XS_PERM_READ|XS_PERM_WRITE|XS_PERM_OWNER; /* Owners and tools get it all... */ - if (!domain_is_unprivileged(conn) || perms->p[0].id == conn->id - || (conn->target && perms->p[0].id == conn->target->id)) + if (!domain_is_unprivileged(conn) || + perm_allows_conn(conn, perms->p)) return (XS_PERM_READ|XS_PERM_WRITE|XS_PERM_OWNER) & mask; for (i = 1; i < perms->num; i++) if (!(perms->p[i].perms & XS_PERM_IGNORE) && - (perms->p[i].id == conn->id || - (conn->target && perms->p[i].id == conn->target->id))) + perm_allows_conn(conn, perms->p + i)) return perms->p[i].perms & mask; return perms->p[0].perms & mask; @@ -XXX,XX +XXX,XX @@ static int do_set_perms(const void *ctx, struct connection *conn, if (!xenstore_strings_to_perms(perms.p, perms.num, permstr)) return errno; - if (domain_alloc_permrefs(&perms)) + if (domain_alloc_permrefs(conn, &perms)) return ENOMEM; if (perms.p[0].perms & XS_PERM_IGNORE) return ENOENT; diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c index XXXXXXX..XXXXXXX 100644 --- a/tools/xenstored/domain.c +++ b/tools/xenstored/domain.c @@ -XXX,XX +XXX,XX @@ #endif #define XENSTORE_FEATURES (XENSTORE_SERVER_FEATURE_ERROR | \ - XENSTORE_SERVER_FEATURE_WATCHDEPTH) + XENSTORE_SERVER_FEATURE_WATCHDEPTH | \ + XENSTORE_SERVER_FEATURE_DOMID_ANY) static xenmanage_handle *xm_handle; xengnttab_handle **xgt_handle; @@ -XXX,XX +XXX,XX @@ static bool chk_domain_generation(unsigned int domid, uint64_t gen) * Allocate all missing struct domain referenced by a permission set. * Any permission entries for not existing domains will be marked to be * ignored. + * A DOMID_ANY entry will be marked to be ignored, if the writing + * domain doesn't have the XENSTORE_SERVER_FEATURE_DOMID_ANY enabled. Note + * that Xen tools will never set DOMID_ANY for a guest owned node. */ -int domain_alloc_permrefs(struct node_perms *perms) +int domain_alloc_permrefs(const struct connection *conn, + struct node_perms *perms) { unsigned int i, domid; struct domain *d; @@ -XXX,XX +XXX,XX @@ int domain_alloc_permrefs(struct node_perms *perms) for (i = 0; i < perms->num; i++) { domid = perms->p[i].id; + if (domid == DOMID_ANY) { + if (!(conn->domain->features & + XENSTORE_SERVER_FEATURE_DOMID_ANY)) + perms->p[i].perms |= XS_PERM_IGNORE; + continue; + } d = find_domain_struct(domid); if (!d) { if (xenmanage_get_domain_info(xm_handle, domid, NULL, @@ -XXX,XX +XXX,XX @@ int domain_adjust_node_perms(struct node *node) for (i = 1; i < node->hdr.num_perms; i++) { if ((perms[i].perms & XS_PERM_IGNORE) || + perms[i].id == DOMID_ANY || chk_domain_generation(perms[i].id, node->hdr.generation)) continue; diff --git a/tools/xenstored/domain.h b/tools/xenstored/domain.h index XXXXXXX..XXXXXXX 100644 --- a/tools/xenstored/domain.h +++ b/tools/xenstored/domain.h @@ -XXX,XX +XXX,XX @@ const char *get_implicit_path(const struct connection *conn); */ int domain_adjust_node_perms(struct node *node); -int domain_alloc_permrefs(struct node_perms *perms); +int domain_alloc_permrefs(const struct connection *conn, + struct node_perms *perms); /* Quota manipulation */ int domain_nbentry_inc(struct connection *conn, unsigned int domid); diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h index XXXXXXX..XXXXXXX 100644 --- a/xen/include/public/io/xs_wire.h +++ b/xen/include/public/io/xs_wire.h @@ -XXX,XX +XXX,XX @@ struct xenstore_domain_interface { #define XENSTORE_SERVER_FEATURE_ERROR 2 /* The XS_WATCH command can be used with a <depth> parameter */ #define XENSTORE_SERVER_FEATURE_WATCHDEPTH 4 +/* The capability to use DOMID_ANY for node permissions */ +#define XENSTORE_SERVER_FEATURE_DOMID_ANY 8 /* Valid values for the connection field */ #define XENSTORE_CONNECTED 0 /* the steady-state */ -- 2.53.0 Currently the @releaseDomain watch is allowed for dom0 only. This is problematic for guests which want to give other domains access to Xenstore entries, as they have no simple way to tell when such a domain is stopped. Allow @releaseDomain to be usable by all domains as the default. Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> --- tools/xenstored/core.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c index XXXXXXX..XXXXXXX 100644 --- a/tools/xenstored/core.c +++ b/tools/xenstored/core.c @@ -XXX,XX +XXX,XX @@ struct connection *get_connection_by_id(unsigned int conn_id) } /* We create initial nodes manually. */ -static void manual_node(const char *name, const char *child) +static void manual_node_perms(const char *name, const char *child, + struct xs_permissions *perms, + unsigned int n_perms) { struct node *node; - struct xs_permissions perms = { .id = priv_domid, - .perms = XS_PERM_NONE }; node = talloc_zero(NULL, struct node); if (!node) barf_perror("Could not allocate initial node %s", name); node->name = name; - node->perms = &perms; - node->hdr.num_perms = 1; + node->perms = perms; + node->hdr.num_perms = n_perms; node->children = (char *)child; if (child) node->hdr.childlen = strlen(child) + 1; @@ -XXX,XX +XXX,XX @@ static void manual_node(const char *name, const char *child) talloc_free(node); } +static void manual_node(const char *name, const char *child) +{ + struct xs_permissions perms = { .id = priv_domid, + .perms = XS_PERM_NONE }; + + manual_node_perms(name, child, &perms, 1); +} + static unsigned int hash_from_key_fn(const void *k) { const char *str = k; @@ -XXX,XX +XXX,XX @@ static int keys_equal_fn(const void *key1, const void *key2) void setup_structure(bool live_update) { + struct xs_permissions perms[] = { + { .id = priv_domid, .perms = XS_PERM_NONE }, + { .id = DOMID_ANY, .perms = XS_PERM_READ }, + }; + nodes = create_hashtable(NULL, "nodes", hash_from_key_fn, keys_equal_fn, HASHTABLE_FREE_KEY | HASHTABLE_FREE_VALUE); if (!nodes) @@ -XXX,XX +XXX,XX @@ void setup_structure(bool live_update) manual_node("/", "tool"); manual_node("/tool", "xenstored"); manual_node("/tool/xenstored", NULL); - manual_node("@releaseDomain", NULL); + manual_node_perms("@releaseDomain", NULL, + perms, ARRAY_SIZE(perms)); manual_node("@introduceDomain", NULL); domain_nbentry_fix(priv_domid, 5); } -- 2.53.0 Wit unprivileged domains now capable to use the @releaseDomain watch, there is no reason not to remove any node permissions which relate to a domain which has been removed. This resolves a complex scenario where a new domain could inherit the permissions of an old one with the same domid. Signed-off-by: Juergen Gross <jgross@suse.com> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> --- V2: use priv_domid instead of literal 0 in message (Jason Andryuk) --- tools/xenstored/domain.c | 62 ++++++++++++++++++++++++---------------- 1 file changed, 37 insertions(+), 25 deletions(-) diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c index XXXXXXX..XXXXXXX 100644 --- a/tools/xenstored/domain.c +++ b/tools/xenstored/domain.c @@ -XXX,XX +XXX,XX @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn, struct node *node, void *arg) { struct domain *domain = arg; - int ret = WALK_TREE_OK; - - if (node->perms[0].id != domain->domid) - return WALK_TREE_OK; + bool node_changed = false; + unsigned int i; - if (keep_orphans) { - domain_nbentry_dec(NULL, domain->domid); - node->perms[0].id = priv_domid; - node->acc.memory = 0; - domain_nbentry_inc(NULL, priv_domid); - if (write_node_raw(NULL, node->name, node, NODE_MODIFY, true)) { - /* That's unfortunate. We only can try to continue. */ - syslog(LOG_ERR, - "error when moving orphaned node %s to dom0\n", - node->name); - } else - trace("orphaned node %s moved to dom0\n", node->name); - } else { + if (node->perms[0].id == domain->domid && !keep_orphans) { if (rm_node(NULL, ctx, node->name)) { /* That's unfortunate. We only can try to continue. */ syslog(LOG_ERR, @@ -XXX,XX +XXX,XX @@ static int domain_tree_remove_sub(const void *ctx, struct connection *conn, trace("orphaned node %s deleted\n", node->name); /* Skip children in all cases in order to avoid more errors. */ - ret = WALK_TREE_SKIP_CHILDREN; + return WALK_TREE_SKIP_CHILDREN; } - return domain->acc_val[ACC_NODES] ? ret : WALK_TREE_SUCCESS_STOP; + if (node->perms[0].id == domain->domid) { + domain_nbentry_dec(NULL, domain->domid); + node->perms[0].id = priv_domid; + node->acc.memory = 0; + domain_nbentry_inc(NULL, priv_domid); + trace("moving orphaned node %s to dom%d\n", node->name, + priv_domid); + node_changed = true; + } + + for (i = 1; i < node->hdr.num_perms; i++) { + if (node->perms[i].id != domain->domid) + continue; + memmove(node->perms + i, node->perms + i + 1, + sizeof(*node->perms) * (node->hdr.num_perms - i - 1)); + node->hdr.num_perms--; + i--; + node_changed = true; + } + + if (node_changed) { + if (write_node_raw(NULL, node->name, node, NODE_MODIFY, true)) { + /* That's unfortunate. We only can try to continue. */ + syslog(LOG_ERR, + "error when writing modified node %s\n", + node->name); + } + } + + return WALK_TREE_OK; } static void domain_tree_remove(struct domain *domain) @@ -XXX,XX +XXX,XX @@ static void domain_tree_remove(struct domain *domain) int ret; struct walk_funcs walkfuncs = { .enter = domain_tree_remove_sub }; - if (domain->acc_val[ACC_NODES]) { - ret = walk_node_tree(domain, NULL, "/", &walkfuncs, domain); - if (ret == WALK_TREE_ERROR_STOP) - syslog(LOG_ERR, - "error when looking for orphaned nodes\n"); - } + ret = walk_node_tree(domain, NULL, "/", &walkfuncs, domain); + if (ret == WALK_TREE_ERROR_STOP) + syslog(LOG_ERR, "error when looking for orphaned nodes\n"); walk_node_tree(domain, NULL, "@releaseDomain", &walkfuncs, domain); walk_node_tree(domain, NULL, "@introduceDomain", &walkfuncs, domain); -- 2.53.0