From patchwork Thu Feb 17 11:30:16 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?b?SsO8cmdlbiBHcm/Dnw==?= X-Patchwork-Id: 12749851 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C17CFC433F5 for ; Thu, 17 Feb 2022 11:30:38 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.274687.470246 (Exim 4.92) (envelope-from ) id 1nKezN-000360-FR; Thu, 17 Feb 2022 11:30:21 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 274687.470246; Thu, 17 Feb 2022 11:30:21 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1nKezN-00035t-Ar; Thu, 17 Feb 2022 11:30:21 +0000 Received: by outflank-mailman (input) for mailman id 274687; Thu, 17 Feb 2022 11:30:20 +0000 Received: from se1-gles-flk1-in.inumbo.com ([94.247.172.50] helo=se1-gles-flk1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1nKezM-00035n-PY for xen-devel@lists.xenproject.org; Thu, 17 Feb 2022 11:30:20 +0000 Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by se1-gles-flk1.inumbo.com (Halon) with ESMTPS id fc1f8a24-8fe4-11ec-b215-9bbe72dcb22c; Thu, 17 Feb 2022 12:30:19 +0100 (CET) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 75DE41F37D; Thu, 17 Feb 2022 11:30:18 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 25A4213BBF; Thu, 17 Feb 2022 11:30:18 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id emn0B8oxDmLCXQAAMHmgww (envelope-from ); Thu, 17 Feb 2022 11:30:18 +0000 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" X-Inumbo-ID: fc1f8a24-8fe4-11ec-b215-9bbe72dcb22c DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1645097418; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=BERprfVQLlUU2/0r8f0rOy+5qxqLqk3fnm/xGVBRun4=; b=O1Y0TMtwIkx/68laJyAF3k1ebX172iok7RuvJtJM+mfoUz5ss9R6T2d9x2aox1Fgb7+NVO cHe2A+23FHigp29E5hOKQhBH4n6yEWclO97GWHsFgMcNWxuSYXN3G9KC6Svn9Om9MKESBv djA4EXKFLpt0mLVafUoB0FNpdi6UbGU= From: Juergen Gross To: xen-devel@lists.xenproject.org Cc: Juergen Gross , Andrew Cooper , George Dunlap , Jan Beulich , Julien Grall , Stefano Stabellini , Wei Liu , Anthony PERARD Subject: [PATCH v2] tools/xenstore: add error indicator to ring page Date: Thu, 17 Feb 2022 12:30:16 +0100 Message-Id: <20220217113016.8260-1-jgross@suse.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 In case Xenstore is detecting a malicious ring page modification (e.g. an invalid producer or consumer index set by a guest) it will ignore the connection of that guest in future. Add a new error field to the ring page indicating that case. Add a new feature bit in order to signal the presence of that error field. Move the ignore_connection() function to xenstored_domain.c in order to be able to access the ring page for setting the error indicator. Signed-off-by: Juergen Gross Reviewed-by: Anthony PERARD --- V2: - add some clarifications (Anthony PERARD) --- docs/misc/xenstore-ring.txt | 35 +++++++++++++++++++++++++ tools/xenstore/xenstored_core.c | 43 +++++++------------------------ tools/xenstore/xenstored_core.h | 1 - tools/xenstore/xenstored_domain.c | 34 +++++++++++++++++++++++- tools/xenstore/xenstored_domain.h | 1 + xen/include/public/io/xs_wire.h | 9 +++++++ 6 files changed, 88 insertions(+), 35 deletions(-) diff --git a/docs/misc/xenstore-ring.txt b/docs/misc/xenstore-ring.txt index 16b4d0f5ac..b338b21b19 100644 --- a/docs/misc/xenstore-ring.txt +++ b/docs/misc/xenstore-ring.txt @@ -22,6 +22,7 @@ Offset Length Description 2060 4 Output producer offset 2064 4 Server feature bitmap 2068 4 Connection state +2072 4 Connection error indicator The Input data and Output data are circular buffers. Each buffer is associated with a pair of free-running offsets labelled "consumer" and @@ -66,6 +67,7 @@ The following features are defined: Mask Description ----------------------------------------------------------------- 1 Ring reconnection (see the ring reconnection feature below) +2 Connection error indicator (see connection error feature below) The "Connection state" field is used to request a ring close and reconnect. The "Connection state" field only contains valid data if the server has @@ -78,6 +80,19 @@ Value Description 1 Ring close and reconnect is in progress (see the "ring reconnection feature" described below) +The "Connection error indicator" is used to let the server indicate it has +detected some error that led to deactivation of the connection by the server. +If the feature has been advertised then the "Connection error indicator" may +take the following values (new values might be added in future without them +being advertised as a new feature): + +Value Description +----------------------------------------------------------------- +0 No error, connection is valid +1 Communication problems (event channel not functional) +2 Inconsistent producer or consumer offset +3 Protocol violation (client data package too long) + The ring reconnection feature ============================= @@ -114,3 +129,23 @@ packet boundary. Note that only the guest may set the Connection state to 1 and only the server may set it back to 0. + +The connection error feature +============================ + +The connection error feature allows the server to signal error conditions +leading to a stop of the communication with the client. In case such an error +condition has occurred, the server will set the appropriate error condition in +the Connection error indicator and will stop communication with the client. + +Any value different from 0 is indicating an error. The value used is meant +just for diagnostic purposes. A client reading the error value should be +prepared to see values not described here, as new error cases might be added +in future. + +The server will discard any already read or written packets, in-flight +requests, watches and transactions associated with the connection. + +Depending on the error cause it might be possible that a reconnect via the +ring reconnection feature (if present) can be performed. There is no guarantee +this will succeed. diff --git a/tools/xenstore/xenstored_core.c b/tools/xenstore/xenstored_core.c index 91d3adccb1..6e4022e5da 100644 --- a/tools/xenstore/xenstored_core.c +++ b/tools/xenstore/xenstored_core.c @@ -1455,35 +1455,6 @@ 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. - */ -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; - /* if this is a socket connection, drop it now */ - if (conn->fd >= 0) - talloc_free(conn); -} - static const char *sockmsg_string(enum xsd_sockmsg_type type) { if ((unsigned int)type < ARRAY_SIZE(wire_funcs) && wire_funcs[type].str) @@ -1598,6 +1569,7 @@ static void handle_input(struct connection *conn) { int bytes; struct buffered_data *in; + unsigned int err; if (!conn->in) { conn->in = new_buffer(conn); @@ -1612,8 +1584,10 @@ static void handle_input(struct connection *conn) if (in->used != sizeof(in->hdr)) { bytes = conn->funcs->read(conn, in->hdr.raw + in->used, sizeof(in->hdr) - in->used); - if (bytes < 0) + if (bytes < 0) { + err = XENSTORE_ERROR_RINGIDX; goto bad_client; + } in->used += bytes; if (in->used != sizeof(in->hdr)) return; @@ -1621,6 +1595,7 @@ static void handle_input(struct connection *conn) if (in->hdr.msg.len > XENSTORE_PAYLOAD_MAX) { syslog(LOG_ERR, "Client tried to feed us %i", in->hdr.msg.len); + err = XENSTORE_ERROR_PROTO; goto bad_client; } } @@ -1638,8 +1613,10 @@ static void handle_input(struct connection *conn) bytes = conn->funcs->read(conn, in->buffer + in->used, in->hdr.msg.len - in->used); - if (bytes < 0) + if (bytes < 0) { + err = XENSTORE_ERROR_RINGIDX; goto bad_client; + } in->used += bytes; if (in->used != in->hdr.msg.len) @@ -1649,14 +1626,14 @@ static void handle_input(struct connection *conn) return; bad_client: - ignore_connection(conn); + ignore_connection(conn, err); } static void handle_output(struct connection *conn) { /* Ignore the connection if an error occured */ if (!write_messages(conn)) - ignore_connection(conn); + ignore_connection(conn, XENSTORE_ERROR_RINGIDX); } struct connection *new_connection(const struct interface_funcs *funcs) diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h index 190d2447cd..742812a974 100644 --- a/tools/xenstore/xenstored_core.h +++ b/tools/xenstore/xenstored_core.h @@ -206,7 +206,6 @@ struct node *read_node(struct connection *conn, const void *ctx, struct connection *new_connection(const struct interface_funcs *funcs); struct connection *get_connection_by_id(unsigned int conn_id); -void ignore_connection(struct connection *conn); void check_store(void); void corrupt(struct connection *conn, const char *fmt, ...); diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c index d03c7d93a9..ae065fcbee 100644 --- a/tools/xenstore/xenstored_domain.c +++ b/tools/xenstore/xenstored_domain.c @@ -427,6 +427,38 @@ static void domain_conn_reset(struct domain *domain) domain->interface->rsp_cons = domain->interface->rsp_prod = 0; } +/* + * 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. + */ +void ignore_connection(struct connection *conn, unsigned int err) +{ + struct buffered_data *out, *tmp; + + trace("CONN %p ignored, reason %u\n", conn, err); + + if (conn->domain && conn->domain->interface) + conn->domain->interface->error = err; + + 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; + /* if this is a socket connection, drop it now */ + if (conn->fd >= 0) + talloc_free(conn); +} + static struct domain *introduce_domain(const void *ctx, unsigned int domid, evtchn_port_t port, bool restore) @@ -1305,7 +1337,7 @@ void read_state_connection(const void *ctx, const void *state) * dead. So mark it as ignored. */ if (!domain->port || !domain->interface) - ignore_connection(conn); + ignore_connection(conn, XENSTORE_ERROR_COMM); if (sc->spec.ring.tdomid != DOMID_INVALID) { tdomain = find_or_alloc_domain(ctx, diff --git a/tools/xenstore/xenstored_domain.h b/tools/xenstore/xenstored_domain.h index 1e929b8f8c..4a37de67a0 100644 --- a/tools/xenstore/xenstored_domain.h +++ b/tools/xenstore/xenstored_domain.h @@ -47,6 +47,7 @@ int do_reset_watches(struct connection *conn, struct buffered_data *in); void domain_init(int evtfd); void dom0_init(void); void domain_deinit(void); +void ignore_connection(struct connection *conn, unsigned int err); /* Returns the implicit path of a connection (only domains have this) */ const char *get_implicit_path(const struct connection *conn); diff --git a/xen/include/public/io/xs_wire.h b/xen/include/public/io/xs_wire.h index 4dd6632669..953a0050a3 100644 --- a/xen/include/public/io/xs_wire.h +++ b/xen/include/public/io/xs_wire.h @@ -124,6 +124,7 @@ struct xenstore_domain_interface { XENSTORE_RING_IDX rsp_cons, rsp_prod; uint32_t server_features; /* Bitmap of features supported by the server */ uint32_t connection; + uint32_t error; }; /* Violating this is very bad. See docs/misc/xenstore.txt. */ @@ -135,11 +136,19 @@ struct xenstore_domain_interface { /* The ability to reconnect a ring */ #define XENSTORE_SERVER_FEATURE_RECONNECTION 1 +/* The presence of the "error" field in the ring page */ +#define XENSTORE_SERVER_FEATURE_ERROR 2 /* Valid values for the connection field */ #define XENSTORE_CONNECTED 0 /* the steady-state */ #define XENSTORE_RECONNECT 1 /* guest has initiated a reconnect */ +/* Valid values for the error field */ +#define XENSTORE_ERROR_NONE 0 /* No error */ +#define XENSTORE_ERROR_COMM 1 /* Communication problem */ +#define XENSTORE_ERROR_RINGIDX 2 /* Invalid ring index */ +#define XENSTORE_ERROR_PROTO 3 /* Protocol violation (payload too long) */ + #endif /* _XS_WIRE_H */ /*