112 lines
4.5 KiB
Text
112 lines
4.5 KiB
Text
$NetBSD: patch-XSA322-o,v 1.1 2020/12/17 16:48:12 bouyer Exp $
|
|
|
|
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
|
|
--- tools/ocaml/xenstored/perms.ml.orig
|
|
+++ 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 3cd0097db9..6a998f8764 100644
|
|
--- tools/ocaml/xenstored/process.ml.orig
|
|
+++ tools/ocaml/xenstored/process.ml
|
|
@@ -437,6 +437,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 0ce6f68e8d..101c094715 100644
|
|
--- tools/ocaml/xenstored/store.ml.orig
|
|
+++ 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 30fc874327..183dd2754b 100644
|
|
--- tools/ocaml/xenstored/xenstored.ml.orig
|
|
+++ tools/ocaml/xenstored/xenstored.ml
|
|
@@ -340,6 +340,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
|