[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[xen staging] tools/xenstore: Preserve bad client until they are destroyed



commit d2fa370d3ef9cbe22d7256c608671cdcdf6e0083
Author:     Harsha Shamsundara Havanur <havanur@xxxxxxxxxx>
AuthorDate: Tue Dec 15 13:36:45 2020 +0100
Commit:     Jan Beulich <jbeulich@xxxxxxxx>
CommitDate: Tue Dec 15 13:36:45 2020 +0100

    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 <wipawel@xxxxxxxxx>
    Signed-off-by: Harsha Shamsundara Havanur <havanur@xxxxxxxxxx>
    Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
    Reviewed-by: Juergen Gross <jgross@xxxxxxxx>
    Reviewed-by: Paul Durrant <paul@xxxxxxx>
---
 tools/xenstore/xenstored_core.c   | 49 ++++++++++++++++++++++++++++++++-------
 tools/xenstore/xenstored_core.h   |  3 +++
 tools/xenstore/xenstored_domain.c |  8 +++++++
 3 files changed, 51 insertions(+), 9 deletions(-)

diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c
index c929cbbc3b..746a1247b3 100644
--- a/tools/xenstore/xenstored_core.c
+++ b/tools/xenstore/xenstored_core.c
@@ -1337,6 +1337,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)
@@ -1395,8 +1421,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;
@@ -1453,14 +1481,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)
@@ -1475,6 +1503,7 @@ struct connection *new_connection(connwritefn_t *write, 
connreadfn_t *read)
        new->pollfd_idx = -1;
        new->write = write;
        new->read = read;
+       new->is_ignored = false;
        new->transaction_started = 0;
        INIT_LIST_HEAD(&new->out_list);
        INIT_LIST_HEAD(&new->watches);
@@ -2136,8 +2165,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)
@@ -2149,8 +2179,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 6c21d5bb9a..4c6c3d6f20 100644
--- a/tools/xenstore/xenstored_core.h
+++ b/tools/xenstore/xenstored_core.h
@@ -77,6 +77,9 @@ struct connection
        /* Who am I? 0 for socket connections. */
        unsigned int id;
 
+       /* 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 7169da9851..7d348d57f3 100644
--- a/tools/xenstore/xenstored_domain.c
+++ b/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);
 }
 
--
generated by git-patchbot for /home/xen/git/xen.git#staging



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.