Xen Security Advisory 233 (CVE-2017-14317) - cxenstored: Race in domain cleanup

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

            Xen Security Advisory CVE-2017-14317 / XSA-233
                               version 3

                  cxenstored: Race in domain cleanup

UPDATES IN VERSION 3
====================

Added metadata file

Public release.

ISSUE DESCRIPTION
=================

When shutting down a VM with a stubdomain, a race in cxenstored may
cause a double-free.

IMPACT
======

The xenstored daemon may crash, resulting in a DoS of any parts of the
system relying on it (including domain creation / destruction,
ballooning, device changes, etc).

VULNERABLE SYSTEMS
==================

All versions of Xen are vulnerable.

Only systems running the C version os xenstored ("xenstored") are
vulnerable; systems running the Ocaml version ("oxenstored") are not
vulnerable.

Only systems running devicemodel stubdomains are vulnerable.  Only x86
HVM guests can use stubdomains.  Therefore ARM systems, x86 systems
running only PV guests, and x86 systems running HVM guests with the
devicemodel not in a stubdomain (eg in dom0), are not vulnerable.

MITIGATION
==========

Running oxenstored will mitigate this issue.  Not using stubdomains
will also mitigate the issue.

CREDITS
=======

This issue was discovered by Eric Chanudet of AIS.

RESOLUTION
==========

Applying the attached patch resolves this issue.

xsa233.patch     xen-unstable, Xen 4.9.x Xen 4.8.x Xen 4.7.x Xen 4.6.x Xen 4.5.x

$ sha256sum xsa233*
66b6f6c0837a5d12a77db7e5cbfd0514968bd47e2d192824da3bc9ddf119bfe0  xsa233.meta
f721cc49ba692b2f36299b631451f51d7340b8b4732f74c98f01cb7a80d8662b  xsa233.patch
$

DEPLOYMENT DURING EMBARGO
=========================

Deployment of the patches and/or mitigations described above (or
others which are substantially similar) is permitted during the
embargo, even on public-facing systems with untrusted guest users and
administrators.

But: Distribution of updated software is prohibited (except to other
members of the predisclosure list).

Predisclosure list members who wish to deploy significantly different
patches and/or mitigations, please contact the Xen Project Security
Team.

(Note: this during-embargo deployment notice is retained in
post-embargo publicly released Xen Project advisories, even though it
is then no longer applicable.  This is to enable the community to have
oversight of the Xen Project Security Team's decisionmaking.)

For more information about permissible uses of embargoed information,
consult the Xen Project community's agreed Security Policy:
  http://www.xenproject.org/security-policy.html
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBCAAGBQJZt80GAAoJEIP+FMlX6CvZVO8IALTEAV/xiPTN1uUPISLQYLmX
6Bu80yrD+5UjVVI01FrkeUfNJBABmxf5q6sTOFeuYctwY6iPMJI46jHda8ugew5j
wnOgtgat0lfQT1/E/C8SsGEHeTULXPHVOaaXRQT55ExhVvEhLvSQV5vd6YNituyq
ow3hYrK3crK3uCOdLyZlxbuHXMFyLIbpoTYnRgXzV/3uLOB5TPsoRzKf4E+Z1Muo
chQXk8OQG+CEYupf00+H/QTvrDLSnf4KT4t4rZXDqUd39QoxV1l9s0daLyMjyJg/
Lu5t1WmcmarZvYICJhWf3Vi2NpaNTyQEeepwUM/XHe+vgHJXzesWyuRoLApmEfE=
=trYV
-----END PGP SIGNATURE-----

_______________________________________________
Xen-announce mailing list
[email protected]
https://lists.xen.org/xen-announce

From: Juergen Gross <[email protected]>
Subject: tools/xenstore: dont unlink connection object twice

A connection object of a domain with associated stubdom has two
parents: the domain and the stubdom. When cleaning up the list of
active domains in domain_cleanup() make sure not to unlink the
connection twice from the same domain. This could happen when the
domain and its stubdom are being destroyed at the same time leading
to the domain loop being entered twice.

Additionally don't use talloc_free() in this case as it will remove
a random parent link, leading eventually to a memory leak. Use
talloc_unlink() instead specifying the context from which the
connection object should be removed.

This is XSA-233.

Reported-by: Eric Chanudet <[email protected]>
Signed-off-by: Juergen Gross <[email protected]>
Reviewed-by: Ian Jackson <[email protected]>

--- a/tools/xenstore/xenstored_domain.c
+++ b/tools/xenstore/xenstored_domain.c
@@ -221,10 +221,11 @@ static int destroy_domain(void *_domain)
 static void domain_cleanup(void)
 {
 	xc_dominfo_t dominfo;
-	struct domain *domain, *tmp;
+	struct domain *domain;
 	int notify = 0;
 
-	list_for_each_entry_safe(domain, tmp, &domains, list) {
+ again:
+	list_for_each_entry(domain, &domains, list) {
 		if (xc_domain_getinfo(*xc_handle, domain->domid, 1,
 				      &dominfo) == 1 &&
 		    dominfo.domid == domain->domid) {
@@ -236,8 +237,12 @@ static void domain_cleanup(void)
 			if (!dominfo.dying)
 				continue;
 		}
-		talloc_free(domain->conn);
-		notify = 0; /* destroy_domain() fires the watch */
+		if (domain->conn) {
+			talloc_unlink(talloc_autofree_context(), domain->conn);
+			domain->conn = NULL;
+			notify = 0; /* destroy_domain() fires the watch */
+			goto again;
+		}
 	}
 
 	if (notify)