tools/ocaml/xenstored: clean up permissions for dead domains

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 ee7fee6bda383a0cd42c6c7c32a6ce82b7c8257a..e8a16221f8fa06d54ab5f9a3af5f6e86db7af891 100644 (file)
--- 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 3cd0097db950c1caa3761dc6f8483a84a8576994..6a998f876418c84fe8b7a7a5cf4976352b10247e 100644 (file)
--- a/tools/ocaml/xenstored/process.ml
+++ b/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 98d368d52fd5d4c3978907aeabd187fb6fdda1d1..6bbbd055529c0409b72c227b5b2486476104743b 100644 (file)
--- 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
@@ -435,6 +442,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 30fc874327d8055815ce413556e11bd3d9410495..183dd2754b76806d558bae04a205ed61722f1ef5 100644 (file)
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/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