[xen master] tools/ocaml/xenstored: clean up permissions for dead domains

[patchbot@xxxxxxx]

commit c46eff921209a2526f0055cdb76fbf69176b729e
Author:     Edwin Török <edvin.torok@xxxxxxxxxx>
AuthorDate: Tue Dec 15 13:36:04 2020 +0100
Commit:     Jan Beulich <jbeulich@xxxxxxxx>
CommitDate: Tue Dec 15 13:36:04 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 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 52b88b3ee1..22d4ac159f 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 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
--
generated by git-patchbot for /home/xen/git/xen.git#master