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 <jgross@suse.com>
[julieng: Handle rebase conflict]
Reviewed-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Paul Durrant <paul@xen.org>

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index 0308b57ff70600e4517e8d79b6b2fc8b39dd6389..2a86c4aa5bcee170f7dd7d13eb23a3d853c3318e 100644 (file)
--- a/tools/xenstore/xenstored_core.c
+++ b/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 2092067502aaa61a80df552f07baf1fa45bc2960..e4966c89e3be0cc79733edda8482ee8258e3e60e 100644 (file)
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -154,15 +154,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);
@@ -173,6 +175,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 8888db697edbca15bd96b60a96ebc81261fa14e3..0b2f49ac7d4c36e641c8838e2733c6541bb6f9b7 100644 (file)
--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -214,7 +214,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);
 
@@ -252,7 +252,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. */
@@ -418,7 +418,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 e5054294f5d0b653fa83b367db747d891f60be1c..36793b9b1af3622ec5364115d37cf71dff3888d4 100644 (file)
--- a/tools/xenstore/xenstored_transaction.c
+++ b/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 fc7e5ce3cb14ad550b791df12576b281d17fcb8b..be2479721fc9c886fe78a233df97dac5d23927a1 100644 (file)
--- a/tools/xenstore/xenstored_watch.c
+++ b/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 1b3c80d3dda110f99f5173c958e44ecebd825727..03094374f3793532ff1455498ea2259403020f05 100644 (file)
--- a/tools/xenstore/xenstored_watch.h
+++ b/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);