From: =?UTF-8?q?Edwin=20T=C3=B6r=C3=B6k?= <edvin.torok@citrix.com>
Subject: tools/ocaml/xenstored: clean up permissions for dead domains
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

domain ids are prone to wrapping (15-bits), and with sufficient number
of VMs in a reboot loop it is possible to trigger it.  Xenstore entries
may linger after a domain dies, until a toolstack cleans it up. During
this time there is a window where a wrapped domid could access these
xenstore keys (that belonged to another VM).

To prevent this do a cleanup when a domain dies:
 * walk the entire xenstore tree and update permissions for all nodes
   * if the dead domain had an ACL entry: remove it
   * if the dead domain was the owner: change the owner to Dom0

This is done without quota checks or a transaction. Quota checks would
be a no-op (either the domain is dead, or it is Dom0 where they are not
enforced).  Transactions are not needed, because this is all done
atomically by oxenstored's single thread.

The xenstore entries owned by the dead domain are not deleted, because
that could confuse a toolstack / backends that are still bound to it
(or generate unexpected watch events). It is the responsibility of a
toolstack to remove the xenstore entries themselves.

This is part of XSA-322.

Signed-off-by: Edwin Török <edvin.torok@citrix.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>

diff --git a/tools/ocaml/xenstored/perms.ml b/tools/ocaml/xenstored/perms.ml
index ee7fee6bda..e8a16221f8 100644
--- a/tools/ocaml/xenstored/perms.ml
+++ b/tools/ocaml/xenstored/perms.ml
@@ -58,6 +58,15 @@ let get_other perms = perms.other
 let get_acl perms = perms.acl
 let get_owner perm = perm.owner
 
+(** [remote_domid ~domid perm] removes all ACLs for [domid] from perm.
+* If [domid] was the owner then it is changed to Dom0.
+* This is used for cleaning up after dead domains.
+* *)
+let remove_domid ~domid perm =
+	let acl = List.filter (fun (acl_domid, _) -> acl_domid <> domid) perm.acl in
+	let owner = if perm.owner = domid then 0 else perm.owner in
+	{ perm with acl; owner }
+
 let default0 = create 0 NONE []
 
 let perm_of_string s =
diff --git a/tools/ocaml/xenstored/process.ml b/tools/ocaml/xenstored/process.ml
index f99b9e935c..73e04cc18b 100644
--- a/tools/ocaml/xenstored/process.ml
+++ b/tools/ocaml/xenstored/process.ml
@@ -443,6 +443,7 @@ let do_release con t domains cons data =
 	let fire_spec_watches = Domains.exist domains domid in
 	Domains.del domains domid;
 	Connections.del_domain cons domid;
+	Store.reset_permissions (Transaction.get_store t) domid;
 	if fire_spec_watches
 	then Connections.fire_spec_watches (Transaction.get_root t) cons Store.Path.release_domain
 	else raise Invalid_Cmd_Args
diff --git a/tools/ocaml/xenstored/store.ml b/tools/ocaml/xenstored/store.ml
index 6b6e440e98..3b05128f1b 100644
--- a/tools/ocaml/xenstored/store.ml
+++ b/tools/ocaml/xenstored/store.ml
@@ -89,6 +89,13 @@ let check_owner node connection =
 
 let rec recurse fct node = fct node; List.iter (recurse fct) node.children
 
+(** [recurse_map f tree] applies [f] on each node in the tree recursively *)
+let recurse_map f =
+	let rec walk node =
+		f { node with children = List.rev_map walk node.children |> List.rev }
+	in
+	walk
+
 let unpack node = (Symbol.to_string node.name, node.perms, node.value)
 
 end
@@ -405,6 +412,15 @@ let setperms store perm path nperms =
 		Quota.del_entry store.quota old_owner;
 		Quota.add_entry store.quota new_owner
 
+let reset_permissions store domid =
+	Logging.info "store|node" "Cleaning up xenstore ACLs for domid %d" domid;
+	store.root <- Node.recurse_map (fun node ->
+		let perms = Perms.Node.remove_domid ~domid node.perms in
+		if perms <> node.perms then
+			Logging.debug "store|node" "Changed permissions for node %s" (Node.get_name node);
+		{ node with perms }
+	) store.root
+
 type ops = {
 	store: t;
 	write: Path.t -> string -> unit;
diff --git a/tools/ocaml/xenstored/xenstored.ml b/tools/ocaml/xenstored/xenstored.ml
index 0d355bbcb8..ff9fbbbac2 100644
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -336,6 +336,7 @@ let _ =
 			finally (fun () ->
 				if Some port = eventchn.Event.virq_port then (
 					let (notify, deaddom) = Domains.cleanup domains in
+					List.iter (Store.reset_permissions store) deaddom;
 					List.iter (Connections.del_domain cons) deaddom;
 					if deaddom <> [] || notify then
 						Connections.fire_spec_watches