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 e07508a2e4724b6647d2ee4cf04c22d37714d7a7..f51979926a09e307fbcba7bfbe94f38401c77d95 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 ef32f6699173bb87a1e728b4db17ad5798d64d26..e20767372f0245447d2b8c5fb25fd5bcc50f3920 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
@@ -436,6 +443,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 8f04cda1901675e363a4f7a19d0096a0dcbcab30..369b5036f43dd9485f94158ea460ac9a20ec75ee 100644 (file)
--- a/tools/ocaml/xenstored/xenstored.ml
+++ b/tools/ocaml/xenstored/xenstored.ml
@@ -341,6 +341,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