[xen staging-4.10] tools/ocaml/xenstored: clean up permissions for dead domains

[patchbot@xxxxxxx]

commit 66e2db1d6ed6dc261658931547454c8065ca9287
Author:     Edwin Török <edvin.torok@xxxxxxxxxx>
AuthorDate: Tue Dec 15 14:44:32 2020 +0100
Commit:     Jan Beulich <jbeulich@xxxxxxxx>
CommitDate: Tue Dec 15 14:44:32 2020 +0100

    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@xxxxxxxxxx>
    Acked-by: Christian Lindig <christian.lindig@xxxxxxxxxx>
---
 tools/ocaml/xenstored/perms.ml     |  9 +++++++++
 tools/ocaml/xenstored/process.ml   |  1 +
 tools/ocaml/xenstored/store.ml     | 16 ++++++++++++++++
 tools/ocaml/xenstored/xenstored.ml |  1 +
 4 files changed, 27 insertions(+)

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 3cd0097db9..6a998f8764 100644
--- 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 98d368d52f..6bbbd05552 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
@@ -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 30fc874327..183dd2754b 100644
--- 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
--
generated by git-patchbot for /home/xen/git/xen.git#staging-4.10